Calculate change denominations 3

Question

I am trying to refactor a large application to use more DDD principles as and when I get chance. I am concentrating on the very simple areas first. The app currently has a very simple Domain Service (one of many) as it stands (see option 1). Please see the code below:

Option 1 - Live

public IEnumerable<System.Collections.Generic.KeyValuePair<decimal, int>> CalculateDenominations(decimal cost, ICurrency currency)
        {
            var target = cost;
            foreach (var denomination in currency.AvailableDenominations)
            {
                var numberRequired = target / denomination;
                if (numberRequired >= 1)
                {
                    int quantity = (int)Math.Floor(numberRequired);
                    yield return new KeyValuePair<decimal, int>(denomination, quantity);
                    target = target - (quantity * denomination);
                }
            }
        }

Option 2

public sealed class DenominationsRequired
    {
        private readonly decimal _cost;
        private readonly ICurrency _currency;

        public DenominationsRequired(decimal cost, ICurrency currency)
        {
            if (currency == null)
                throw new ArgumentNullException("Currency cannot be null", "ICurrency");
            if (cost < 0)
                throw new ArgumentException("Cost cannot be less than zero", "Cost");
            if (decimal.Round(cost, 2) != cost)
                throw new ArgumentException(string.Concat("Cost has too many decimal places.  It should only have: ", currency.DecimalPlaces), "Cost");
            _cost = cost;
            _currency = currency;
        }

        public decimal Cost
        {
            get { return _cost; }
        }

        public ICurrency Currency
        {
            get { return _currency; }
        }

        public IEnumerable<System.Collections.Generic.KeyValuePair<decimal, int>> CalculateDenominations()
        {
            var target = _cost;
            foreach (var denomination in _currency.AvailableDenominations)
            {
                var numberRequired = target / denomination;
                if (numberRequired >= 1)
                {
                    int quantity = (int)Math.Floor(numberRequired);
                    yield return new KeyValuePair<decimal, int>(denomination, quantity);
                    target = target - (quantity * denomination);
                }
            }
        }
    }

I am thinking about refactoring the Domain Service (option 1 - live) into a Value Object (option 2) so that I can add validation. I would be grateful for critical comments about this approach. Is a ValueObject (option 2) "better" than a domain service (option 1) for this? I believe it is acceptable to put caclculations in Value Objects as long as they do not mutate state.

I am specifically interested to know:

1. Is a Value Object "better" than a Domain Service for this specific requirement?

2. Is it wise to have a property called: Currency.DecimalPlaces (indicates how many decimal places the currency allows e.g. 2 for pounds and 2 for dollars - some currencies allow 3 or 4 decimal places), which is used in the DenominationsRequired constructor for validation?

3. Should I keep option 1 or go with option 2? If option 1, then how do I approach validation?

Answer 1

1) is a Value Object "better" than a Domain Service?

This is certainly subjective, but i'll offer this pragmatic view point. Evaluate how often this code will be actively referenced and debugged by other developers. If this code is located in an easily reusable utility assembly, and it is a realistic expectation that it will be reused, then Value Object wins for me every time. It offers more self documentation than your initial attempt which is important because you have not implemented code comments at all. At the end of the day you have to justify the time and effort required to get the code into production, code that is reused a lot, especially by other developers is worth the effort to get it right, but you have increased you code surface and will have to increase you unit test coverage to match. Value objects like this i find are much easier to unit test, and like you have said with validation, the object now has additional options for expressing it's state without blocking execution with exceptions and we can more easily inherit this structure and when we need to override specific functionality, such as validation when we as developers do reuse your code.

2) Is it wise to have a property called Currency.DecimalPlaces

Absolutely, especially with currency where rounding errors can have a real world monetary impact on the user, this seems like a useful thing to know, and it is natural to expect that Currency would already have this attribute.

3) Should I go with option 2?

See my response to question 1 😀 from a purist point of view your Value Object is a complete package, it has a dependency on Currency, but is easier to lock down with unit tests and to distribute in a utility assembly for reuse. It's up to you to evaluate the cost of developing this solution and refactoring existing code to use it.

Certainly follow the advice from @t3chb0t, you've gone to this much effort, implement your own contract definition for the response from CalculateDenominations, a key value pair response is too ambiguous for users know how to use the response, it's the missing bit of "self" documentation.

How do i approach validation with option 1?

Throwing exceptions from the calculation routine is an acceptable method for validation, if this was a Mutable type (not readonly) then I would argue that it would be better than throwing exceptions from the constructor which can lead to other interesting and less than satisfactory implementation patterns.

UPDATE: Moving validation to an explicit method called Validate() further helps to self document your class, and makes it easier to maintain long term. You still call validation from either your calculation method or the constructor. As a bonus when validation fails, the stack trace will now show DenominationsRequired.Validate() which further describes why the exception was raised for future debuggers.

For mutable types it's only at the point of execution that the validation matters, the code that interacts with the instances of this class should be free to instantiate the object and react to its state at a later point in the execution. It is hard for example to check the state of a Value object if we failed to create the object in the first place.

Here your Value type is an Immutable Type, that is to say that it's value over time will not change, and cannot because you have marked the fields as readonly. This is a special case where it is more appropriate to validate in the constructor rather than in the calculation because there will not be any opportunity to change the value if it is not valid.

A quick note about your validation, you hardcoded 2 as the comparison, rather than Currency.DecimalPlaces, so it should be:

if (decimal.Round(cost, currency.DecimalPlaces) != cost)
        throw new ArgumentException(string.Concat("Cost has too many decimal places.  It should only have: ", currency.DecimalPlaces), "Cost");