6
\$\begingroup\$

I just want to make sure that my Rolling (Moving) Average class is understandable and reasonably thread safe.

My main questions are:

  1. Is having totalCounts as a long just for a potential edge case overkill?

  2. Is checking to see if the number of observed counts is 1 (and thus, just return the observed total), also edge case overkill?

  3. Is there any way to minimize the locking in Add Method? (It's not a bottle neck or anything, but I'm just wondering if I might be locking that part more than needed)

--

public sealed class RollingAverage
{
    public RollingAverage()
        : this(defaultMaxRememberedNumbers)
    {
        //
    }

    public RollingAverage(int maxRememberedNumbers)
    {
        if (maxRememberedNumbers <= 0)
        {
            throw new ArgumentException("maxRememberedNumbersmust be greater than 0.", "maxRememberedNumbers");
        }

        this.counts = new Queue<int>(maxRememberedNumbers);
        this.maxSize = maxRememberedNumbers;
        this.currentTotal = 0L;
        this.padLock = new Object();
    }

    private const int defaultMaxRememberedNumbers = 10;

    private readonly Queue<int> counts;
    private readonly int maxSize;

    private long currentTotal;

    private object padLock;

    public void Add(int value)
    {
        lock (this.padLock)
        {
            if (this.counts.Count == this.maxSize)
            {
                this.currentTotal -= (long)this.counts.Dequeue();
            }

            this.counts.Enqueue(value);

            this.currentTotal += (long)this.value;
        }
    }

    public int CurrentAverage
    {
        get
        {
            long lenCounts;
            long observedTotal;

            lock (this.padLock)
            {
                lenCounts = (long)this.counts.Count;
                observedTotal = this.currentTotal;
            }

            if (lenCounts == 0)
            {
                throw new InvalidOperationException("No counts to average.");
            }
            else if (lenCounts == 1)
            {
                return (int)observedTotal;
            }
            else
            {
                return (int)(observedTotal / lenCounts);
            }
        }
    }

    public void Clear()
    {
        lock (this.padLock)
        {
            this.currentTotal = 0L;
            this.counts.Clear();
        }
    }
}

-Edit- Fixed inconsistent use of "this.".

\$\endgroup\$
2
  • \$\begingroup\$ With int it does not matter, but for floating-point computations, the naive formula for a rolling average is prone to unnecessary round-off errors. And this has been known for 50 years. See this. \$\endgroup\$ Commented Jan 27, 2014 at 22:31
  • \$\begingroup\$ Take look at docs.microsoft.com/en-us/dotnet/standard/collections/… if you haven’t considered it \$\endgroup\$
    – rebornx
    Commented Jan 10, 2021 at 12:01

2 Answers 2

6
\$\begingroup\$

To answer your questions:

  1. It's hard to say. If you expect a large number of values or moderate number of large values, you could potentially overflow an int. Otherwise, you may be fine using int instead of long.
  2. Yes, it's overkill, since dividing the total by the count would yield the correct answer anyways.
  3. Unfortunately, no. You need to lock anything which can be mutated by other threads, and you're doing precisely that.

I would do a few things:

  • Use a different naming format for fields than you do for local variables. The most common format I have seen with .NET code is an underscore prefix, though the naming guidelines do not have any particular rule for private fields. Once that is done, you can drop all the this references.

  • Constants should use different casing from fields. SCREAMING CASE is the one I follow and see most often, but the framework isn't really consistent. There appears to be a mix of PascalCase and SCREAMING_CASE, depending on which class you look at in ILDASM, even just within mscorlib.dll.

  • Avoid unnecessary casting. .NET performs implicit casting in many cases, so there is no need to use most of the casts. The only exception is casting the calculated average to int (from long).

  • Use proper exceptions. The exception in the constructor should instead be ArgumentOutOfRangeException, as it more clearly states what the problem was.

  • Returning from within a lock works, so there is no need to define local variables for the total in the CurrentAverage property

    public sealed class RollingAverage
    {
        public RollingAverage()
            : this(DEFAULT_MAX_CAPACITY)
        {
        }
    
        public RollingAverage(int maxCapacity)
        {
            if (maxCapacity <= 0)
            {
                throw new ArgumentOutOfRangeException("maxRememberedNumbersmust be greater than 0.",
                                                        "maxRememberedNumbers");
            }
    
            _counts = new Queue<int>(maxCapacity);
            _maxSize = maxCapacity;
            _currentTotal = 0L;
            _padLock = new Object();
        }
    
        private const int DEFAULT_MAX_CAPACITY = 10;
    
        private readonly Queue<int> _counts;
        private readonly int _maxSize;
    
        private long _currentTotal;
        private object _padLock;
    
        public void Add(int value)
        {
            lock (_padLock)
            {
                if (_counts.Count == _maxSize)
                {
                    _currentTotal -= _counts.Dequeue();
                }
    
                _counts.Enqueue(value);
                _currentTotal += value;
            }
        }
    
        public int CurrentAverage
        {
            get
            {
                lock (_padLock)
                {
                    var lenCounts = _counts.Count;
                    if (lenCounts == 0)
                    {
                        throw new InvalidOperationException("No counts to average.");
                    }
    
                    return (int)(_currentTotal / lenCounts);
                }
            }
        }
    
        public void Clear()
        {
            lock (_padLock)
            {
                _currentTotal = 0L;
                _counts.Clear();
            }
        }
    }
    

Other things to consider

  • Consider using a float or double type for the CurrentAverage property. It is better to preserve the correct decimal answer in case you need a more exact average. You can always truncate the answer back down to an int or float in the caller if you really want.

  • If the ratio of reads/writes heavily favors reads, consider using a ReaderWriterLockSlim. This will allow multiple readers to retrieve the average without blocking one another. Just remember to differentiate between obtaining the lock in reader mode versus writer or upgrade mode.

\$\endgroup\$
1
  • \$\begingroup\$ I'll agree with most of your assessments (especially returning double rather than int for CurrentAverage - let the calling code decide how to deal with the fractional part). But I'm keeping the structure of CurrentAverage, due to the fact that I'm only needing to look at those two values (this.counts.Count and this.currentTotal), so why lock for longer than I need? And I also prefer explicit conversion as a nice way of showing two variables aren't the same type without resorting to Systems Hungarian Notation. \$\endgroup\$
    – Tory
    Commented Jan 27, 2014 at 19:23
2
\$\begingroup\$

Couple of small bits:

  • The arguments to your ArgumentOutOfRangeException exception are in the wrong order.
  • I eliminated the padLock member variable, because the counts member variable is perfectly convenient to use as a lock argument for the usage under consideration.

I agree with what Dan Lyons has to say, but I take a somewhat different tack on naming conventions and this usage. A lot of this comes from StyleCop and ReSharper. Just as a comparison, here's my version:

public sealed class RollingAverage
{
    private const int DefaultMaxCapacity = 10;

    private readonly Queue<int> counts;

    private readonly int maxSize;

    private long currentTotal;

    public RollingAverage() : this(DefaultMaxCapacity)
    {
    }

    public RollingAverage(int maxCapacity)
    {
        if (maxCapacity < 1)
        {
            throw new ArgumentOutOfRangeException(
                "maxCapacity",
                "maxCapacity must be greater than 0.");
        }

        this.counts = new Queue<int>(maxCapacity);
        this.maxSize = maxCapacity;
        this.currentTotal = 0L;
    }

    public double CurrentAverage
    {
        get
        {
            lock (this.counts)
            {
                var lenCounts = this.counts.Count;

                if (lenCounts == 0)
                {
                    throw new InvalidOperationException("No counts to average.");
                }

                return this.currentTotal / (double)lenCounts;
            }
        }
    }

    public void Add(int value)
    {
        lock (this.counts)
        {
            if (this.counts.Count == this.maxSize)
            {
                this.currentTotal -= this.counts.Dequeue();
            }

            this.counts.Enqueue(value);
            this.currentTotal += value;
        }
    }

    public void Clear()
    {
        lock (this.counts)
        {
            this.counts.Clear();
            this.currentTotal = 0L;
        }
    }
}
\$\endgroup\$
4
  • \$\begingroup\$ Sorry to be nit-picky, but I used ArgumentException, not ArgumentOutOfRangeException, which the arguments ARE in the correct order. And if it weren't for currentTotal, I might agree with you about using counts as a lock - though I'd have to think about how that might impact maintenance. It seems useful though, so I think I'll use it in a future project. \$\endgroup\$
    – Tory
    Commented Jan 27, 2014 at 22:19
  • \$\begingroup\$ Whups, that's what I get for looking at both the original code and the one in one of the answers at the same time. Sorry about that. \$\endgroup\$ Commented Jan 27, 2014 at 22:26
  • \$\begingroup\$ Any reason not to use Interlocked.Increment instead of the second lock on the incrementation? \$\endgroup\$
    – Alenros
    Commented Jan 8, 2021 at 6:34
  • 1
    \$\begingroup\$ @Alenros Yes, the reason not to is because there is more than just the increment happening within the lock. \$\endgroup\$ Commented Jan 8, 2021 at 13:24

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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