Skip to content
February 6, 2011 / marrowboy

Composition over Inheritance

Hey, so I thought I’d share a situation that I saw recently, and how it relates to the classic Gang Of Four tenet of favouring Composition over Inheritance.  I really believe that the only universally-applicable “best practice” is to think about what you’re doing, so I wasn’t going to blindly follow GoF advice, no matter how well-respected they are.  Rather, let’s treat it as an experiment and see what the pros and cons are for Inheritance and Composition.

The Problem

The problem is very simple: We would like to apply some already-written “processing” methods to various lists of items. The processing is always the same, but there are many ways to get the list of items.  First, lets see the two approaches.

Inheritance

We will use an abstract base class which can be subclassed, and each subclass can implement its own way of getting lists

public abstract class BaseListProcessor {

    public List<Item> getListAndProcess(){
        List<Item> items = this.getList();
        doProcessing(items);
        doMoreProcessing(items);
    }

    protected abstract List getRawList();

    // ... code for processing is in here too
}

Then there are some concrete subclasses, which all look roughly like this:

public class FilteredListProcessor extends BaseListProcessor {

    @Override
    protected List<Item> getRawList() {
        Filter itemFilter = this.createComplexFilter();
        // NB itemSource is set by dependency injection
        return itemSource.getFilteredList(itemFilter);
    }
}

Lets say there are 4 classes like this, so there are also 4 unit tests. The ItemSource can be mocked during unit tests, so each test is something like the next code snippet. Notice that all 4 tests will be using the processing code.

    @Test
    public void filteredListsAreProcessedCorrectly (){
        expect( mockItemSource.getFilteredList() )
            .andReturn( someKnownListOfItems );

        List processedItems =
            filteredListProcessor.getListAndProcess();

        // assert that the processedItems list
        // contents are as we expect.
    }

We would have to jump through a hoop or two to test the getRawList methods directly as they are protected. Not impossible to get at, but inconvenient by design.

We could also have a unit test for the processing part of the abstract class by creating a mocking subclass which we can control the getRawList method – but I’ve noticed that these are often absent when there is an abstract base class.

Composition

The code for the composition solution is like this:

public class ListProcessor {
    
    public void getProcessedList(){
        List<Item> items = itemListSource.getList();
        doProcessing(items);
        doMoreProcessing(items);
    }

    // ... processing stuff still down here
}

ItemListSource is an interface which defines the .getList() method, and one of its concrete implementations is passed into the ListProcessor class. As before there are 4 concrete implementations, looking something like this:

public class FilteredListProcessor implements ItemListSource {

    @Override
    protected List<Item> getList() {
        Filter itemFilter = this.createComplexFilter();
        return itemSource.getFilteredItemList();
    }
}

But now, there are 5 totally independent tests. We’ve put a seam (ie a decoupling) along an interface. Each implementation of ItemListSource can be tested in isolation, and there can also be an independent test for the processor class which uses a mock to avoid relying on any particular instance of ItemListSource.

Discussion

Inheritance solution

Here, we are tightly-coupling the distinct operations of retrieving the list and processing it. This might not seem like such a big problem initially, because in this pair of operations is always tied together in our expected use-cases. But it has a big disadvantage in terms of testability! We’ve thrown the Single Responsibility Principle to the wind, and now we can’t isolate the getting and processing stages, even in our unit tests – so we can’t test them independently.

A single change that breaks the processing will cause 5 tests to fail. And this happens because we have to deal with the processing behaviour just to be able to test the way that the items are got in the first place! A clear failure to keep ourselves DRY!

There’s also the dubious and smelly function name: getListAndProcess. More on that later.

Composition solution

This doesn’t suffer from the testability problems we saw above, demonstrating perhaps that TDD is telling us what is likely to be bad design.

We were able to separate all logical sections of the code, but at what cost? Well our DI configuration will be more complex – we need to create 8 classes instead of 4, and wire them up right. So it’s arguably harder to see the overall structure of the program. We’ve created a complex network of simple objects instead of a simple network of complex objects.

Conclusion

With the abstract base class solution, we had to work hard to test each part, and still our tests overlap and are brittle. The DI config is easier, and the code may be slightly shorter – fewer classes, interfaces, tests, but it is less flexible and robust to change (How could we implement a class which got and processed 2 types of list at once? This would necessitate a major refactor or code-duplication in the inheritance model, but with composition all you’d need is a composite ItemListSource and you’re away!)

Smells

  • Abstract base class – not universally bad but there’s usually better ways to achieve the same end. Restrictive, too, if you have the misfortune to meet a library that wants you to extend their base classes.
  • Awful stinky method name: getListAndProcess. Do this AND do that. If you have to use And in a method, it’s usually pretty clear that you’re conflating two responsibilities in your code. Stop it. Refactor now – it will only be harder later.
  • Awkward testability. I linked to it above but I’ll quote it here for truth: TDD gives you immediate feedback about what is likely to be bad design. (although it doesn’t automagially give you good design either).

Conclusion

If you find yourself designing an inheritance-based solution to a coding problem, you should probably stop and think about whether you’re harming your ability to be testable and flexible to change. Composition usually works out better. But like I said at the top: think about what you’re doing, – it’s the best practice.

Advertisements

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: