Rolling Average class sanity check

Question

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

Answer 1

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.

Answer 2

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