Moving average implementation

Question

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 < Minimum)
        {
            Minimum = 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<Candle> _candles;
    private Candle _newestCandle;
    private double _sumExceptNewest;

    /**
     * only median supported now
     */
    public MovingAverage(int length)
    {
        _length = length;
        _candles = new Queue<Candle>(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()
    {
        double result = 0;
        foreach (var candle in _candles)
        {
            result += candle.Median;
        }
        result /= _candles.Count;
        Console.WriteLine("checked MA Value = " + Value + " Should be = " + result + 
            " delta (1 = 100%) = " + (Value - result) / result);
    }

}

Answer 1

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.