HiveBrain v1.2.0
Get Started
← Back to all entries
patterncsharpMinor

Moving average implementation

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
implementationmovingaverage

Problem

I've created a 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 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.