Growing hairy software, guided by tests

Software grows organically. One line at a time, one change at a time. These changes soon add up. In an ideal world, they add up to a coherent architecture with an intention revealing design. But sometimes software just grows hairy – full of little details that obscure the underlying logic. What makes software hairy and how can we stop it?

Hairy code

Generally code starts out clean – brand new, shiny code. But each time you make a change that doesn’t quite fit the original design you add a hair - a small, subtle detail. It doesn’t detract from the overall purpose of the code, it just covers a specific detail that wasn’t thought of originally. One hair on its own is fine. But then you add another, and another, and another. Before you know it, your clean, shiny code is covered in little hairs. Eventually code becomes so hairy you can’t even see the underlying design any more.

Let’s face it, we’re all basically maintenance programmers. How many of us actually work on a genuinely greenfield project? And anyway, soon after starting a greenfield project, you’re changing what went before and you’re back into maintenance land. We spend most of our time changing existing code. If we’re not careful, we spend most of our time adding new hairs.

The simplest thing

When changing existing code, there’s a temptation to make the smallest change that could possibly work. Generally, it’s a good approach. Christ, TDD is great at keeping you focused on this. Write a test, make it pass. Write a test, make it pass. Do the simplest thing that could possibly work. But, you have to do the refactor step. “Red, green, refactor“, people. If you’re not refactoring, your code’s getting hairy. If you’re not refactoring, what you just added is a kludge. Sure, it’s a well tested, beautifully written kludge; but it’s still a kludge.

The trouble is, it’s easy to forgive yourself.

But it’s just a little if statement

It’s just one little change. In this specific case we want to do something subtly different. It may not look like it, but it’s a kludge. You’ve described the logic of the change but not the reason. You’ve described how the behaviour is different, but not why. Congratulations, you just grew a new hair.

An example

Perhaps an example would help right about now. Let’s imagine we work for an online retailer. When we fulfill an order, we take each item and attempt to ship it. For those that are out of stock, we add to a queue to ship as soon as we get new stock.

public class OrderItem {
    public void shipIt() {
        if (stockSystem.inStock(getItem()) > getQuantity()) {
            warehouse.shipItem(getItem(),
                               getQuantity(),
                               getCustomer());
        } else {
            warehouse.addQueuedItem(getItem(),
                                    getQuantity(),
                                    getCustomer());
        }
    }
}

As happens with online retailers, we’re slowly taking over the universe: now we’re expanding into shipping digital items as well as physical stuff. This means that some orders will be for items that don’t need physical shipment. Each item knows whether it’s a digital product or a physical product; the rights management team have created an electronic shipment management system (email to you and me) – so all we need to do is make sure we don’t try and post digital items but email them instead. Well, the simplest thing that could possibly work is:

public class OrderItem {
    public void shipIt() {
        if (getItem().isDigitalDelivery()) {
            email.shipItem(getItem(), getCustomer());
        } else if (stockSystem.inStock(getItem()) >
                       getQuantity()) {
            warehouse.shipItem(gettem(),
                               getQuantity(),
                               getCustomer());
        } else {
            warehouse.addQueuedItem(getItem(),
                                    getQuantity(),
                                    getCustomer());
        }
    }
}

After all, it’s just a little “if”, right?

This is all fine and dandy, until in UAT we realise that we’re showing delivery in 3 days for digital items. That’s not right, so we get a request to show immediate delivery for digital items. There’s a method on Item that calculates estimated delivery date:

public class Item {
    private static final int STANDARD_POST_DAYS = 3;
    public int getEstimatedDaysToDelivery() {
        if (isDigitalDelivery()) {
            return 0;
        } else if (stockSystem.inStock(this) > 0) {
            return STANDARD_POST_DAYS;
        } else {
            return stockSystem.getEstArrivalDays(getSKU()) +
                       STANDARD_POST_DAYS;
        }
    }
}

After all, it’s just one more if, right? Where’s the harm? But little by little the code is getting hairier and hairier.

The trouble is you get lots of little related hairs smeared across the code. You get a hair here, another one over there. You know they’re related – they were done as part of the same set of changes. But will someone else looking at this code in 6 months time? What if we need to make a change so users can select electronic and/or physical delivery for items that support both? Now I need to find all the places that were affected by our original change and make more changes. But, they’re not grouped together, they’ve been spread all over. Sure, I can be methodical and find them. But maybe if I’d built it better in the first place it would be easier?

A better way

This all started with a little boolean flag – that was the first smell. Then we find ourselves checking the state of the flag and switching behaviour based on it. It’s almost like there was a new domain concept here of a delivery method. Say, instead I create a DeliveryMethod interface – so each Item can have a DeliveryMethod.

public interface DeliveryMethod {
    void shipItem(Item item, int quantity, Customer customer);
    int getEstimatedDaysToDelivery(Item item);
}

I then create two concrete implementations of this:

public class PostalDelivery implements DeliveryMethod {
    private static final int STANDARD_POST_DAYS = 3;
    @Override
    public void shipItem(Item item, int quantity,
                         Customer customer) {
        if (stockSystem.inStock(item) > quantity) {
            warehouse.shipItem(item, quantity, customer);
        } else {
            warehouse.addQueuedItem(item, quantity, customer);
        }
    }
    @Override
    public int getEstimatedDaysToDelivery(Item item) {
        if (stockSystem.inStock(item) > 0) {
            return STANDARD_POST_DAYS;
        } else {
            return stockSystem.getEstArrivalDays(item) +
                       STANDARD_POST_DAYS;
        }
    }
}

public class DigitalDelivery implements DeliveryMethod {
    @Override
    public void shipItem(Item item, int quantity,
                         Customer customer) {
        email.shipItem(item, customer);
    }
    @Override
    public int getEstimatedDaysToDelivery(Item item) {
        return 0;
    }
}

Now all the logic about how different delivery methods work is local to the DeliveryMethod classes. This groups related changes together; if we later need to make a change to delivery rules we know exactly where they’ll be.

Discipline

Ultimately writing clean code is all about discipline. TDD is a great discipline – it keeps you focused on the task at hand, only adding code that is needed right now; all the while ensuring you have near complete test coverage.

However, avoiding hairy code needs yet more discipline. We need to remember to describe the intention of our change, not just the implementation. Code is primarily to be read by humans so expressing the reason the code does what it does is much more important than expressing the logic. The tests only ensure your logic is correct, you also need to make sure your code reveals it’s reasoning.

From http://blog.activelylazy.co.uk/2011/10/28/growing-hairy-software-guided-by-tests/