(Red-Green)N-Refactor

Red/Green/Refactor is the TDD mantra. And fair enough, it's a good description of the concept of TDD which I guess it's meant to describe. Heck it even describes the order you should perform these steps in when writing a test. Make sure the test is able to fail, make sure it passes for the right reasons and when done clean up. This is all good but I see people being mislead by this mantra. To explain this a bit better let's have a look at Uncle Bob's three rules of TDD

1. You are not allowed to write any production code unless it is to make a failing unit test pass.

2. You are not allowed to write any more of a unit test than is sufficient to fail; and compilation failures are failures.

3. You are not allowed to write any more production code than is sufficient to pass the one failing unit test.

Following these rules (and you should!) you'll see that when writing code you're mantra should in fact be red/green/red/green/red/green/............./refactor. Frankly though that wouldn't make a very good mantra. At the same time red/green/refactor doesn't work as an implementation pattern. The same way as agile methods will have you delivering working software in short iterations these three rules will deliver a suite of passing tests in about 30 second long iterations. And more importantly every red/green cycle will have you focusing on a single small task which is an efficient way of working. Summing up the mantra through Uncle Bob's three rules we get red/green repeated until the test is completed. This gives us (Red-Green)N.
So, why isn't refactoring part of the three rules of TDD? TDD or not TDD you should ALWAYS refactor. Make sure your code is readable and maintainable. TDD just happens to make your code a lot easier to refactor safely. Given that the concept of TDD tells you to refactor for every completed test (and you should!) the mantra you should follow when implementing is (Red-Green)N-Refactor.

Focus on functionality, use technology - Revisited

Lars-Erik Kindblad  posted some great questions about the implementation in my previous post. I thought I'd post the reply here as the questions pins a lot of the thought behind the previous post. The code up for discussion is:

public void BlockCustomer(int customerID)
{
    var customer = getCustomer(customerID);
    customer.Block();
}

He's questions were:

1) How would you go about unit testing the second example? Since the customer is not injected into the constructor, the unit test would have a dependency on the customer.Block() method?
2) Would customer.Block() (and customer.Save(), Remove(), Notify() etc.) break the Single Responsibility Principle?
3) I agree customer.Block() creates more readable code, but it can create large and messy entity classes when the code base is growing. I've seen such classes with 1000+ lines of code. In that case I rather prefer using manager/service classes in order to partition the functionality across multiple classes.

So, first thing. In my code I use repositories extensively. Most likely the getCustomer method would use some repository to fetch the customer. The class holding the BlockCustomer method would therefore have the ICustomerRepository injected. When dealing with dependency injection I separate between stateless classes and state full classes. Usually I automatically inject stateless classes through a dependency injection container.  The repository class is a stateless class and would be injected into the class holding the BlockCustomer method automatically. The repository would be responsible for constructing the state full class Customer. To make sure that we are  able to write maintainable tests for the BlockCustomer method we'll have the repository return an ICustomer interface.

On to the second question, what about SRP. He is absolutely right, having the save and remove methods on the customer would break the Single Responsibility Principle.When using a domain model I'm not a big fan of using that type of active record implementation. Add and Remove is something I would have implemented in the ICustomerRepository. Within the repository I would use some kind of data mapper framework (NHibernate, EF..). Preferably one that supports automatic entity change tracking so that I would not have to implement Save. But still he has a point. From the first example in the previous post we could see that customer.Block() would have to do more than modifying it's own state. However I don't see anything wrong with it delegating the rest of it's work to a dependency. The dependency would be injected into the Customer class by the customer repository before handing it off.

class Customer
{
    ...
    private CustomerState _state;
    private IDependencyNeededToBlock _dependencyNeededToBlock;
    
    public IDependencyNeededToBlock DependencySetter
    {
        set
        {
            if (_dependencyNeededToBlock != null)
                throw new Exception("Dependency already set");
            _dependencyNeededToBlock = value;
        }
    }

    public void Block()
    {
        _state = CustomerState.Blocked;
        _dependencyNeededToBlock.DoWhatIsNeeded();
    }
}

However what is important to note is that the implementation should show the intention of the functionality. Since the intent of this functionality was blocking a customer it's natural that the functionality is initiated by the customer. If on the other hand the functionality was about something else and part of it was blocking the customer then of course that other functionality would not be delegated by the customer.

If the Customer class now is limited to modifying internal state and calling dependencies we should have a nice and maintainable class. On the other hand if it still ends up as a 1000+ lines class we probably need to redefine our perception of the word customer :)

Focus on functionality, use technology

As time goes I find myself more and more intrigued by Domain Driven Design. It is for me like a shift where we go from forcefully molding functionality to fit the technology to elegantly produce functionality with the help of technology. Instead of looking at a piece of code and seeing Nhibernate sessions / Datasets and terms like update, delete and insert we'll see the true intention of the functionality we're looking at. The following example is code the way I would have written it some time ago.

    public void UpdateCustomerState(int customerID, CustomerState newState)
    {
        DataSet ds = getCustomerDataset(customerID);
        ds.Tables["Customer"]
            .Rows[0]["CustomerState"] = stateToInt(newState);
        
        switch (newState)
        {
            case CustomerState.Blocked:
                // Do varius stuff done to blocked customers
                break;
                // Handle more state variants...
        }
        updateCustomerDataset(ds);
    }

Now, looking at this code it looks pretty much like any code we would expect to find anywhere in any source code right? It has split some functionality out into separate methods to clean up the code. It reads pretty well so we can understand what it does, it sets the state of the customer. So what's the problem?
The problem is that it's all done from the tooling perspective and not from the actual intent of the functionality. What happened here was that the developer was told to create the functionality for blocking a customer. The developer did as we usually do.. went right into techno mode. "Ok, we have this state field in the customer table. If I just set that field to state blocked and make the necessary changes to the linked rows in table x and y that should do the trick." And when done the code looked like the code above. This is very much like what happens in Hitchhikers Guide To the Galaxy when Deep Thought reveals that the answer to the Ultimate Question of Life, the Universe, and Everything as being 42. As we know the answer wasn't the problem. The problem was that question. We can say the same thing about this piece of code. What you see is the answer but there is nothing mentioned about the intention behind it. When browsing through the classes you'll only find a method called UpdateCustomerState and nothing about blocking customers.

So what do we do about it? We write the code as stated by the intent behind the functionality. The developer was told that a customer needed to be able to be blocked. From that we can determine that a customer is some entity and it needs a behavior which is block. The implementation would look something like this:

public void BlockCustomer(int customerID)
    {
        var customer = getCustomer(customerID);
        customer.Block();
    }

The first example is the typical: Classes with mostly properties in addition to a set of manager/handler/service classes manipulating these properties to achieve desired behavior. The second example keeps the customers state hidden within the customer and only exposes it's behaviors.

Write functionality with the help of technology!