Value Objects member used to validate another value object

Question

Please see the code below:

    public sealed class UKCurrency : ICurrency
    {
        private static readonly int _decimalPlaces=2; 
            private static readonly decimal[] _denominations = new decimal[] {
                50.00M, 20.00M, 10.00M,
                5.00M,  2.00M,  1.00M,
                0.50M,  0.20M,  0.10M,
                0.05M,  0.02M,  0.01M,
            };

            public IEnumerable<decimal> Denominations
            {
                get { foreach (var denomination in _denominations) yield return denomination; }
            }
    }

    public sealed class DenominationCounter
    {
        private readonly decimal _cost;

        public decimal Cost
        {
            get { return _cost; }
        }

        public ICurrency Currency
        {
            get { return _currency; }
        }

            public DenominationCounter(decimal cost, ICurrency currency)
            {
                if (currency == null)
                    throw new ArgumentNullException("Currency cannot be null", "ICurrency");
                if (cost < 0)
                    throw new ArgumentException("Cost cannot be negative", "Cost");
                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");
                _cost = cost;
                _currency = 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);
                }
            }
        }
    }

The DenominationCounter constructor throws an exception if the cost has the wrong number of decimal places.

Notice that the UKCurrency class is used to validate the DenominationCounter as shown below:

if (decimal.Round(cost, currency.DecimalPlaces) != cost)

Is this a normal to approach validation like this:

1) A Value Objects member is used to validate an entity

2) A Value Objects member is used to validate another value object

I am asking this because I have never seen validation approached like this before and I am trying to follow the principle of least astonishment these days.

Answer 1

I don't see any need for your DenominationCounter object. It's purpose is to provide an IEnumerable<(decimal, int)> (I've simplified your data type to use a tuple) via CalculateDenominations.

But that method is completely deterministic in nature. You are achieving nothing useful (beyond arguments around primitive obsession) by having instances.

Further, you are creating a non-cohesive solution to denominations by having multiple classes responsible for handling what should be a single responsibility. Don't just use subtype polymorphism just for the sake of it.

I'd make the whole lot a single static class, to maximise cohesion, to remove all the complications around constructors, to remove the need to duplicate functionality in every implementation of ICurrency.Denomination etc:

public enum Currency { GBP, EUR, USD, ... }

public static class Denominations
{
    private static readonly decimal[] GBPDenominations = new decimal[]
    {
        50.00M, 20.00M, 10.00M,
        5.00M,  2.00M,  1.00M,
        0.50M,  0.20M,  0.10M,
        0.05M,  0.02M,  0.01M,
    };

    private static readonly decimal[] EURDenominations = ...

    private static Dictionary<Currency, decimal[]> CurrencyDenominations =
        new Dictionary<Currency, decimal[]>
        {
            [Currency.GBP] = GBPDenominations,
            [Currency.EUR] = EURDenominations,
            ...
        };


    public static IEnumerable<(decimal denomination, int quantity)>
        CalculateDenominations(Currency currency, decimal cost)
    {
        var remainingCost = cost;
        foreach (var denomination in CurrencyDenominations[currency])
        {
            var numberRequired = remainingCost / denomination;
            if (numberRequired >= 1)
            {
                var quantity = (int)Math.Floor(numberRequired);
                yield return (denomination, quantity);
                remainingCost = remainingCost - (quantity * denomination);
            }
        }
    }
}

Answer 2

You don't have two different value objects. You have a value object and a fancy array. You probably need to combine a lot of UKCurrency and DenominationCounter. It's not wrong for a value object to validate that its dependencies are valid even if they are other value objects, but both objects need to be valuable independent of each other.

A value object needs a value that's actually comparable (even if you never do or never implement a comparison), UKCurrency doesn't have that. Every instance of UKCurrency is exactly the same, and even worse if CanadianCurreny existed it would be functionally identical to UKCurrency.

Try stating what your objects do/ what value they provide in 1-2 sentences. If you can't do that or what you said doesn't seem useful then something needs to change.

Answer 3

OK So! here is my understanding of your approach

1. In your domain you have a special value type eg. Currency In Denominations
2. You want to prevent the construction of a value type that would be impossible by definition, ie half a 10p piece.
3. You are modeling value types as immutable reference types. rather than using value types. Because of reasons.
4. Some of the information required to validate the 'value type' is contained in another class. eg the possible denominations of a particular currency.

Bad things:

1. Throwing in constructors is generally bad. The exception to the rule is impossible value types eg 31st of Feb
2. All of your examples could be refactored to do the validation in a method rather than a constructor.

If you are married to your way of doing things, then you have no alternative to your current solution. Yes it seems slightly odd, but it is functional.

If you want a solution where you don't have that particular combination of validation, why not get rid of the constructor exceptions completely?

public class ImmutableListOfCoins 
{
    private IEnumerable<KeyValuePair<decimal, uint>> listOfCoins {get; private set;}
    public ImmutableListOfCoins(IEnumerable<KeyValuePair<decimal, uint>> listOfCoins) 
    {
         ....
    }
}

public static ImmutableListOfCoins CalculateDenominations(decimal cost)
    {
        var target = _cost;
        if (decimal.Round(cost, currency.DecimalPlaces) != cost)
        {
            throw new ArgumentException("Cost has too many decimal places.");
        }
        var loc = new Dictionary<decimal,uint>();

        foreach (var denomination in _currency.AvailableDenominations)
        {
            var numberRequired = target / denomination;
            if (numberRequired >= 1)
            {
                var quantity = (uint)Math.Floor(numberRequired);
                loc.Add(new KeyValuePair<decimal, uint>(denomination, quantity));
                target = target - (quantity * denomination);
            }
        }
        return new ImmutableListOfCoins(loc);
    }