8
\$\begingroup\$

In my project, I have a type BalanceByBucket that is an immutable type and has a bunch of methods.

I have another class called a FundBalance which is basically just a BalanceByBucket that has an extra int FundID property applied to it. I want this FundBalance to also have access to all the same methods as BalanceByBucket, but remain immutable and return FundBalances instead of BalanceByBuckets. Is there a better way to implement this so that FundBalance can do everything a BalanceByBucket can do without having to reimplement every method?

Below are the classes we have in place. (Currently they are structs, but we are considering changing them to classes since they have grown in storage and complexity recently)

BalanceByBucket:

public struct BalanceByBucket : IEquatable<BalanceByBucket>
{
    private static readonly IEnumerable<BucketType> _AllBucketTypes = Enum.GetValues(typeof(BucketType)).Cast<BucketType>();
    private static readonly BalanceByBucket zeroBalance = new BalanceByBucket(0);

    public static BalanceByBucket Zero { get { return zeroBalance; } }
    public static IEnumerable<BucketType> AllBucketTypes { get { return _AllBucketTypes; } }

    private ImmutableDictionary<BucketType, decimal> _valueDictionary;

    private decimal _total;
    public decimal Total { get { return _total; } }

    public decimal this[BucketType bucket]
    {
        get
        {
            if (_valueDictionary == null || !_valueDictionary.ContainsKey(bucket))
                return 0;

            return _valueDictionary[bucket];
        }
    }


    /// <summary>
    /// This is the base constructor. all other constructors should point to this one.
    /// </summary>
    /// <param name="values"></param>
    public BalanceByBucket(Dictionary<BucketType, decimal> values)
    {
        var dict = new Dictionary<BucketType, decimal>();

        foreach (var item in values)
            dict.Add(item.Key, item.Value);

        _valueDictionary = dict.ToImmutableDictionary();

        _total = _valueDictionary.Sum(x => x.Value);
    }


    private BalanceByBucket(decimal initValue)
        : this(AllBucketTypes.ToDictionary(x => x, x => initValue))
    {
    }

    private BalanceByBucket(IEnumerable<KeyValuePair<BucketType, decimal>> bucketsWithValue)
        : this(bucketsWithValue
            .GroupBy(x => x.Key)
            .Select(x => new
            {
                x.Key,
                Value = x.Sum(y => y.Value)
            })
            .ToDictionary(x => x.Key, x => x.Value))
    {
    }
    public BalanceByBucket(decimal amount, BucketType bucket)
        : this(new Dictionary<BucketType, decimal>
        {
            {bucket, amount}
        })
    {
    }

    public BalanceByBucket GetBucketProportions()
    {
        if (this == zeroBalance)
            return zeroBalance;

        //have to copy local to use in lambda - DJL 12/2/2014
        var total = Total;
        return Apply((value) => value / total);
    }

    /// <summary>
    /// Given a function f:decimal -> decimal, when applied to a balance by bucket with bucket values {x_1, x_2, ... , x_n} returns a new balance by bucket with bucket valeus {f(x_1), f(x_2),..., f(x_n)}
    /// </summary>
    /// <param name="aggFunc"></param>
    /// <returns></returns>
    public BalanceByBucket Apply(Func<decimal, decimal> aggFunc)
    {
        if (aggFunc == null)
            throw new ArgumentNullException();

        if (_valueDictionary == null)
            return BalanceByBucket.Zero.Apply(aggFunc);

        return new BalanceByBucket(_valueDictionary.Select(entry => new KeyValuePair<BucketType, decimal>(entry.Key, aggFunc(entry.Value))));
    }

    public BalanceByBucket Apply(Func<decimal, decimal, decimal> aggFunc, BalanceByBucket y)
    {
        return Apply(aggFunc, this, y);
    }

    public static BalanceByBucket Apply(Func<decimal, decimal> aggFunc, BalanceByBucket x)
    {
        return x.Apply(aggFunc);
    }

    public static BalanceByBucket Apply(Func<decimal, decimal, decimal> aggFunc, BalanceByBucket x, BalanceByBucket y)
    {
        var aggregated = new Dictionary<BucketType, decimal>(AllBucketTypes.Count());
        foreach (BucketType bt in AllBucketTypes)
        {
            aggregated[bt] = aggFunc(x[bt], y[bt]);
        }
        return new BalanceByBucket(aggregated);
    }

    public BalanceByBucket RoundTo(int places)
    {
        return Apply((value) => Math.Round(value, places));
    }

    public decimal Max()
    {
        if (_valueDictionary == null)
            return 0;

        return _valueDictionary.Values.Max();
    }

    public decimal Min()
    {
        if (_valueDictionary == null)
            return 0;

        return _valueDictionary.Values.Min();
    }

    public decimal AbsoluteMax()
    {
        return this.Apply(x => Math.Abs(x)).Max();
    }

    public BalanceByBucket AddUnitsFrom(ITransactionLog transaction)
    {
        return new BalanceByBucket(transaction.Units, transaction.BucketType);
    }

    public BalanceByBucket AddDollarsFrom(ITransactionLog transaction)
    {
        var units = BalanceByBucket.Zero.AddUnitsFrom(transaction);

        if (units.AbsoluteMax() > 0)
            return units * (transaction.DollarAmount / transaction.Units);

        return BalanceByBucket.Zero;
    }

    public static BalanceByBucket CreateFrom(IEnumerable<FundBalance> balances)
    {
        return new BalanceByBucket(
            balances.Select(x => x.Buckets)
            .Where(x => x._valueDictionary != null)
            .SelectMany(x => x._valueDictionary
                .Select(entry => new KeyValuePair<BucketType, decimal>(entry.Key, entry.Value))));
    }

    public BalanceByBucket GreaterOfByBucket(BalanceByBucket y)
    {
        return GreaterOfByBucket(this, y);
    }

    public static BalanceByBucket GreaterOfByBucket(BalanceByBucket x, BalanceByBucket y)
    {
        var dict = new Dictionary<BucketType, decimal>();
        foreach (BucketType bt in Enum.GetValues(typeof(BucketType)))
        {
            dict[bt] = Math.Max(x[bt], y[bt]);
        }
        return new BalanceByBucket(dict);
    }

    public static BalanceByBucket LesserOfByBucket(BalanceByBucket x, BalanceByBucket y)
    {
        var dict = new Dictionary<BucketType, decimal>();
        foreach (BucketType bt in Enum.GetValues(typeof(BucketType)))
        {
            dict[bt] = Math.Min(x[bt], y[bt]);
        }
        return new BalanceByBucket(dict);
    }

    /// <summary>
    /// Divides each bucket pairwise, but returns zero for any bucket where the demonimator would be zero.
    /// </summary>
    public static BalanceByBucket DivideOrReturnZero(BalanceByBucket x, BalanceByBucket y)
    {
        return Apply((a, b) => b == 0 ? 0 : a / b, x, y);
    }

    /// <summary>
    /// Pairwise subtraction between two balances by bucket.
    /// </summary>
    public static BalanceByBucket operator -(BalanceByBucket x, BalanceByBucket y)
    {
        return Apply((a, b) => a - b, x, y);
    }

    /// <summary>
    /// Negation of each bucket balance
    /// </summary>
    public static BalanceByBucket operator -(BalanceByBucket x)
    {
        return Apply(a => -a, x);
    }

    /// <summary>
    /// Pairwise addition between two balances by bucket.
    /// </summary>
    public static BalanceByBucket operator +(BalanceByBucket x, BalanceByBucket y)
    {
        return Apply((a, b) => a + b, x, y);
    }

    /// <summary>
    /// Pairwise multiplication between two balances by bucket.
    /// </summary>
    public static BalanceByBucket operator *(BalanceByBucket x, BalanceByBucket y)
    {
        return Apply((a, b) => a * b, x, y);
    }

    /// <summary>
    /// Pairwise division between two balances by bucket
    /// </summary>
    public static BalanceByBucket operator /(BalanceByBucket x, BalanceByBucket y)
    {
        if (x._valueDictionary.Any(bal => bal.Value == 0))
            throw new DivideByZeroException();

        return Apply((a, b) => a / b, x, y);
    }

    /// <summary>
    /// Returns a new balance with each bucket multiplied by a given scalar
    /// </summary>
    public static BalanceByBucket operator *(BalanceByBucket x, decimal scalar)
    {
        return Apply(a => a * scalar, x);
    }

    /// <summary>
    /// Returns a new balance with each bucket multiplied by a given scalar
    /// </summary>
    public static BalanceByBucket operator *(decimal scalar, BalanceByBucket x)
    {
        return x * scalar;
    }

    /// <summary>
    /// Returns a new balance with each bucket divided by a given scalar
    /// </summary>
    public static BalanceByBucket operator /(BalanceByBucket x, decimal scalar)
    {
        if (scalar == 0)
            throw new DivideByZeroException();

        return Apply(a => a / scalar, x);
    }

    /// <summary>
    /// Compares pairwise by bucket to determine if any buck is > than the other
    /// </summary>
    public static bool operator >(BalanceByBucket x, BalanceByBucket y)
    {
        return Compare(x, y, (a, b) => a > b);
    }

    /// <summary>
    /// Compares pairwise by bucket to determine if any buck is < than the other
    /// </summary>
    public static bool operator <(BalanceByBucket x, BalanceByBucket y)
    {
        return Compare(x, y, (a, b) => a < b);
    }

    /// <summary>
    /// Compares pairwise by bucket using provided function. If any bucket meets the requirement, returns true.
    /// </summary>
    private static bool Compare(BalanceByBucket x, BalanceByBucket y, Func<decimal, decimal, bool> comparerer)
    {
        foreach (var bucket in AllBucketTypes)
        {
            if (comparerer(x[bucket], y[bucket]))
                return true;
        }
        return false;
    }

    public static bool operator ==(BalanceByBucket x, BalanceByBucket y)
    {
        return x.Equals(y);
    }

    public static bool operator !=(BalanceByBucket x, BalanceByBucket y)
    {
        return !x.Equals(y);
    }


    public bool TrueForAnyBucket(Func<decimal, bool> checkFunc)
    {
        if (checkFunc == null)
            throw new ArgumentNullException();

        foreach (var bucket in AllBucketTypes)
        {
            if (checkFunc(this[bucket]))
                return true;
        }
        return false;
    }

    public override bool Equals(object obj)
    {
        if (obj == null) return false;
        if (obj.GetType() != this.GetType()) return false;

        return this.Equals((BalanceByBucket)obj);
    }

    public override int GetHashCode()
    {
        unchecked
        {
            int hash = 17;
            hash = (hash * 23) ^ Total.GetHashCode();
            foreach (BucketType bt in Enum.GetValues(typeof(BucketType)))
            {
                hash = (hash * 23) ^ this[bt].GetHashCode();
            }
            return hash;
        }
    }


    public bool Equals(BalanceByBucket other)
    {
        foreach (var bucket in AllBucketTypes)
        {
            if (this[bucket] != other[bucket])
                return false;
        }
        return true;
    }

    public override string ToString()
    {
        var sb = new StringBuilder();
        sb.Append("Total Balance: ").Append(Total);
        foreach (BucketType bt in Enum.GetValues(typeof(BucketType)))
        {
            sb.Append(bt.GetDescription()).Append(": ").Append(this[bt].ToString("C"));
        }
        return sb.ToString();
    }
}

FundBalance is below. You'll see we had to "re-implement" many methods that basically do nothing but point back to the BalanceByBucket class.

public struct FundBalance : IEquatable<FundBalance>
{
    public readonly int FundId;
    public readonly BalanceByBucket Buckets;

    public FundBalance(int fundid, BalanceByBucket bucketBalances)
    {
        FundId = fundid;
        Buckets = bucketBalances;
    }

    public FundBalance(int fundid, Dictionary<BucketType, decimal> bucketBalances)
        : this(fundid, new BalanceByBucket(bucketBalances))
    {
    }


    /// <summary>
    /// Returns new fund balance item where bucket amounts represent the relative percentages of balance by bucket of the original balance datum.
    /// </summary>
    /// <returns></returns>
    public FundBalance BucketProportions()
    {
        return new FundBalance(FundId, Buckets.GetBucketProportions());
    }

    /// <summary>
    /// Creates the zero dollar fund balance for a given fund id.
    /// </summary>
    /// <param name="fundid"></param>
    /// <returns></returns>
    public static FundBalance Zero(int fundid)
    {
        return new FundBalance(fundid, BalanceByBucket.Zero);
    }

    /// <summary>
    /// Returns a new fund balance item with the result of a function applied to each bucket.
    /// </summary>
    /// <param name="operation"></param>
    /// <returns></returns>
    /// <exception cref="ArgumentNullException"/>
    public FundBalance Apply(Func<decimal, decimal> operation)
    {
        if (operation == null)
            throw new ArgumentNullException();

        return new FundBalance(FundId, Buckets.Apply(operation));
    }

    /// <summary>
    /// Returns a new fund balance that is the maximum of each bucket
    /// </summary>
    /// <param name="fundBal"></param>
    /// <param name="bal"></param>
    /// <returns></returns>
    public static FundBalance GreaterOf(FundBalance fundBal, BalanceByBucket bal)
    {
        return new FundBalance(fundBal.FundId, fundBal.Buckets.GreaterOfByBucket(bal));
    }


    /// <summary>
    /// Returns a new fund balance item with an operation applied pairwise to each bucket
    /// </summary>
    /// <param name="operation">The operation to apply to a given bucket and aggregate balance bucket.  The first argument should be the fund bucket, the second should be the aggregate bucket</param>
    /// <param name="aggBalance"></param>
    /// <returns></returns>
    /// <exception cref="ArgumentNullException"/>
    /// <example>balanceItem.Apply((x,y) => x > y ? x : y,agg) would return a new fund balance with the greater of each bucket</example>
    public FundBalance Apply(Func<decimal, decimal, decimal> operation, BalanceByBucket aggBalance)
    {
        if (operation == null)
            throw new ArgumentNullException();

        return new FundBalance(FundId, Buckets.Apply(operation, aggBalance));
    }


    /// <summary>
    /// Returns a new fund balance item with an operation applied pairwise to each bucket
    /// </summary>
    /// <param name="operation">The operation to apply to a given bucket and another balance's bucket.  The first argument should be the fund bucket of the current instance, the second should be the other bucket</param>
    /// <param name="aggBalance"></param>
    /// <returns></returns>
    /// <exception cref="ArgumentNullException"/>
    /// <exception cref="InvalidOperationException"/>
    /// <example>balanceItem.Apply((x,y) => x > y ? x : y,agg) would return a new fund balance with the greater of each bucket</example>
    public FundBalance Apply(Func<decimal, decimal, decimal> operation, FundBalance aggBalance)
    {
        if (operation == null)
            throw new ArgumentNullException();

        if (FundId != aggBalance.FundId)
            throw new InvalidOperationException();

        return this.Apply(operation, aggBalance.Buckets);
    }

    /// <summary>
    /// Returns a new fund balance where each bucket is the minimum per bucket of the two inputs.
    /// </summary>
    /// <param name="x"></param>
    /// <param name="y"></param>
    /// <returns></returns>
    public static FundBalance MinByBucket(FundBalance x, FundBalance y)
    {
        return x.Apply((a, b) => a > b ? b : a, y.Buckets);
    }


    /// <summary>
    /// Pairwise subtraction by bucket between two fund balances.
    /// </summary>
    /// <param name="x"></param>
    /// <param name="y"></param>
    /// <returns></returns>
    public static FundBalance operator -(FundBalance x, FundBalance y)
    {
        if (x.FundId != y.FundId)
            throw new InvalidOperationException("Cannot subtract balances from different funds");

        return new FundBalance(x.FundId, x.Buckets - y.Buckets);
    }

    /// <summary>
    /// Pairwise addition by bucket between two fund balances.
    /// </summary>
    /// <param name="x"></param>
    /// <param name="y"></param>
    /// <returns></returns>
    public static FundBalance operator +(FundBalance x, FundBalance y)
    {
        if (x.FundId != y.FundId)
            throw new InvalidOperationException("Cannot add balances from different funds");

        return new FundBalance(x.FundId, x.Buckets + y.Buckets);
    }


    /// <summary>
    /// Returns a new fund balances created by multiplying each bucket by a constant.
    /// </summary>
    /// <param name="scalar"></param>
    /// <param name="balance"></param>
    /// <returns></returns>
    public static FundBalance operator *(decimal scalar, FundBalance balance)
    {
        return new FundBalance(balance.FundId, balance.Buckets * scalar);
    }

    /// <summary>
    /// Returns a new fund balances created by multiplying each bucket by a constant.
    /// </summary>
    /// <param name="scalar"></param>
    /// <param name="balance"></param>
    /// <returns></returns>
    public static FundBalance operator *(FundBalance balance, decimal scalar)
    {
        return scalar * balance;
    }


    /// <summary>
    /// Returns a new fund balance created by dividing each bucket by a constant
    /// </summary>
    /// <param name="scalar"></param>
    /// <param name="balance"></param>
    /// <returns></returns>
    /// <exception cref="DivideByZeroException"/>
    public static FundBalance operator /(FundBalance balance, decimal scalar)
    {
        if (scalar == 0)
            throw new DivideByZeroException("Cannot divide a fund balance by zero");

        return new FundBalance(balance.FundId, balance.Buckets / scalar);
    }

    /// <summary>
    /// Returns a new fund balance by pairwise dividing each bucket of a given fund balance by the bucket of an aggregate balance.
    /// </summary>
    /// <param name="balance"></param>
    /// <param name="divisor"></param>
    /// <returns></returns>
    /// <exception cref="DivideByZeroException"/>
    public static FundBalance operator /(FundBalance balance, BalanceByBucket divisor)
    {
        if (divisor.TrueForAnyBucket(x => x == 0))
            throw new DivideByZeroException("Aggregate balance must have all non-zero buckets in order to divide");

        return new FundBalance(balance.FundId, balance.Buckets / divisor);
    }

    /// <summary>
    /// Returns a new fund balance by pairwise multiplying each bucket of a given fund balance by the bucket of an aggregate balance.
    /// </summary>
    /// <param name="balance"></param>
    /// <param name="total"></param>
    /// <returns></returns>
    public static FundBalance operator *(FundBalance balance, BalanceByBucket total)
    {
        return new FundBalance(balance.FundId, balance.Buckets * total);
    }

    /// <summary>
    /// Returns a new fund balance by pairwise multiplying each bucket of a given fund balance by the bucket of an aggregate balance.
    /// </summary>
    /// <param name="balance"></param>
    /// <param name="total"></param>
    /// <returns></returns>
    public static FundBalance operator *(BalanceByBucket total, FundBalance balance)
    {
        return balance * total;
    }

    public static bool operator ==(FundBalance x, FundBalance y)
    {
        return x.Equals(y);
    }

    public static bool operator !=(FundBalance x, FundBalance y)
    {
        return !x.Equals(y);
    }

    public override bool Equals(object obj)
    {
        if (obj == null) return false;
        if (obj.GetType() != this.GetType()) return false;

        return this.Equals((FundBalance)obj);

    }

    public override int GetHashCode()
    {
        unchecked
        {
            int hash = 17;
            hash = (hash * 23) ^ FundId;
            hash = (hash * 23) ^ Buckets.GetHashCode();
            return hash;
        }
    }


    public bool Equals(FundBalance other)
    {
        return FundId == other.FundId &&
               Buckets == other.Buckets;
    }

    public override string ToString()
    {
        return String.Format("Fundid:{0}/{1}", FundId, Buckets);
    }
}

If you'd like to run this code, I believe that the ImmutableDictionary is the only type that requires a non-standard library. You should be able to swap it with a normal Dictionary.

Below is a simplified example that can be run in LinqPad that shows the issue.

void Main()
{
    var immutable = new MyImmutable(new Dictionary<ImmutableKey, decimal>{
        { ImmutableKey.Key1, 1 },
        { ImmutableKey.Key2, -5 },
        { ImmutableKey.Key3, 1.25m },
    });

    var immutable2 = new MyImmutable(new Dictionary<ImmutableKey, decimal>{
        { ImmutableKey.Key1, 1 },
        { ImmutableKey.Key2, 2 },
        { ImmutableKey.Key3, 3 },
    });

    var added = immutable.Apply((a, b) => a + b, immutable2);
    added[ImmutableKey.Key1].Dump();
    added[ImmutableKey.Key2].Dump();
    added[ImmutableKey.Key3].Dump();

    var subImmutable1 = new SubImmutable(1, new Dictionary<ImmutableKey, decimal>{
        { ImmutableKey.Key1, 1 },
        { ImmutableKey.Key2, -5 },
        { ImmutableKey.Key3, 1.25m },
    });
    var subImmutable2 = new SubImmutable(1, new Dictionary<ImmutableKey, decimal>{
        { ImmutableKey.Key1, 1 },
        { ImmutableKey.Key2, 2 },
        { ImmutableKey.Key3, 3 },
    });

    var subImmutableAdded = subImmutable1.Apply((a, b) => a + b, subImmutable2);
    subImmutableAdded.GetType().Name.Dump(); //prints MyImmutable, it's not a SubImmutable
    //after adding two SubImmutables, the type is changed back to the base type

    var asSub = (SubImmutable)subImmutableAdded; // Unable to cast object of type 'MyImmutable' to type 'SubImmutable', SomeOtherValue was lost.
}

public enum ImmutableKey 
{
    Key1,
    Key2,
    Key3
}

public class MyImmutable
{
    protected static readonly IEnumerable<ImmutableKey> AllKeys = Enum.GetValues(typeof(ImmutableKey)).Cast<ImmutableKey>();

    private Dictionary<ImmutableKey, decimal> _dict { get; set; }

    public MyImmutable(Dictionary<ImmutableKey,decimal> d)
    {
        _dict = d;
    }

    public decimal this[ImmutableKey key]
    {
        get
        {
        if (_dict == null || !_dict.ContainsKey(key))
            return 0;

        return _dict[key];
        }
    }

    public MyImmutable Apply(Func<decimal, decimal, decimal> aggFunc, MyImmutable y)
    {
        var aggregated = new Dictionary<ImmutableKey, decimal>(AllKeys.Count());
        foreach (ImmutableKey bt in AllKeys)
        {
            aggregated[bt] = aggFunc(this[bt], y[bt]);
        }
        return new MyImmutable(aggregated);
    }
}

public class SubImmutable : MyImmutable
{
    public int SomeOtherValue { get; set; }
    public SubImmutable(int someValue, Dictionary<ImmutableKey,decimal> d)
        :base(d)
    {
        SomeOtherValue= someValue;
    }
}

Companion SO question: https://stackoverflow.com/questions/28326884/c-sharp-immutable-class-sub-class

\$\endgroup\$
2
  • 1
    \$\begingroup\$ Thank you for sharing the link to the companion question on SO. I've been periodically messing around with this code driving myself crazy trying to find a solution... \$\endgroup\$
    – RubberDuck
    Feb 5 '15 at 1:32
  • 1
    \$\begingroup\$ Thanks for helping me improve the question, which ultimately helped me find the answer! \$\endgroup\$
    – DLeh
    Feb 5 '15 at 1:44
2
\$\begingroup\$

Firstly I would like to describe some generic recommendations (and performance improvements, as I assume it is quite a critical data structure for you):

  1. Since you are building an immutable type, it would be better to mark fields as readonly.

    private readonly ImmutableDictionary<BucketType, decimal> _valueDictionary;
    private readonly decimal _total;
    
  2. _AllBucketTypes will enumerate the enum every time you start iterating on, it, so it is better to capture the result in array.

    private static readonly BucketType[] _AllBucketTypes = Enum.GetValues(typeof(BucketType)).Cast<BucketType>().ToArray();
    
  3. There is no need to create a copy of dictionary before calling ToImmutableDictionary() in BalanceByBucket(Dictionary<BucketType, decimal>), and you can use IDictionary<TKey, TValue> as a parameter type. Also you can optimize the performance of the BalanceByBucket by adding another constructor which accepts ImmutableDictionary<BucketType, decimal> (optimizations shown below).

    public BalanceByBucket(IDictionary<BucketType, decimal> values)
        : this(values.ToImmutableDictionary())
    {
    }
    
    private BalanceByBucket(ImmutableDictionary<BucketType, decimal> values)
    {
        _valueDictionary = values;
        _total = values.Sum(x => x.Value);
    }
    
  4. Select() call is redundant in the BalanceByBucket(IEnumerable<KeyValuePair<BucketType, decimal>>), and you can directly convert to ImmutableDictionary to save on memory allocations and transformations

    private BalanceByBucket(IEnumerable<KeyValuePair<BucketType, decimal>> bucketsWithValue)
        : this(bucketsWithValue
            .GroupBy(x => x.Key)
            .ToImmutableDictionary(x => x.Key, x => x.Sum(y => y.Value)))
    {
    }
    
  5. Apply(Func<decimal, decimal>) and other Apply methods can use new constructor to avoid redundant memory allocations.

    public BalanceByBucket Apply(Func<decimal, decimal> aggFunc)
    {
        if (aggFunc == null)
            throw new ArgumentNullException("aggFunc");
    
        if (_valueDictionary == null)
            return Zero.Apply(aggFunc);
    
        return new BalanceByBucket(_valueDictionary.ToImmutableDictionary(entry => entry.Key, entry => aggFunc(entry.Value)));
    }
    
  6. CreateFrom(IEnumerable<FundBalance>) can be slightly simplified

    public static BalanceByBucket CreateFrom(IEnumerable<FundBalance> balances)
    {
        return new BalanceByBucket(
            balances.SelectMany(x => x.Buckets._valueDictionary ?? Enumerable.Empty<KeyValuePair<BucketType, decimal>>()));
    }
    
  7. You have a bug in operator /(BalanceByBucket, BalanceByBucket) implementation, you're throwing DivideByZeroException based on x instead of y values. Also, you don't actually need to throw this exception explicitly, it will be thrown anyway by Apply((a, b) => a / b, x, y) expression.

Now back to your original question - "FundBalance can do everything a BalanceByBucket can do without having to reimplement every method":

  1. Operator overloads cannot be reused, you would have to reimplement them anyway.
  2. Both structures have references to each other which is not great.
  3. FundBalance appears to add a very little value on top of BalanceByBucket.

As a result, I would try to have a single structure/class that provides required functionality. If you want to be able to reference the BalanceByBucket functionality without explicit reference to FundBalance, consider switching from struct to class and define an interface for BalanceByBucket functionality.

\$\endgroup\$
2
  • \$\begingroup\$ Thank you very much for your feedback. on #6, any reason you chose Enumerable.Empty<KeyValuePair> instead of ImmutableDictionary<BucketType, decimal>.Empty? \$\endgroup\$
    – DLeh
    Feb 5 '15 at 13:31
  • \$\begingroup\$ ImmutableDictionary<BucketType, decimal>.Empty will not add any value here because SelectMany method expects Func<TSource, IEnumerable<TResult>> as parameter, so any empty IEnumerable<KeyValuePair<BucketType, decimal>> will suffice \$\endgroup\$
    – almaz
    Feb 6 '15 at 14:09
1
\$\begingroup\$

The constructor public BalanceByBucket(Dictionary<BucketType, decimal> values) should be polished by using the overloaded constructor of Dictionary<TKey,TValue> which takes a Dictionary as parameter.

public BalanceByBucket(Dictionary<BucketType, decimal> values)
{
    var dict = new Dictionary<BucketType, decimal>(values);

    _valueDictionary = dict.ToImmutableDictionary();

    _total = _valueDictionary.Sum(x => x.Value);
}  

You should use braces {} for single line if statements and loops to make your code less error prone.


You are using the StringBuilder object in a very good way by making use of the fact that the Append() method returns the StringBuilder instance.


In the ToString() implementation of the FundBalance class you are returning String.Format("Fundid:{0}/{1}", FundId, Buckets). I would mention the Buckets also.


I love how you have commented your code because the comment I have found is describing why you have done something.

//have to copy local to use in lambda - DJL 12/2/2014  
\$\endgroup\$
2
  • \$\begingroup\$ _valueDictionary can be null as BalanceByBucket is a struct. \$\endgroup\$
    – almaz
    Feb 5 '15 at 10:54
  • 1
    \$\begingroup\$ @almaz thanks for that. I am used to mostly work with classes ;-) Updated answer. \$\endgroup\$
    – Heslacher
    Feb 5 '15 at 11:32

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.