Put your behaviour where it belongs

This past week I have been working on a feature which allows the application to record the net weight of a various loads on a shipment.  There are of course certain rules which control whether a weight can be accepted or not and other side affects to accepting a weight, such as setting the date and time the weight was accepted.  All of this should of course be controlled by my domain classes, but how often have you seen code that looks something like this:

	// This could just as easily be a command handler in a view model
	private void AcceptButton_Clicked(object sender, EventArgs e)
	{
	if (CurrentNetWeight <= 0)
	throw new Exception("Can not accept weights that <= 0");
	Load.GrossWeight = GrossWeight;
	Load.TareWeight = TareWeight;
	Load.NetWeight = CurrentNetWeight;
	Load.WeightAcceptedTime = DateTime.Now;
	}

view raw badway.cs hosted with ❤ by GitHub

I must confess I have been guilty of writing code like that my self, but I have learned a lot since then and I now know that this can be done better.  Too often we fall into the trap of making our domain objects just a place to hold our data.  If we do this we wind up having our core business logic and behaviours strewn all over our application.  More and more I tend to favour having private or protected setters for all my properties.  Then the only way to make changes is by calling methods which model the actions that the users actually want to perform.  In my shipping example they are accepting the weight, they aren’t just setting it.  Accepting the weight is a core business concept and it has rules which apply.  My code now looks more like this instead, which in my opinion is a vast improvement:

	// This could just as easily be a command handler in a view model
	privte void AcceptButton_Clicked(object sender, EventArgs e)
	{
	// Much better the behaviour and checks related to accepting a weight
	// Can go where they belong now
	Load.AcceptWeights(GrossWeight, TareWeight);
	}

view raw goodway.cs hosted with ❤ by GitHub

Your methods for changing domain states should be verbs that the business would use to describe the action that is actually taking place.  Going back to my shipping example a Weight is made up of multiple Weighs.  This is because sometimes a truck is too long to fit on a weighbridge so they need multiple readings.  They describe the process as capturing a weigh, so my method is called CaptureWeigh(decimal reading, CaptureType captureType).  There are rules when capturing a weigh which are applied in that method.  Typically though a lot of code would just add the reading to a collection provided through a property on the Weight class, but that removes the chance to have the business behaviour captured in the object that represents it in your model.
When modelling your domain this way you will find that nearly all of your collections shouldn’t be exposed as IList<T> because you want to control what happens when you add something to them.  Rather than exposing IList<T> use IEnumerable<T>.  If you need to get the count you can just use the Count() extension method, if the instance you call Count() on is an IList then it will actually just return straight away without looping through the entire sequence.  I still return the underlying IList<T> from my IEnumerable<T> property even though I don’t want people to modify it.  I realise that that means they can cast it and then make changes, but the fact that I am IEnumerable<T> means they should know that isn’t a supported scenario.  If they want to ignore that and muck with it anyway then that is their problem.  I have seen people advocate wrapping the underlying list in a ReadOnlyCollection<T> instance to prevent modification, but I feel this is overkill.  After all at the end of the day I can’t stop people from accessing the list anyway as they can always use reflection and if they are going to use things in ways that the interface clearly shows isn’t supported then good luck to them and if it blows up in their faces that is their problem!