Wow, its been over a month since I posted last on this, and what a month it’s been. Over the last month I uncovered something pretty special: Domain-Driven Design by Eric Evans. Seriously, buy this book. Every developer should own a copy of this. Fact.
Right, now I’ve gotten that out of my system, I’ll continue with what I was planning on writing about. Something was up about my implementation. Something didn’t seem to smell right. I couldn’t put my finger on in, but I found myself coding myself into corners. I found myself coding unit tests that didn’t seem to be testing the right “layer” of code, and found myself getting mixed up in issues like “how to lazy-load collections” when really I knew that this persistence related code didn’t sit right amongst my business logic.
So, I did some research, and some reading, and here’s what I found…
Code Smells
When writing my first unit test for the problem in hand I made a few assumptions; first that the repository would be programmed against an abstraction (IPerson rather that Person) and second that the repository would return an IQueryable of Person objects. On reflection, perhaps I did this because I had an implementation in mind, or perhaps this was for another reason entirely, but I believe this lead me to a few issues and code smells.
First of all, I breached the YAGNI principle – I wasn’t trying to filter this list of people (or Persons), or at least, not at this stage of the game anyway, so I shouldn’t have been trying to return an IQueryable when perhaps an IEnumerable collection would have better represented the task in hand.
Secondly, I breached the purpose of the repository, to obtain a reference to the aggregate root, which in this case, should have been a collection of Person objects. What I actually returned was a IQueryable object; a query nothing else. Rather than try to explain why I believe this to be an issue, I think this is much better summed by Fredrik Normén in his post here. I really think Frederik hit the nail on the head in that a Repository should behave as a collection of entities, and to call a mechanism that serves up a query a “repository” goes against the original definition of what Martin Fowler and Evans intended.
I now think a better option to meet the goal of filtering a list of entities, or rather specifying the criteria a list of entities must adhere to in order to be returned by the repository (should I require this), would be to use the specification pattern. We can then compose our query using well defined domain concepts (like MandatoryPersonDetailsSpecification), chain specifications together if required (And/Or/Not) and then pass them into our repository. An example of this that I particularly like can be found on Scott Walkers blog here.
Code Responsibilities
I paused for thought when I realised I was writing unit tests that seemed to be testing the wrong “layer” of code. This was the code that sounded the alarm bells in my head:
[TestMethod]
public void Person_ShouldHave_Forename_Surname_Address_Fields_NotNull()
{
var p = (from persons in rep.Repository<IPerson>()
select persons).SingleOrDefault();
Assert.IsNotNull(p.Forename);
Assert.IsNotNull(p.Surname);
Assert.IsNotNull(p.Address);
}
The code in question is testing that the repository is returning values for Forename, Surname and Address…but surely it’s not the repositories responsibility to enforce our business rule that a person must have these attributes? I wanted to ensure that “draft” person objects could be captured in our domain, but full person details must be provided in order for a person to be “published”. So where was I going wrong?
Lets consider the facts – the concept of a “draft” person is a domain concept; the rules enforcing this should therefore be contained in the domain. I had previously considered this as a persistence only concern, however I had overlooked the fact that the rules defining whether a person was “complete” or “draft” have to exist in the domain.
I no longer necessarily require a “draft repository” for persisting temporary data; a draft person object (or a person deemed to be in a draft state) is more suitable in this context.
Lastly, in regard to lazy loading, my hunch is that by carefully considering where my aggregate boundaries are, I shouldn’t need to worry about lazy loading, especially considering this is such a basic example.
Where do I go next from here?
What this really drove home to me is that the first few hours of TDD really are the most painful. I’m going to stick with it though, and using that in conjunction with my new favourite book, I should be a few steps closer to writing more elegant code.
