5
\$\begingroup\$

Working on a game I need to record events compute some values about it (the more the better). The twist is, I need to store as little data as possible. Constant (as in O(1)) weight is required. The following code is not acceptable :

public class Player
{
    private List<float> previousGamesDuration;//Too heavy
    float GetAvgGameTime ();
    float GetStdDevGameTime ();
    float GetMaxGameTime ();
    ...
}

So I ended up having the following :

public class Player
{
    public MagicFloat GameDuration;
}

With the MagicFloat being a subclass of MagicNumber as following.

namespace network
{
    [Serializable]
    public abstract class MagicProperty<T> where T : IComparable
    {
        protected T _last;

        public T Last
        {
            get { return _last; }
            set
            {
                if (Count == 0)
                {
                    Min = value;
                    Max = value;
                }
                Min = value.CompareTo (Min) < 0 ? value : Min;
                Max = value.CompareTo (Max) > 0 ? value : Max;
                Count++;
                UpdateSums (value);
                UpdateSigma ();
                UpdateAverage ();
                _last = value;
            }
        }

        public override string ToString ()
        {
            return string.Format ("Last: {0}, Max: {1}, Min: {2}, Avg: {3}, Count: {4}, Sum: {5}, Sum2: {6}, Sigma: {7}", Last,
                Max, Min, Avg, Count, Sum, Sum2, Sigma);
        }

        protected abstract void UpdateSigma ();

        protected abstract void UpdateSums (T value);

        protected abstract void UpdateAverage ();

        public T Max { get; private set; }

        public T Min { get; private set; }

        public float Avg { get; protected set; }

        public int Count { get; private set; }

        public T Sum { get; protected set; }

        public T Sum2 { get; protected set; }

        public float Sigma { get; protected set; }

        // ReSharper disable once MemberCanBePrivate.Global
        protected MagicProperty ()
        {
        }

        public MagicProperty (T initialValue)
        {
            Last = initialValue;
        }

        #region equality

        protected bool Equals (MagicProperty<T> other)
        {
            return EqualityComparer<T>.Default.Equals (Max, other.Max) && EqualityComparer<T>.Default.Equals (Min, other.Min) &&
                   Avg.Equals (other.Avg) && Count == other.Count && EqualityComparer<T>.Default.Equals (Sum, other.Sum) &&
                   EqualityComparer<T>.Default.Equals (Sum2, other.Sum2) && Sigma.Equals (other.Sigma);
        }

        public override bool Equals (object obj)
        {
            if (ReferenceEquals (null, obj)) return false;
            if (ReferenceEquals (this, obj)) return true;
            if (obj.GetType () != this.GetType ()) return false;
            return Equals ((MagicProperty<T>) obj);
        }

        public override int GetHashCode ()
        {
            unchecked
            {
                int hashCode = EqualityComparer<T>.Default.GetHashCode (Max);
                hashCode = (hashCode * 397) ^ EqualityComparer<T>.Default.GetHashCode (Min);
                hashCode = (hashCode * 397) ^ Avg.GetHashCode ();
                hashCode = (hashCode * 397) ^ Count;
                hashCode = (hashCode * 397) ^ EqualityComparer<T>.Default.GetHashCode (Sum);
                hashCode = (hashCode * 397) ^ EqualityComparer<T>.Default.GetHashCode (Sum2);
                hashCode = (hashCode * 397) ^ Sigma.GetHashCode ();
                return hashCode;
            }
        }

        #endregion
    }


    [Serializable]
    public class MagicInt : MagicProperty<int>
    {
        public MagicInt ()
        {
        }

        public MagicInt (int initialValue)
        {
            _last = initialValue;
        }

        protected override void UpdateSums (int value)
        {
            Sum += value;
            Sum2 += value * value;
        }

        protected override void UpdateSigma ()
        {
            float meanSquared = Avg * Avg;
            Sigma = Mathf.Sqrt ((float) decimal.Divide (Sum2, Count) - meanSquared);
        }

        protected override void UpdateAverage ()
        {
            Avg = (float) decimal.Divide (Sum, Count);
        }
    }

    [Serializable]
    public class MagicFloat : MagicProperty<float>
    {
        public MagicFloat ()
        {
        }

        public MagicFloat (float initialValue)
        {
            _last = initialValue;
        }

        protected override void UpdateSigma ()
        {
            float meanSquared = Avg * Avg;
            Sigma = Mathf.Sqrt ((float) decimal.Divide ((decimal) Sum2, Count) - meanSquared);
        }

        protected override void UpdateSums (float value)
        {
            Sum += value;
            Sum2 += value * value;
        }

        protected override void UpdateAverage ()
        {
            Avg = (float) decimal.Divide ((decimal) Sum, Count);
        }
    }
}

I'm looking for feedbacks and hindsight on the following :

  • MagicProperty and such are poor names, but I lack a good qualifier
  • I don't like having to implement ComputeNewAverage, UpdateSum and UpdateSigma twice. The lack of a generic type common to floats and ints prevents me from doing so in the generic class
  • Any other point that I'd have missed
\$\endgroup\$
  • 1
    \$\begingroup\$ I'd compute Avg from Sum and Count. Sigma = Mathf.Sqrt (sumSquared / Count - meanSquared; looks surprising, not just for the parenthesis. (Check what changes to a question on CR are admissible when, especially "shortly after asking" as opposed to "after the first answer". \$\endgroup\$ – greybeard Jan 11 '17 at 9:22
  • 1
    \$\begingroup\$ Inside MagicProperty class, inside Last there are the following method calls - ComputeNewAverage, UpdateSum and UpdateSigma. But these methods are absent in class definition. Please correct your code. \$\endgroup\$ – Disappointed Jan 11 '17 at 9:41
  • 2
    \$\begingroup\$ The posted code is incomplete \$\endgroup\$ – paparazzo Jan 11 '17 at 10:57
  • 1
    \$\begingroup\$ Come on. Disappointed spelled it out for you. Copy paste your post and see if it works for you. \$\endgroup\$ – paparazzo Jan 11 '17 at 12:06
  • 1
    \$\begingroup\$ Wha do you mean by Too heavy here private List<float> previousGamesDuration; ? \$\endgroup\$ – t3chb0t Jan 12 '17 at 8:55
8
\$\begingroup\$

You can add additional constraint to your generic type :

public abstract class MagicProperty<T> 
        where T : struct, IComparable

It doesn't makes sense to have a public constructor in an abstract class, since abstract classes are instantiated through they're derived types, more appropriate access modifier would be protected.

public MagicProperty(T initialValue)

There is no need of 2 constructors either as you can use optional parameters and have just a single constructor :

protected MagicProperty(T initialValue = default(T))
{
    if (!EqualityComparer<T>.Default.Equals(initialValue, default(T)))
    {
        Last = initialValue;
    }
}

But repeating the absolute same code in all of your derived types constructors is not a nice thing. You should instead inherit the base constructor like this :

protected MagicInt(int initialValue = default(int)) : base(initialValue)
{
}

protected MagicFloat(float initialValue = default(float)) : base(initialValue)
{
}

A lot of your code has repetitive implementation in the derived class and this is due to the generic type parameter since it can't be restricted to numbers only thus apply arithmetic operations to them.

What you can do in this case is use dynamic instead. I would keep the type argument just so you can be sure that your class always operates on the exact same type.

BUT in order for this to work you will need to explicitly specify the type of the value you are assigning to Last :

MagicProperty<float> GameDuration = new MagicProperty<float>();
GameDuration.Last = 123;

You can't do that since the type of 123 right now looks like an Int32, instead of float the proper way of doing this is :

MagicProperty<float> GameDuration = new MagicProperty<float>();
GameDuration.Last = 123f; // <---- f

And also assign 0 to Sum, Sum2, Min, Max in your ctor.

Let's start by changing all of your properties and fields types :

protected dynamic _last;

public dynamic Last
{
    get { return _last; }
    set
    {
        //..
    }
}

public dynamic Max { get; private set; }

public dynamic Min { get; private set; }

public float Avg { get; protected set; }

public int Count { get; private set; }

public dynamic Sum { get; protected set; }

public dynamic Sum2 { get; protected set; }

public float Sigma { get; protected set; }

Next we need to have a way of verifying if the dynamic type matches the original type argument T, let's create a short function to do that for us :

private bool IsSameTypeAsT(Type type)
{
    return type == typeof(T);
}

Since Last is publicly settable we need to have additional check there for the type :

if (!IsSameTypeAsT(value.GetType()))
{
    throw new InvalidCastException("Types don't match");
}

Next let's replace those lines :

Min = value.CompareTo (Min) < 0 ? value : Min;
Max = value.CompareTo (Max) > 0 ? value : Max;

With the predefined methods in the Math class :

Min = Math.Min(value, Min);
Max = Math.Max(value, Max);

This check here :

if (Count == 0)
{
    Min = value;
    Max = value;
}

Is redundant as you're overriding that value on the next line.

Now your property setter looks like this :

set
{
    if (!IsSameTypeAsT(value.GetType()))
    {
        throw new InvalidCastException("Types don't match");
    }
    Min = Math.Min(value, Min);
    Max = Math.Max(value, Max);
    Count++;
    UpdateSums(value);
    UpdateSigma();
    UpdateAverage();
    _last = value;
}

Next we have the abstract UpdateSums(...) method which is implemented the same way in both of your derived types. Let's implement it just once in the base class like this :

protected void UpdateSums(dynamic value)
{
    if (!IsSameTypeAsT(value.GetType()))
    {
        throw new InvalidCastException("Types don't match");
    }
    Sum += value;
    Sum2 += value * value;
}

We also have abstract UpdateAverage() which is implemented again the same way in both derived types :

protected void UpdateAverage()
{
    Avg = (float) decimal.Divide((decimal) Sum, Count);
}

Or you can simply do :

protected void UpdateAverage()
{
    Avg = (float) Sum / Count;
}

Lastly we have abstract UpdateSigma() :

protected void UpdateSigma()
{
    float meanSquared = Avg * Avg;
    Sigma = (float)Math.Sqrt((float)decimal.Divide((decimal)Sum2, Count) - meanSquared);
}

Having all of those methods implemented in your base class, makes all of your derived types useless. Which means you can now have a single non-abstract class which does the job for you, with some final property access modifiers changes your class can look like this :

[Serializable]
public class MagicProperty<T>
    where T : struct, IComparable
{
    private dynamic _last;
    public dynamic Last
    {
        get { return _last; }
        set
        {
            if (!IsSameTypeAsT(value.GetType()))
            {
                throw new InvalidCastException("Types don't match");
            }
            Min = Math.Min(value, Min);
            Max = Math.Max(value, Max);
            Count++;
            UpdateSums(value);
            UpdateSigma();
            UpdateAverage();
            _last = value;
        }
    }

    public dynamic Max { get; private set; }

    public dynamic Min { get; private set; }

    public float Avg { get; private set; }

    public int Count { get; private set; }

    public dynamic Sum { get; private set; }

    public dynamic Sum2 { get; private set; }

    public float Sigma { get; private set; }

    public override string ToString()
    {
        return string.Format(
            "Last: {0}, Max: {1}, Min: {2}, Avg: {3}, Count: {4}, Sum: {5}, Sum2: {6}, Sigma: {7}", Last,
            Max, Min, Avg, Count, Sum, Sum2, Sigma);
    }

    private bool IsSameTypeAsT(Type type)
    {
        return type == typeof(T);
    }

    private void UpdateSigma()
    {
        float meanSquared = Avg * Avg;
        Sigma = (float) Math.Sqrt((float) decimal.Divide((decimal) Sum2, Count) - meanSquared);
    }

    private void UpdateSums(dynamic value)
    {
        if (!IsSameTypeAsT(value.GetType()))
        {
            throw new InvalidCastException("Types don't match");
        }
        Sum += value;
        Sum2 += value * value;
    }

    private void UpdateAverage()
    {
        Avg = (float) Sum / Count;
    }

    public MagicProperty(T initialValue = default(T))
    {
        if (!EqualityComparer<T>.Default.Equals(initialValue, default(T)))
        {
            Last = initialValue;
        }
        Sum = 0;
        Sum2 = 0;
        Min = 0;
        Max = 0;
    }

    #region equality

    private bool Equals(MagicProperty<T> other)
    {
        return EqualityComparer<T>.Default.Equals(Max, other.Max) &&
               EqualityComparer<T>.Default.Equals(Min, other.Min) &&
               Avg.Equals(other.Avg) && Count == other.Count && EqualityComparer<T>.Default.Equals(Sum, other.Sum) &&
               EqualityComparer<T>.Default.Equals(Sum2, other.Sum2) && Sigma.Equals(other.Sigma);
    }

    public override bool Equals(object obj)
    {
        return !ReferenceEquals(null, obj) && (ReferenceEquals(this, obj) || 
               obj.GetType() == GetType() && Equals((MagicProperty<T>) obj));
    }

    public override int GetHashCode()
    {
        unchecked
        {
            int hashCode = EqualityComparer<T>.Default.GetHashCode(Max);
            hashCode = (hashCode * 397) ^ EqualityComparer<T>.Default.GetHashCode(Min);
            hashCode = (hashCode * 397) ^ Avg.GetHashCode();
            hashCode = (hashCode * 397) ^ Count;
            hashCode = (hashCode * 397) ^ EqualityComparer<T>.Default.GetHashCode(Sum);
            hashCode = (hashCode * 397) ^ EqualityComparer<T>.Default.GetHashCode(Sum2);
            hashCode = (hashCode * 397) ^ Sigma.GetHashCode();
            return hashCode;
        }
    }
    #endregion
}
\$\endgroup\$
  • 2
    \$\begingroup\$ The problem with this is that dynamic is slow as snot in C# compared to other implementation possibilities. There's a hell of a lot more reflection that happens with dynamic than there is with even object. \$\endgroup\$ – Der Kommissar Jan 12 '17 at 6:39
  • \$\begingroup\$ @denis Interesting feedback ! Why throwing an exception right away, instead of trying to cast it to (T) value, though ? \$\endgroup\$ – monsieur_h Jan 12 '17 at 9:40
  • \$\begingroup\$ That's a better idea @monsieur_h. \$\endgroup\$ – Denis Jan 12 '17 at 15:50
  • \$\begingroup\$ @EBrown your tests prove that it's not all that slower right ? \$\endgroup\$ – Denis Jan 12 '17 at 21:37
  • \$\begingroup\$ @denis My tests don't prove anything except that all three methods are fairly quick with two numbers. It shows that this method is 20% to 25% slower in the cases I tested. More tests are needed to draw authoritative conclusions. \$\endgroup\$ – Der Kommissar Jan 12 '17 at 21:55
7
\$\begingroup\$

Your Last.set does too much. It is not acceptable as a property.

But that's alright, we're going to rewrite this whole block of code a bit different. I leave it up to you to be the judge of it, but personally I think this is a lot more appropriate than what you currently have.


The first bit is to create a ConsistentValue (or ConsistentNumber, etc.) struct that wraps every numeric type into a consistent struct.

[StructLayout(LayoutKind.Explicit)]
public struct ConsistentValue
{
    [FieldOffset(0)]
    private int _intValue;
    public int IntValue => ValueType == ConsistentValueType.Int ? _intValue : ValueAs(ConsistentValueType.Int).IntValue;

    [FieldOffset(0)]
    private long _longValue;
    public long LongValue => ValueType == ConsistentValueType.Long ? _longValue : ValueAs(ConsistentValueType.Long).LongValue;

    [FieldOffset(0)]
    private float _floatValue;
    public float FloatValue => ValueType == ConsistentValueType.Float ? _floatValue : ValueAs(ConsistentValueType.Float).FloatValue;

    [FieldOffset(0)]
    private double _doubleValue;
    public double DoubleValue => ValueType == ConsistentValueType.Double ? _doubleValue : ValueAs(ConsistentValueType.Double).DoubleValue;

    [FieldOffset(8)]
    private ConsistentValueType _valueType;

    public ConsistentValueType ValueType => _valueType;

    public ConsistentValue(int value)
        : this()
    {
        _intValue = value;
        _valueType = ConsistentValueType.Int;
    }

    public ConsistentValue(long value)
        : this()
    {
        _longValue = value;
        _valueType = ConsistentValueType.Long;
    }

    public ConsistentValue(float value)
        : this()
    {
        _floatValue = value;
        _valueType = ConsistentValueType.Float;
    }

    public ConsistentValue(double value)
        : this()
    {
        _doubleValue = value;
        _valueType = ConsistentValueType.Double;
    }

    public static implicit operator ConsistentValue (int value) => new ConsistentValue(value);

    public static implicit operator ConsistentValue (long value) => new ConsistentValue(value);

    public static implicit operator ConsistentValue (float value) => new ConsistentValue(value);

    public static implicit operator ConsistentValue (double value) => new ConsistentValue(value);

    public static ConsistentValue Sqrt(ConsistentValue value)
    {
        switch (value.ValueType)
        {
            case ConsistentValueType.Int:
                return Math.Sqrt(value._intValue);
            case ConsistentValueType.Long:
                return Math.Sqrt(value._longValue);
            case ConsistentValueType.Float:
                return Math.Sqrt(value._floatValue);
            case ConsistentValueType.Double:
                return Math.Sqrt(value._doubleValue);
        }

        return new ConsistentValue();
    }

    public static ConsistentValue operator +(ConsistentValue left, ConsistentValue right)
    {
        if (left.ValueType == right.ValueType)
        {
            switch (left.ValueType)
            {
                case ConsistentValueType.Int:
                    return left._intValue + right._intValue;
                case ConsistentValueType.Long:
                    return left._longValue + right._longValue;
                case ConsistentValueType.Float:
                    return left._floatValue + right._floatValue;
                case ConsistentValueType.Double:
                    return left._doubleValue + right._doubleValue;
            }
        }
        else
        {
            var newType = PromotionType(left.ValueType, right.ValueType);
            return ValueAs(left, newType) + ValueAs(right, newType);
        }

        return new ConsistentValue();
    }

    public static ConsistentValue operator -(ConsistentValue left, ConsistentValue right)
    {
        if (left.ValueType == right.ValueType)
        {
            switch (left.ValueType)
            {
                case ConsistentValueType.Int:
                    return left._intValue - right._intValue;
                case ConsistentValueType.Long:
                    return left._longValue - right._longValue;
                case ConsistentValueType.Float:
                    return left._floatValue - right._floatValue;
                case ConsistentValueType.Double:
                    return left._doubleValue - right._doubleValue;
            }
        }
        else
        {
            var newType = PromotionType(left.ValueType, right.ValueType);
            return ValueAs(left, newType) - ValueAs(right, newType);
        }

        return new ConsistentValue();
    }

    public static ConsistentValue operator *(ConsistentValue left, ConsistentValue right)
    {
        if (left.ValueType == right.ValueType)
        {
            switch (left.ValueType)
            {
                case ConsistentValueType.Int:
                    return left._intValue * right._intValue;
                case ConsistentValueType.Long:
                    return left._longValue * right._longValue;
                case ConsistentValueType.Float:
                    return left._floatValue * right._floatValue;
                case ConsistentValueType.Double:
                    return left._doubleValue * right._doubleValue;
            }
        }
        else
        {
            var newType = PromotionType(left.ValueType, right.ValueType);
            return ValueAs(left, newType) * ValueAs(right, newType);
        }

        return new ConsistentValue();
    }

    public static ConsistentValue operator /(ConsistentValue left, ConsistentValue right)
    {
        if (left.ValueType == right.ValueType)
        {
            switch (left.ValueType)
            {
                case ConsistentValueType.Int:
                    return left._intValue / right._intValue;
                case ConsistentValueType.Long:
                    return left._longValue / right._longValue;
                case ConsistentValueType.Float:
                    return left._floatValue / right._floatValue;
                case ConsistentValueType.Double:
                    return left._doubleValue / right._doubleValue;
            }
        }
        else
        {
            var newType = PromotionType(left.ValueType, right.ValueType);
            return ValueAs(left, newType) / ValueAs(right, newType);
        }

        return new ConsistentValue();
    }

    public static bool operator <(ConsistentValue left, ConsistentValue right)
    {
        if (left.ValueType == right.ValueType)
        {
            switch (left.ValueType)
            {
                case ConsistentValueType.Int:
                    return left._intValue < right._intValue;
                case ConsistentValueType.Long:
                    return left._longValue < right._longValue;
                case ConsistentValueType.Float:
                    return left._floatValue < right._floatValue;
                case ConsistentValueType.Double:
                    return left._doubleValue < right._doubleValue;
            }
        }
        else
        {
            var newType = PromotionType(left.ValueType, right.ValueType);
            return ValueAs(left, newType) < ValueAs(right, newType);
        }

        return false;
    }

    public static bool operator >(ConsistentValue left, ConsistentValue right)
    {
        if (left.ValueType == right.ValueType)
        {
            switch (left.ValueType)
            {
                case ConsistentValueType.Int:
                    return left._intValue > right._intValue;
                case ConsistentValueType.Long:
                    return left._longValue > right._longValue;
                case ConsistentValueType.Float:
                    return left._floatValue > right._floatValue;
                case ConsistentValueType.Double:
                    return left._doubleValue > right._doubleValue;
            }
        }
        else
        {
            var newType = PromotionType(left.ValueType, right.ValueType);
            return ValueAs(left, newType) > ValueAs(right, newType);
        }

        return false;
    }

    public static bool operator ==(ConsistentValue left, ConsistentValue right)
    {
        if (left.ValueType == right.ValueType)
        {
            switch (left.ValueType)
            {
                case ConsistentValueType.Int:
                    return left._intValue == right._intValue;
                case ConsistentValueType.Long:
                    return left._longValue == right._longValue;
                case ConsistentValueType.Float:
                    return left._floatValue == right._floatValue;
                case ConsistentValueType.Double:
                    return left._doubleValue == right._doubleValue;
            }
        }
        else
        {
            var newType = PromotionType(left.ValueType, right.ValueType);
            return ValueAs(left, newType) == ValueAs(right, newType);
        }

        return false;
    }

    public static bool operator !=(ConsistentValue left, ConsistentValue right) =>
        !(left == right);

    public static ConsistentValue ValueAs(ConsistentValue value, ConsistentValueType newType)
    {
        if (value.ValueType == newType)
        {
            return value;
        }

        switch (value.ValueType)
        {
            case ConsistentValueType.Int:
                return ValueAs(value._intValue, newType);
            case ConsistentValueType.Long:
                return ValueAs(value._longValue, newType);
            case ConsistentValueType.Float:
                return ValueAs(value._floatValue, newType);
            case ConsistentValueType.Double:
                return ValueAs(value._doubleValue, newType);
        }

        return new ConsistentValue();
    }

    public static ConsistentValue ValueAs<T>(T value, ConsistentValueType newType)
        where T : IConvertible
    {
        switch (newType)
        {
            case ConsistentValueType.Int:
                return (int)Convert.ChangeType(value, typeof(int));
            case ConsistentValueType.Long:
                return (long)Convert.ChangeType(value, typeof(long));
            case ConsistentValueType.Float:
                return (float)Convert.ChangeType(value, typeof(float));
            case ConsistentValueType.Double:
                return (double)Convert.ChangeType(value, typeof(double));
        }

        return new ConsistentValue();
    }

    public static ConsistentValueType PromotionType(ConsistentValueType left, ConsistentValueType right)
    {
        // Read more about promotions: https://msdn.microsoft.com/en-us/library/aa691330(v=vs.71).aspx
        if (left == ConsistentValueType.Double || right == ConsistentValueType.Double)
        {
            return ConsistentValueType.Double;
        }

        if (left == ConsistentValueType.Float || right == ConsistentValueType.Float)
        {
            return ConsistentValueType.Float;
        }

        if (left == ConsistentValueType.Long || right == ConsistentValueType.Long)
        {
            return ConsistentValueType.Long;
        }

        return ConsistentValueType.Int;
    }

    public override string ToString()
    {
        switch (ValueType)
        {
            case ConsistentValueType.Int:
                return _intValue.ToString();
            case ConsistentValueType.Long:
                return _longValue.ToString();
            case ConsistentValueType.Float:
                return _floatValue.ToString();
            case ConsistentValueType.Double:
                return _doubleValue.ToString();
        }

        return null;
    }
}

public enum ConsistentValueType
{
    Int,
    Long,
    Float,
    Double
}

Is this long? You're darn right it is. Is it necessary? For my plans, yes. I only implemented the four most common types, but adding the rest is trivial.

So once we have this class (and an enum as you see) it's easy to use:

public class MagicValue
{
    public ConsistentValue Last { get; private set; }

    public int Count { get; set; }

    public ConsistentValue Min { get; private set; }

    public ConsistentValue Max { get; private set; }

    public ConsistentValue Sum { get; private set; }

    public ConsistentValue SumSquared { get; private set; }

    public float Avg { get; private set; }

    public float Sigma { get; private set; }

    public void UpdateLast(ConsistentValue value)
    {
        Last = value;

        if (Count == 0)
        {
            Min = value;
            Max = value;
        }

        Min = value < Min ? value : Min;
        Max = value > Max ? value : Max;

        Count++;

        Sum += value;
        SumSquared += value * value;

        var inverseCount = 1f / Count;

        var meanSquared = Avg * Avg;
        Sigma = (float)Math.Sqrt((SumSquared.FloatValue * inverseCount) - meanSquared);

        Avg = Sum.FloatValue * inverseCount;
    }

    public override string ToString ()
    {
        return string.Format ("Last: {0}, Max: {1}, Min: {2}, Avg: {3}, Count: {4}, Sum: {5}, SumSquared: {6}, Sigma: {7}", Last,
            Max, Min, Avg, Count, Sum, SumSquared, Sigma);
    }
}

That's it. I did a comparison of your version vs. my version:

var intType = new MagicInt();

intType.Last = 5;
intType.Last = 10;

Console.WriteLine(intType.ToString());

var myType = new MagicValue();

myType.UpdateLast(5);
myType.UpdateLast(10);

Console.WriteLine(myType.ToString());

Real simple, output:

Last: 10, Max: 10, Min: 5, Avg: 7.5, Count: 2, Sum: 15, Sum2: 125, Sigma: 6.123724
Last: 10, Max: 10, Min: 5, Avg: 7.5, Count: 2, Sum: 15, SumSquared: 125, Sigma: 6.12372435695795

I've yet to test performance, but this should make it easier to work with. You don't need to know (or even care) what type the value being passed to UpdateLast is, and if you change types mid-operation it supports it. If you start passing int types in the beginning, but pass a double later, it will upgrade all the internal types for free.

Theoretically this should outperform the dynamic version proposed in the other answer, and it avoids the wrong assumptions. You can only give ConsistentValue the types it supports, which means you are less prone to errors.

We avoid generics almost entirely (only used in one location in ConsistentValue), we avoid dynamic entirely, we avoid unnecessary casting entirely. Overall, we saved ourselves from a lot of pain.

Even better: ConsistentValue is extremely easy to unit test. And now MagicValue is as well.

You could also define an instance method for ValueAs as well (someValue.ValueAs(ConsistentValueType.Long)), would save a little typing in the future.


I would consider changing the name from MagicValue to something like StatisticCalculator. It doesn't matter how it does it, only that it calculates statistics of the data we're working with.


Benchmarking

Because I'm an overachiever and I love seeing how these things stack up, I tested some benchmarks of each method against the others.

This first table is using MagicInt, MagicProperty<int> and MagicValue with only int types supplied to it:

       Method |      Mean |    StdDev | Scaled | Scaled-StdDev |  Gen 0 | Allocated |
------------- |---------- |---------- |------- |-------------- |------- |---------- |
 EBrownMethod | 1.9509 us | 0.0088 us |   1.08 |          0.01 | 0.0493 |     788 B |
     Original | 1.7986 us | 0.0049 us |   1.00 |          0.00 | 0.0112 |     572 B |
  DenisMethod | 2.1486 us | 0.0066 us |   1.19 |          0.00 | 0.0804 |     884 B |

So what's interesting is that the method I proposed here was not that much slower than the original method, but shares a single implementation. We have the exact same compatibility level as well.

And as my original claim stated: dynamic is slower.

This second table is MagicFloat, MagicProperty<float> and MagicValue with only float types supplied to it:

       Method |      Mean |    StdDev | Scaled | Scaled-StdDev |  Gen 0 | Allocated |
------------- |---------- |---------- |------- |-------------- |------- |---------- |
 EBrownMethod | 2.2574 us | 0.0090 us |   0.94 |          0.01 | 0.0381 |     692 B |
     Original | 2.3928 us | 0.0151 us |   1.00 |          0.00 | 0.0153 |     572 B |
  DenisMethod | 3.0193 us | 0.0067 us |   1.26 |          0.01 | 0.1088 |   1.08 kB |

Even more curious: the MagicValue method I proposed was the fastest. I was not expecting that at all, and we can see that the dynamic method is still the slowest.

When I expanded the int tests to 1000 iterations, the gap for the dynamic version grew much more:

       Method |        Mean |    StdDev | Scaled | Scaled-StdDev |   Gen 0 | Allocated |
------------- |------------ |---------- |------- |-------------- |-------- |---------- |
 EBrownMethod | 205.8873 us | 1.3600 us |   1.06 |          0.01 |       - |  48.76 kB |
     Original | 194.8517 us | 0.6223 us |   1.00 |          0.00 |       - |  24.59 kB |
  DenisMethod | 314.7668 us | 1.5765 us |   1.62 |          0.01 | 19.9219 | 156.64 kB |

And the float test difference grew a lot.

       Method |        Mean |    StdDev | Scaled | Scaled-StdDev |   Gen 0 | Allocated |
------------- |------------ |---------- |------- |-------------- |-------- |---------- |
 EBrownMethod |  94.6512 us | 0.1268 us |   0.32 |          0.00 |       - |     776 B |
     Original | 294.3016 us | 0.4353 us |   1.00 |          0.00 |       - |  24.61 kB |
  DenisMethod | 407.5140 us | 2.3531 us |   1.38 |          0.01 | 11.0677 | 156.85 kB |

Magic. ;) I am betting that the float performs fastest with my method because MagicFloat does all that casting to decimal and such. If we remove that, we get a slightly different result (still favours my version):

protected override void UpdateSigma()
{
    float meanSquared = Avg * Avg;
    Sigma = (float)Math.Sqrt((Sum2 / Count) - meanSquared);
}

Results in:

       Method |       Mean |    StdDev | Scaled | Scaled-StdDev |     Gen 0 | Allocated |
------------- |----------- |---------- |------- |-------------- |---------- |---------- |
 EBrownMethod |  9.2216 ms | 0.0501 ms |   0.42 |          0.00 |         - |   1.02 kB |
     Original | 21.9658 ms | 0.0451 ms |   1.00 |          0.00 |         - |    2.4 MB |
  DenisMethod | 43.5882 ms | 0.2850 ms |   1.98 |          0.01 | 2370.8333 |   15.6 MB |

So, we can see (it's heavily demonstrated) that there are far fewer allocations with this version, and in some cases it's significantly faster than the original version.

These next tests use far more iterations.

If we fix MagicFloat.UpdateAverage():

protected override void UpdateAverage()
{
    Avg = Sum / Count;
}

We get:

       Method |       Mean |    StdErr |    StdDev | Scaled | Scaled-StdDev |     Gen 0 | Allocated |
------------- |----------- |---------- |---------- |------- |-------------- |---------- |---------- |
 EBrownMethod | 10.0894 ms | 0.3284 ms | 1.3539 ms |   3.63 |          0.47 |         - |   1.02 kB |
     Original |  2.7784 ms | 0.0017 ms | 0.0067 ms |   1.00 |          0.00 |  376.0417 |    2.4 MB |
  DenisMethod | 43.4391 ms | 0.0236 ms | 0.0915 ms |  15.63 |          0.05 | 2320.8333 |   15.6 MB |

And now my version is back to being slower.


What does any of this prove? When the OP's code is optimized it results in a very nice benchmark, but it also requires a different set of work to add new types. Once we optimize everything, it comes down to what you want the best. Do you want one class that magically handles everything? A class you have to derive for each type you need? A single class with no additional references with a magic type applied to it?

\$\endgroup\$
  • \$\begingroup\$ About 'Last.Set doing too much' how is that not the purpose of properties ? What's the advantage of a function instead (performance-wise)? Also, can you explain the reason behing the memory alignment of the struct ? In essence, why did you align ConsistentValueType at 8 ? Also, why do \$\endgroup\$ – monsieur_h Jan 12 '17 at 9:32
  • 4
    \$\begingroup\$ @monsieur_h The Last.set has too many responsibilities is the problem. For a property setter it's too complex and can throw unintended exceptions. The ConssitentValueType is aligned at 8 bytes because long and double are 8 bytes wide, and are aligned at 0. By aligning all the types to the same boundary we shrink the total data space requirement of the struct. (Since only one will contain a valid value at any given point in time.) \$\endgroup\$ – Der Kommissar Jan 12 '17 at 17:10
4
\$\begingroup\$

I think you're making this more complex then necessary. This class just needs some clean-up starting with leaving only one protected constructor that takes the initial value.

The UpdateX methods shouldn't actually update the properties, there is a risk that you may forget to update some or update the wrong one. The derived types shouldn't mess-up with its base properties if this is not absolutely requried. It's safer to have CalcX methods that return the new value and you set the properties only once.

You do _value = value; after updating all other properties. Is this intended? I believe you need to update it first and then recalc the rest. I moved it upwards.

The name RecordedValue seems to be fine. In my option you should change the name of the main property and field to Value.

I upgraded Sum2 to SquareSum - seems to be a better name.

[Serializable]
public abstract class RecordedValue<T> where T : IComparable
{
    private T _value;

    protected RecordedValue(T value)
    {
        Value = value;
    }

    public T Value
    {
        get { return _value; }
        set
        {
            if (Count == 0)
            {
                Min = value;
                Max = value;
            }
            _value = value;
            Min = value.CompareTo(Min) < 0 ? value : Min;
            Max = value.CompareTo(Max) > 0 ? value : Max;
            Count++;
            Sum = CalcSum();
            SquareSum = CalcSquareSum();
            Sigma = CalcSigma();
            Avg = CalcAvg();            
        }
    }

    public T Max { get; private set; }

    public T Min { get; private set; }

    public float Avg { get; private set; }

    public int Count { get; private set; }

    public T Sum { get; private set; }

    public T SquareSum { get; private set; }

    public float Sigma { get; private set; }


    protected abstract float CalcSigma();

    protected abstract T CalcSum();

    protected abstract T CalcSquareSum();

    protected abstract float CalcAvg();

    public override string ToString()
    {
        return string.Format("Value: {0}, Max: {1}, Min: {2}, Avg: {3}, Count: {4}, Sum: {5}, Square: {6}, Sigma: {7}",
            Value, Max, Min, Avg, Count, Sum, SquareSum, Sigma);
    }


    #region equality

    ...

    #endregion
}

Now you have only four methods that need to be overriden and implemented. This is much easier to implement and less error-prone.

If you want to have default constructor for the custom type then you can call the main constructor with a default value and it calls the base constructor. Make use of constructor chanining.

You should use full-type names. It's called Int32 and not Int.

If you care for peformance then you shouldn't use decimal to avoid a division with /. Decimal is not a native type and has some overhead. See @EBrown's benchmarks.

[Serializable]
public class RecordedInt32 : RecordedValue<int>
{
    public RecordedInt32(int initialValue) : base(initialValue) { }

    public RecordedInt32() : this(default(int)) { }

    protected override int CalcSum()
    {
        return Sum + Value;
    }

    protected override int CalcSquareSum()
    {
        return SquareSum + Value * Value;
    }

    protected override float CalcSigma()
    {
        float meanSquared = Avg * Avg;
        return (float)Math.Sqrt(((float)SquareSum / Count) - meanSquared);
    }

    protected override float CalcAvg()
    {
        return (float)Sum / Count;
    }
}
\$\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.