3
\$\begingroup\$

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?

\$\endgroup\$
  • \$\begingroup\$ Are the denomination keys inside CalculateDenominations guaranteed to be unique? If so, you should change that method to return a Dictionary<decimal, int> instead. That also implies you can't use an iterator block. Personally I would just make that method a property as well, since it is pure and the rest of your class is immutable (assuming the ICurrency doesn't mutate). \$\endgroup\$ – Brad M Feb 13 '18 at 19:43
  • \$\begingroup\$ @Brad M, yes they are guaranteed to be unique. Just to confirm you are saying: 1) make the return type of CalculateDenominations a dictionary and 2) Make CalculateDenominations a method rather than a property? \$\endgroup\$ – w0051977 Feb 13 '18 at 19:51
  • 2
    \$\begingroup\$ I do not agree with making the method a property. It might be pure but its result changes based on the two other properties. It should remain a method. I also find it's ok to not use a dictionary here but an IEnumerable. As I've also said in one of my previous comments: instead of returning key-value-pairs you should return tuples with strong property names. \$\endgroup\$ – t3chb0t Feb 13 '18 at 20:03
  • \$\begingroup\$ @t3chb0t Fair enough regarding the method/property, but I have to disagree about the Dictionary. By returning IEnumerable, the caller of this method no longer has a runtime guarantee that the keys/denominations are unique. \$\endgroup\$ – Brad M Feb 13 '18 at 20:41
  • \$\begingroup\$ @t3chb0t, I like the idea of a Tuple (I realise you suggested this last time as well). However, I was thinking about taking it one step further and creating an object. What are your thoughts on that? \$\endgroup\$ – w0051977 Feb 14 '18 at 8:10
2
\$\begingroup\$

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");
\$\endgroup\$
  • \$\begingroup\$ If I don't put the validation in the constructor of the value object, then were would I put it in the value object option? The validation is contained in the constructor here: blog.ploeh.dk/2015/01/19/… \$\endgroup\$ – w0051977 Feb 13 '18 at 21:45
  • \$\begingroup\$ You're right, in an immutable value type such as this, its not a bad place to put it, the alternative would be to make the constructor private and make a static method that creates the class for you after validating the inputs. Constructors throwing exceptions can play havoc with some UI and dependency packages, you end up getting TargetInvocationExceptions instead of your validation messages coming back. There is a healthy discussion about this stackoverflow.com/q/77639/1690217 but your immutable value object is a good special case. \$\endgroup\$ – Chris Schaller Feb 13 '18 at 22:23
  • \$\begingroup\$ Thanks. If this was an entity (rather than a value object) then were would you put the validation. Perhaps in a factory? Not all my entities have factories. \$\endgroup\$ – w0051977 Feb 13 '18 at 22:25
  • \$\begingroup\$ Good way to phrase it :) then I would make a method called Validate() so it could be explicitly evaluated (and potentially overridden) and make the first line in CalculateDenominations call Validate() \$\endgroup\$ – Chris Schaller Feb 13 '18 at 22:28
  • 1
    \$\begingroup\$ Thanks. Can you confirm that is for entities only i.e. not for value objects. If so then what makes value objects different. Thanks. \$\endgroup\$ – w0051977 Feb 13 '18 at 22:30

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.