ChangeCalculator for calculating money denominations in change

Question

I'm doing some exercises in OOP and I just created a program that calculates amounts of denominations in a change at the shop. So, let's say something costs 10, I give 25.30 (for some reason), so I should get as a change: one 10, one 5, one 0.20, one 0.10.

When I started to create the program, I thought it would be nice if client of the ChangeCalculator had an option to specify his way of calculating the amounts - for example to use his own currency. That's why I created interfaces.

My program consists of:

• IChangeCalculator - represents change calculator
• ChangeCalculatorPLN - implementation that returns MoneyAmountPLN (money denominations amounts in PLN currency)
• IMoneyAmount - represents data about amounts of denominations
• MoneyAmountPLN - represents money denominations amounts in PLN
• IMoneyToCoinsConverter - represents converter that converts given money to something that implements IMoneyAmount
• MoneyToCoinsConverterPLN - converts given amount of money to MoneyAmountPLN

Here is the source code:

public interface IChangeCalculator<T> where T : IMoneyAmount
{
    T Calculate(decimal price, decimal payedAmount);
}

public class ChangeCalculatorPLN : IChangeCalculator<MoneyAmountPLN>
{
    IMoneyToCoinsConverter<MoneyAmountPLN> _moneyToCoinsConverter;

    public ChangeCalculatorPLN( IMoneyToCoinsConverter<MoneyAmountPLN> moneyToCoinsConverter )
    {
        _moneyToCoinsConverter = moneyToCoinsConverter;
    }

    public MoneyAmountPLN Calculate(decimal price, decimal payedAmount)
    {
        var change = payedAmount - price;
        if (change < 0)
            throw new Exception("The given amount of money is not enough!");
        if (change == 0)
            return MoneyAmountPLN.Empty;

        var amount = GetAmount(change);
        return amount;
    }

    private MoneyAmountPLN GetAmount(decimal change)
    {
        return _moneyToCoinsConverter.Convert(change);
    }
}

public interface IMoneyAmount
{
    IEnumerable<int> GetAmounts( );
}

//MoneyAmountPLN is quite long, because it contains fields for each denomination: 500, 200, 100, 50, 20, 10, 1, 0.5, 0.2, 0.1, 0.05, 0.02, 0.01. I'll not put it here.

public interface IMoneyToCoinsConverter<T> where T : IMoneyAmount
{
    T Convert(decimal money);
}

public class MoneyToCoinsConverterPLN : IMoneyToCoinsConverter<MoneyAmountPLN>
{
    public MoneyAmountPLN Convert(decimal money)
    {
        //logic doesn't really matter here. It returns MoneyAmountPLN with corrects amounts of denominations.
    }

}

The program works, I did some unit tests. My real issue is that I'm not sure if this implementation is a good one. Did I do some mistakes? Could some parts be done better? Personally I think that maybe the structure is a little too complicated? If I saw a program like this for the first time, I would probably feel lost, but I'm not a very good programmer, so I can't say if my feeling is right. Adding new currency requires creation of new classes that implement: IChangeCalculator, IMoneyAmount, IMoneyToCoinsConverter. I think that this is too much. I think that ChangeCalculator shouldn't need to be reimplemented for each new currency, but I didn't know how to do that, since each currency requires different IMoneyAmount implementation. Calculate method of ChangeCalculator needs to know what type of IMoneyAmount it returns.

Please provide constructive feedback, it would be great to hear any of your advices.

Answer 1

The main problem with your code is that you've been encoding currency denomination data in C#'s type system: one type for each currency. That somehow led to a complicated interface/generics design, all without providing any real benefits. In fact, this design has several drawbacks:

• Each new currency requires additional code (3 extra classes in total).
• Apparently the core money-to-coins algorithm needs to be duplicated for each new currency - a proper design should make this reusable across different currencies.
• Those generic interfaces aren't very useful: IChangeCalculator<MoneyAmountPLN> is not interchangeable with IChangeCalculator<MoneyAmountEuro>, despite being the 'same' interface.
• The IChangeCalculator layer doesn't seem to provide much value: this could easily be done at the call-site.
• Wrapping the results in an IMoneyAmount object on which the caller has to call a GetAmounts method seems a little pointless - why not just return those amounts directly?

---

This can be simplified a lot with a data-driven approach. The core money-to-coins method needs an amount and knowledge about a currency's denominations, and returns a list of coin (or bill) amounts:

public (int amount, decimal denomination)[] SplitIntoDenominations(
    decimal amount,
    decimal[] denominations)

Such a method can easily be reused for different currencies:

var euroDenominations = new decimal[] { 500m, 200m, 100m, ..., 0.05m, 0.02m, 0.01m };
// Denomination arrays can be stored anywhere: in hard-coded arrays,
// in a currency-name/denominations dictionary, or loaded from a file, etc.

var returned = SplitIntoDenominations(amount, euroDenominations);
foreach ((var amount, var denomination) in returned)
    Console.WriteLine($"{amount} coins of €{denomination}.");

You may find it useful to create a CurrencyInfo class that contains a denominations array as well as the name and symbol of the currency, and perhaps additional information, depending on your needs:

var euro = new CurrencyInfo("Euro", "€", new decimal[] { 500m, ..., 0.01m });
var dollar = new CurrencyInfo("Dollar", "$", new decimal[] { 100m, ..., 0.01m });

The same can be done for the return value of this method, if you find an array of named tuples to be insufficient. Again, this depends on your needs, but keep in mind that you only need a single class, regardless of currency.