-
Notifications
You must be signed in to change notification settings - Fork 148
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Represent TimeSeries as an interface #14
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -5,21 +5,36 @@ import ( | |
) | ||
|
||
// TimeSeries represents an array of candles | ||
type TimeSeries struct { | ||
Candles []*Candle | ||
type TimeSeries interface { | ||
LastIndex() int | ||
FirstCandle() *Candle | ||
LastCandle() *Candle | ||
GetCandle(int) *Candle | ||
GetCandleData() []*Candle | ||
AddCandle(*Candle) bool | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. in terms of the methods in this interface, I want to make sure we're being deliberate about what we choose, because once we commit to these we can't remove or change any of them. Based on the usages in this repo, I think we could get away with something like this: type ITimeSeries interface {
AddCandle(*Candle) bool
Candles() []*Candle
} the others seem like convenience methods to access data at a particular location. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, I understand your concerns. I guess that was a bit premature decision, disregarding legacy code. And presenting as small interface as possible is also a valid point. As for motivation behind this PR, it's just what I think is usually right: parts that "do" or "provide" stuff are usually interfaces, and parts that circulate around as DTOs are usually structs. The time series looks like a "provider" of Candles to me, hence it's an interface. But you're right, I should have thought it all through more carefully. For instance, some indicators use internal caches that also are slices. If we decide to go the whole way, cache must be more abstract too. For example, "infinite" time series implemented as some kind of ring buffer will not use too much memory, but cache as a slice definitely will. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No worries! feel free to iterate on this and tag me for another review. Thanks for your contribution! It's exciting to see folks wanting to work on this and make it better |
||
} | ||
|
||
// NewTimeSeries returns a new, empty, TimeSeries | ||
func NewTimeSeries() (t *TimeSeries) { | ||
t = new(TimeSeries) | ||
t.Candles = make([]*Candle, 0) | ||
func NewTimeSeries() TimeSeries { | ||
return new(BaseTimeSeries) | ||
} | ||
|
||
// BaseTimeSeries implements TimeSeries using in-memory slice | ||
type BaseTimeSeries struct { | ||
Candles []*Candle | ||
} | ||
|
||
func (ts *BaseTimeSeries) GetCandle(idx int) *Candle { | ||
return ts.Candles[idx] | ||
} | ||
|
||
return t | ||
func (ts *BaseTimeSeries) GetCandleData() []*Candle { | ||
return ts.Candles | ||
} | ||
|
||
// AddCandle adds the given candle to this TimeSeries if it is not nil and after the last candle in this timeseries. | ||
// If the candle is added, AddCandle will return true, otherwise it will return false. | ||
func (ts *TimeSeries) AddCandle(candle *Candle) bool { | ||
func (ts *BaseTimeSeries) AddCandle(candle *Candle) bool { | ||
if candle == nil { | ||
panic(fmt.Errorf("error adding Candle: candle cannot be nil")) | ||
} | ||
|
@@ -32,8 +47,17 @@ func (ts *TimeSeries) AddCandle(candle *Candle) bool { | |
return false | ||
} | ||
|
||
// FirstCandle will return the firstCandle in this series, or nil if this series is empty | ||
func (ts *BaseTimeSeries) FirstCandle() *Candle { | ||
if len(ts.Candles) > 0 { | ||
return ts.Candles[0] | ||
} | ||
|
||
return nil | ||
} | ||
|
||
// LastCandle will return the lastCandle in this series, or nil if this series is empty | ||
func (ts *TimeSeries) LastCandle() *Candle { | ||
func (ts *BaseTimeSeries) LastCandle() *Candle { | ||
if len(ts.Candles) > 0 { | ||
return ts.Candles[len(ts.Candles)-1] | ||
} | ||
|
@@ -42,6 +66,6 @@ func (ts *TimeSeries) LastCandle() *Candle { | |
} | ||
|
||
// LastIndex will return the index of the last candle in this series | ||
func (ts *TimeSeries) LastIndex() int { | ||
func (ts *BaseTimeSeries) LastIndex() int { | ||
return len(ts.Candles) - 1 | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like the idea behind this, but, because the original
TimeSeries
struct was exported, I don't think we can just replace the struct def with an interface def, because it will break every project where someone was previously using a concreteTimeSeries
struct. I think what we need to do is invert the naming here, s.t. the we give the interface another name (maybeITimeSeries
to borrow from C++, but I'm sure you can suggest something better) and keepTimeSeries
bound to the struct def. WDYT?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, there must be a more elegant way. Sorry for bothering you too early. I need to think a bit more about it all.