patterncsharpMinor
Moving average implementation
Viewed 0 times
implementationmovingaverage
Problem
I've created a
How would you improve it for better performance?
```
interface Indicator
{
double? Value { get; }
}
public class Candle
{
public Candle(double value)
{
Minimum = value;
Maximum = value;
}
public void ValueUpdated(double value)
{
if (value Maximum)
{
Maximum = value;
}
}
public double Maximum { get; private set; }
public double Minimum { get; private set; }
public double Median
{
get { return (Maximum + Minimum) / 2; }
}
}
public class MovingAverage : Indicator
{
private int _length;
public Queue _candles;
private Candle _newestCandle;
private double _sumExceptNewest;
/**
* only median supported now
*/
public MovingAverage(int length)
{
_length = length;
_candles = new Queue(length);
}
public void Add(double val)
{
if (_candles.Count == _length)
{
_sumExceptNewest -= _candles.Dequeue().Median;
}
if (_candles.Count > 0)
{
_sumExceptNewest += _newestCandle.Median;
}
// TODO: avoid new
_newestCandle = new Candle(val);
_candles.Enqueue(_newestCandle);
RecalculateValue();
}
public void Modify(double val)
{
if (_newestCandle == null)
{
Add(val);
}
else
{
_newestCandle.ValueUpdated(val);
RecalculateValue();
}
}
private void RecalculateValue()
{
Value = (_sumExceptNewest + _newestCandle.Median)/_candles.Count;
}
public double? Value { get; private set; }
public void CheckValueAndPrintDelta()
{
MovingAverage class, but it is not optimized for performance at all. I care about performance a little bit because I'm using this class in trading (I collocate on exchange and so on and so on; let's talk about performance of this particular class).How would you improve it for better performance?
```
interface Indicator
{
double? Value { get; }
}
public class Candle
{
public Candle(double value)
{
Minimum = value;
Maximum = value;
}
public void ValueUpdated(double value)
{
if (value Maximum)
{
Maximum = value;
}
}
public double Maximum { get; private set; }
public double Minimum { get; private set; }
public double Median
{
get { return (Maximum + Minimum) / 2; }
}
}
public class MovingAverage : Indicator
{
private int _length;
public Queue _candles;
private Candle _newestCandle;
private double _sumExceptNewest;
/**
* only median supported now
*/
public MovingAverage(int length)
{
_length = length;
_candles = new Queue(length);
}
public void Add(double val)
{
if (_candles.Count == _length)
{
_sumExceptNewest -= _candles.Dequeue().Median;
}
if (_candles.Count > 0)
{
_sumExceptNewest += _newestCandle.Median;
}
// TODO: avoid new
_newestCandle = new Candle(val);
_candles.Enqueue(_newestCandle);
RecalculateValue();
}
public void Modify(double val)
{
if (_newestCandle == null)
{
Add(val);
}
else
{
_newestCandle.ValueUpdated(val);
RecalculateValue();
}
}
private void RecalculateValue()
{
Value = (_sumExceptNewest + _newestCandle.Median)/_candles.Count;
}
public double? Value { get; private set; }
public void CheckValueAndPrintDelta()
{
Solution
If performance of this code is critical, then it could make sense to avoid heap allocations for
Though mutable value types are evil, so I would also refactor
I don't see any other potential performance issue in your code. But when it comes to performance, you should always rely on profiling, not your (or someone else's) intuition.
Also, I don't like some names of your methods. Specifically:
-
-
Candles. I think the most reasonable way to do that would be make Candle into a struct.Though mutable value types are evil, so I would also refactor
Candle to be immutable. This also means the implementation of _newestCandle would have to change, probably into a pair of double fields (or, alternatively, a separate mutable and resettable class).I don't see any other potential performance issue in your code. But when it comes to performance, you should always rely on profiling, not your (or someone else's) intuition.
Also, I don't like some names of your methods. Specifically:
-
ValueUpdated. Method names should usually be in the form “do something”, not “something happened”. So I think a better name would be UpdateValue.-
Add, Modify. These are the two fundamental operations of your MovingAverage and I think that those names don't express the meaning well. I would call them something like MoveAndSetCurrent and SetCurrent, respectively. Though such naming indicates that the fundamental operations should rather be Move and SetCurrent.Context
StackExchange Code Review Q#47542, answer score: 2
Revisions (0)
No revisions yet.