1
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ Confusing to me. I am not seeing where is actually makes change. \$\endgroup\$ – paparazzo Aug 26 '18 at 17:29
  • \$\begingroup\$ It looks like some important parts of your code are missing here, which makes it difficult to review. However, if I understand your problem correctly, then a single function that accepts an amount and a list of available denominations, and that returns a list of (count, denomination) pairs should be sufficient. Why would you need to create different types for different currencies? \$\endgroup\$ – Pieter Witvoet Aug 27 '18 at 12:06
  • \$\begingroup\$ The part of calculating denomination is in Convert method of MoneyToCoinsConverterPLN. It's just a few lines of code and it's not really interesting. My question is more about interfaces, classes, etc. @PieterWitvoet - I thought that it's a good idea to create a class instead of returning a list of numbers. \$\endgroup\$ – Loreno Aug 27 '18 at 18:30
1
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ Thanks! You are right, over-complication of the way how currency data is stored led to unneeded complexity of the solution. Generally I thought tuples are not a good solution, but in this case it looks good \$\endgroup\$ – Loreno Aug 31 '18 at 14:15

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.