Rolling Average class sanity check

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

My main questions are:

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

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

-
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 (maxRememberedNumbers);
this.maxSize = maxRememberedNumbers;
this.currentTotal = 0L;
this.padLock = new Object();
}

private const int defaultMaxRememberedNumbers = 10;

private readonly Queue 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)(obs

To answer your questions:

• 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.

• Yes, it's overkill, since dividing the total by the count would yield the correct answer anyways.

• 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 (maxCapacity);
        _maxSize = maxCapacity;
        _currentTotal = 0L;
        _padLock = new Object();
    }

    private const int DEFAULT_MAX_CAPACITY = 10;

    private readonly Queue _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.

StackExchange Code Review Q#40177, answer score: 6