3
\$\begingroup\$

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);
    }

}
\$\endgroup\$
2
\$\begingroup\$

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.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.