Breakdown of crafting “Crafting Wicked Domain Models”


This post is based upon the video presentation given by Jimmy Bogard on how he crafts Domain Models. The video is available here while the original source is available here. I have also taken the liberty to create my own GitHub repository which will provide various “markers” regarding the various refactorings I made according to the presentation.

I am using this to record what lessons I’ve taken from that presentation and would like to re-iterate that the source code and the refactoring that I will be applying is in no way my work. Now I am doing this because I want to understand some of the more subtle points that he makes by methodically working through his presentation. Moreover, it will be easier and quicker for me to reference certain points in this post as I’m aiming to provide a “summary” of his points.

The problem space

The application that is under consideration concerns a loyalty/reward card programme. This is something that most of us are familiar with whereby you may accumulate points for purchases made. Upon accumulating enough points you can redeem them to get a voucher or a gift. So that’s fairly straightforward at this point.

Presentation walkthrough

He begins the presentation by going through the Anaemic Domain Models of the application.

Member

He starts off with the Member class

He then highlights the fact that “NumberOfActiveOffers” is being used to quickly retrieve and display to the user the persisted number of offers associated with a particular Member, rather than having to calculate it every time the details of a Member is retrieved.

Moreover, he points out that the AssignedOffers only belongs to the given Member and cannot belong to another Member.

Offer

Then he moves onto the Offer class

He points out that the MemberAssigned denotes the fact that for any given Offer we can know with which Member this Offer is assigned to.

OfferType

This is just metadata about an Offer.

ExpirationType

Finally the ExpirationType is simply an enumeration.

So far so good.

However, since these Domain Models are anaemic, i.e. they don’t contain any behaviour, it means that the business logic is not modelled adequately. The latter is actually in OfferAssignmentService

What should be immediately apparent is that the AssignOffer is much too big and doing too many things. The next section details how to mainly refactor that method.

Upon closer inspection the method is doing the following:

Calculate the value of the Offer

var member = _memberRepository.GetById(memberId);
 var offerType = _offerTypeRepository.GetById(offerTypeId);
var value = _offerValueCalculator
 .CalculateValue(member, offerType);

Calculate the expiry date of the Offer

switch (offerType.ExpirationType)
{
    case ExpirationType.Assignment:
        dateExpiring = DateTime.Now.AddDays(offerType.DaysValid);
        break;
    case ExpirationType.Fixed:
        if (offerType.BeginDate != null)
        dateExpiring =
        offerType.BeginDate.Value.AddDays(offerType.DaysValid);
    else
        throw new InvalidOperationException();
        break;
    default:
        throw new ArgumentOutOfRangeException();
}

Assign the offer to the member

var offer = new Offer
{
    MemberAssigned = member,
    Type = offerType,
    Value = value,
    DateExpiring = dateExpiring
};

member.AssignedOffers.Add(offer);

Increment the Number of Active Offer(s)

member.NumberOfActiveOffers++;

Persist the Offer

_offerRepository.Save(offer);

What is not immediately apparent is the importance of

member.AssignedOffers.Add(offer);
member.NumberOfActiveOffers++;

which the developer needs to remember to have in the code so that tracking of offers is done appropriately. This can be easily missed and that’s yet another reason why having “proper” Domain Models is important.

Refactoring

Hence what we need to do is to refactor the code i.e. we need Domain Models which:

  1. Are responsible for their operational behaviours.
  2. Provide proper encapsulation.

Problem 1

What would happen if we were to add code like:

member.AssignedOffers.Add(offer);
member.AssignedOffers.Clear();
member.NumberOfActiveOffers++;

From a Product Owner POV, it’s a cardinal sin because it would be taking offers from a Member! However, the API, as it is, allows for that operation all too easily. In order to fix that, he applies the Encapsulate Collection refactoring.

Solution 1

So he creates a method representative of what we are trying to achieve i.e. assign an Offer to a Member.

member.AssignOffer(offer);

and getting rid of the code which was assigning the Offer straight to the collection. He then performs a lot of refactoring to the Member class which now looks like:

using System.Collections.Generic;

namespace WickedDomainModels.Model
{
    public class Member : Entity
    {
        private readonly List<Offer> _assignedOffers;

        public Member()
        {
            _assignedOffers = new List<Offer>();
        }

        public string FirstName { get; set; }
        public string LastName { get; set; }
        public string Email { get; set; }

        public IEnumerable<Offer> AssignedOffers
        {
            get { return _assignedOffers; }
            // Removed set
        }

        public int NumberOfActiveOffers { get; set; }

        public void AssignOffer(Offer offer)
        {
            _assignedOffers.Add(offer);
        }
    }
}
  • First,  the ICollection has been changed to an IEnumerable. This is due to the fact that using the former you can do various invalid operations e.g.
member.AssignedOffers.CopyTo(...);
  • Secondly, there is no direct access to the collection as the setter for AssignedOffers property has been removed. This prevents external classes from adding to the collection and only allowing  the Member class to add an Offer. Of course, to be in a position to do that, we must have a private field _assignedOffers.

Problem 2

var member = new Member();
member.AssignOffer(new Offer());

Again this from a Product Owner’s point of view, should not be possible. This is effectively saying “Give some ’empty’ offer to someone whose credentials we don’t have on record” (Assuming that one of the aims of the reward card programme is to identify who the people not spending much are).

So what we need to do is to introduce the invariants of the Member and Object classes.

Solution 2

The simple way to do that would be to use constructor injection.

So for the Member class we don’t have the empty constructor anymore while for the Object class, we introduce a constructor. (In a later part of the video presentation, he also does the same thing with OfferType (00:42:39))

Problem 3

We previously mentioned that whenever we assign an offer to a Member, we need to remember to increment the NumOfActiveOffers for that Member.

member.AssignOffer(offer);            
member.NumberOfActiveOffers++;

Solution 3

The correct implementation should hence not be in OfferAssignmentService.AssignOffer but rather internally in Member.AssignOffer.

public void AssignOffer(Offer offer)
{
    _assignedOffers.Add(offer);
    NumberOfActiveOffers++;
}

Problem 4

How about this code:

member.AssignOffer(offer);
member.NumberOfActiveOffers = Int32.MaxValue;

While this will compile, this kind of operation should not be possible from a business logic point of view. This is of course caused by the fact that we have got a public setter on NumberOfActiveOffers.

Solution 4

So the solution is fairly simple and is as follows:

public int NumberOfActiveOffers { get; private set; }

This is just good encapsulation. So the responsibility to keep track of the NumberOfActiveOffers is now rightly given to the Member class.

Problem 5

Although we’ve created a constructor for the Object class in order to provide the invariants, there is still an issue i.e.:

var value = Int32.MaxValue;
var offer = new Offer(member, offerType, dateExpiring, value);

This compiles but from a Domain Model POV, this should not be valid.

If you remember from the code above, the actual value to be passed in is calculated by:

var value = _offerValueCalculator.CalculateValue(member, offerType);

and we have to somehow tether this value to the Offer instance creation.

Moreover, we want to prevent code such as:

var offer = new Offer(new Member(...), offerType, dateExpiring, value);

because with this we could be assigning the Offer to the wrong Member.

Solution 5

The way to achieve this is to make the Member class responsible for creating the Offer as well as having it calculating the value.

public void AssignOffer(Guid memberId, Guid offerTypeId)
{
    var member = _memberRepository.GetById(memberId);
    var offerType = _offerTypeRepository.GetById(offerTypeId);

    DateTime dateExpiring;

    // No longer calculating the offer value in this class

    switch (offerType.ExpirationType)
    {
        case ExpirationType.Assignment:
            dateExpiring = DateTime.Now.AddDays(offerType.DaysValid);
            break;
        case ExpirationType.Fixed:
            if (offerType.BeginDate != null)
                dateExpiring =
                    offerType.BeginDate.Value.AddDays(offerType.DaysValid);
            else
                throw new InvalidOperationException();
            break;
        default:
            throw new ArgumentOutOfRangeException();
    }
    // No longer instantiating the Offer here.   
    var offer = member.AssignOffer(offerType, dateExpiring, _offerValueCalculator);

    _offerRepository.Save(offer);
}

//////////////////////////////////////////////////////////
public Offer AssignOffer(OfferType offerType, DateTime dateExpiring
            , IOfferValueCalculator offerValueCalculator)
{
    var value = offerValueCalculator
        .CalculateValue(this, offerType);

    var offer = new Offer(this, offerType, dateExpiring, value);

    _assignedOffers.Add(offer);
    NumberOfActiveOffers++;

    return offer;
}

Problem 6

In the code, we would really like to get rid of the switch statement because it violates the OCP.

switch (offerType.ExpirationType)
{
    case ExpirationType.Assignment:
        dateExpiring = DateTime.Now.AddDays(offerType.DaysValid);
        break;
    case ExpirationType.Fixed:
        if (offerType.BeginDate != null)
            dateExpiring =
                offerType.BeginDate.Value.AddDays(offerType.DaysValid);
        else
            throw new InvalidOperationException();
        break;
    default:
        throw new ArgumentOutOfRangeException();
}

Solution 6 ()

The way he resolves this issue is by first pushing date expiry calculation into the AssignOrder method.

public Offer AssignOffer(OfferType offerType /*, Removed dateExpiring */ 
            , IOfferValueCalculator offerValueCalculator)
{
    var value = offerValueCalculator
        .CalculateValue(this, offerType);

    DateTime dateExpiring;

    switch (offerType.ExpirationType)
    {
        case ExpirationType.Assignment:
            dateExpiring = DateTime.Now.AddDays(offerType.DaysValid);
            break;
        case ExpirationType.Fixed:
            if (offerType.BeginDate != null)
                dateExpiring =
                    offerType.BeginDate.Value.AddDays(offerType.DaysValid);
            else
                throw new InvalidOperationException();
            break;
        default:
            throw new ArgumentOutOfRangeException();
    }

    var offer = new Offer(this, offerType, dateExpiring, value);

    _assignedOffers.Add(offer);
    NumberOfActiveOffers++;

    return offer;
}

The switch statement issue is still preseent and he then points out that normally he would try and push that behaviour into the ExpirationType ‘class’. However, if you recall, the latter is actually an enumeration. So, he then suggests mimicking Java based enumerations which can behave more like classes and he does so through the use the following class :

The only thing you really need to know about this class is that it is similar to a .NET enum but on steroids.

What he does next is to change the enum ExpirationType to a class deriving from the Enumeration class.

Then, he exposes two properties ‘Assignment’ and ‘Fixed’ which are of type ExpirationType and to get those types he creates a private constructor as he doesn’t want external clients to be able to create an ExpirationType. In this manner, it make the latter seem like an enumeration because you will refer to each “enumeration” as:

ExpirationType.Assignment
ExpirationType.Fixed

Here is the ExpirationType class:

The fact that we have changed from an enumeration to a class means that we can no longer use the switch statement but have to resort to ifs (00:51:22).

public Offer AssignOffer(OfferType offerType
            , IOfferValueCalculator offerValueCalculator)
{
    var value = offerValueCalculator
        .CalculateValue(this, offerType);

    var dateExpiring = new DateTime();

    if (offerType.ExpirationType == ExpirationType.Assignment)
        dateExpiring = DateTime.Now.AddDays(offerType.DaysValid);
    else if (offerType.ExpirationType == ExpirationType.Fixed)
    {
        if (offerType.BeginDate != null)
        {
            dateExpiring = offerType.BeginDate.Value.AddDays(offerType.DaysValid);
        }
        // Notice that we haven't got the else throwing an InvalidOperationException
        // here because we are going to deal with this later.
    }
    else
    {
        throw new InvalidOperationException();
    }

    var offer = new Offer(this, offerType, dateExpiring, value);

    _assignedOffers.Add(offer);
    NumberOfActiveOffers++;

    return offer;
}

At this point, you might be wondering why we did all of this work to produce what is arguably less readable code. Well this work has now allowed us to push the behaviour down into  OfferType. The reason why we are using the latter is due to the fact that in the ifs, we can see that there is a offerType is being called many times (code in blue above) and that’s an indication of the Inappropriate Intimacy code smell.

using System;

namespace WickedDomainModels.Model
{
    public class OfferType
    {
        public OfferType(string name, ExpirationType expirationType, int daysValid, DateTime? beginDate)
        {
            Name = name;
            ExpirationType = expirationType;
            DaysValid = daysValid;
            BeginDate = beginDate;
        }

        public string Name { get; set; }
        public ExpirationType ExpirationType { get; set; }
        public int DaysValid { get; set; }
        public DateTime? BeginDate { get; set; }

        public DateTime CalculateExpiration()
        {
            var dateExpiring = new DateTime();

            if (ExpirationType == ExpirationType.Assignment)
                dateExpiring = DateTime.Now.AddDays(DaysValid);
            else if (ExpirationType == ExpirationType.Fixed)
            {
                if (BeginDate != null)
                {
                    dateExpiring = BeginDate.Value.AddDays(DaysValid);
                }
                // Notice that we haven't got the else throwing an InvalidOperationException
                // here because we are going to deal with this later.
            }
            else
            {
                throw new InvalidOperationException();
            }
            return dateExpiring;
        }
    }
}

This is looking more promising but there are still the conditional strutures. Well how about we push down the behaviour down to the ExpirationType class i.e. doing what in the DDD world is know about double dispatch (This last bit is quoting the author and I don’t know what this is exactly)?

using System;

namespace WickedDomainModels.Model
{
    public class ExpirationType : Enumeration
    {
        private ExpirationType(int value, string displayName)
            : base(value, displayName)
        {
        }
        public static readonly ExpirationType Assignment = new ExpirationType(1, "Assignment");
        public static readonly ExpirationType Fixed = new ExpirationType(1, "Fixed");

        public DateTime CalculateExpiration(OfferType offerType)
        {
            var dateExpiring = new DateTime();

            if (this == Assignment)
                dateExpiring = DateTime.Now.AddDays(offerType.DaysValid);
            else if (this == Fixed)
            {
                if (offerType.BeginDate != null)
                {
                    dateExpiring = offerType.BeginDate.Value.AddDays(offerType.DaysValid);
                }
                // Notice that we haven't got the else throwing an InvalidOperationException
                // here because we are going to deal with this later.
            }
            else
            {
                throw new InvalidOperationException();
            }
            return dateExpiring;
        }
    }
}

Looking better but still got the ugly ifs. We will now take care of this and the way to do that is to make use of polymorphism as follows:

using System;

namespace WickedDomainModels.Model
{
    public abstract class ExpirationType : Enumeration
    {
        private ExpirationType(int value, string displayName)
            : base(value, displayName)
        {
        }

        public static readonly ExpirationType Assignment = 
            new AssignmentType();
        public static readonly ExpirationType Fixed = 
            new FixedType();

        public abstract DateTime CalculateExpiration(OfferType offerType);

        private class AssignmentType: ExpirationType{
            public AssignmentType() 
                : base(1, "Assignment")
            {
            }

            public override DateTime CalculateExpiration(OfferType offerType)
            {
                return DateTime.Now.AddDays(offerType.DaysValid);
            }
        }

        private class FixedType : ExpirationType
        {
            public FixedType()
                : base(2, "Fixed")
            {
            }

            public override DateTime CalculateExpiration(OfferType offerType)
            {
                return offerType.BeginDate.Value.AddDays(offerType.DaysValid);
            }
        }
    }
}

Before we go into the details of the code above let me remind you of the OfferType constructor:

public OfferType(string name, ExpirationType expirationType, int daysValid, DateTime? beginDate)

So the expirationType can either be “ExpirationType.Assigned” or “ExpirationType.Fixed”.

Now back to the code and in particular pay attention to the code in green which shows how by assigning a property (Assigned or Fixed) to the  ExpirationType will in turn create the correct subclass of the latter. So the property is determining which subclass is being created.

Then the code in red is used to declare an abstract method CalculateExpiration so that we can call

ExpirationType.CalculateExpiration(offerType)

we can delegate to the appropriate implemenation of the method in AssignmentType and FixedType shown in blue. Note how the latter two are declared as private classes because we don’t wan’t external clients to be able to instantiate an instance of either subclass. We have actually applied the strategy pattern here i.e. AssignmentType and FixedType are the strategies. Oh and one final thing which you might have not noticed is the fact that we no longer have an else statement which is throwing an exception since we are using strategies to calculate the expiry date.

I must however admit that I’m not too enamoured with this strategy pattern implementation because the class can get huge should the number of strategies increase significantly.

Conclusion

One of the conclusions that Jimmy Bogard had was that, as this walkthrough has shown, you can start with a poorly modelled domain but by refactoring, you can get a much better Domain Model which has the appropriate API and also is more decoupled.

Personally, I have learned a few things:

  • Use the appropriate language features. For instance, the code originally used an ICollection which was then changed to an IEnumerable. This meant that we couldn’t do funny things such as CopyTo. So this was a concrete demontration of the principle of least priviledge.
  • Try not to use enumerations but instead do something similar to what the ExpirationType did. This will help get rid of conditional structures.
  • Having an anaemic class might be an indication that the code actually needs refactoring.
Advertisements
Tagged with: , , , ,
Posted in .NET, 2012, NDC Walkthrough

Leave a Reply

Fill in your details below or click an icon to log in:

WordPress.com Logo

You are commenting using your WordPress.com account. Log Out / Change )

Twitter picture

You are commenting using your Twitter account. Log Out / Change )

Facebook photo

You are commenting using your Facebook account. Log Out / Change )

Google+ photo

You are commenting using your Google+ account. Log Out / Change )

Connecting to %s

%d bloggers like this: