Saturday, 9 February 2008

Refactoring / Design: Composed Method

I'm going to try and write a few posts over the next few weeks around the subject of Refactoring and Design. This is mainly practice for me, so that when asked to explain, I don't confuse the issue. All comments and suggestions are gratefully accepted.

When I am looking at code, I feel a lot more comfortable if a method is composed of several steps of similar context, in the appropriate order.

This is the Composed Method pattern from Smalltalk Best Practice Patterns.
In Refactoring To Patterns, Joshua Kerievsky says that this is one of the most important refactorings that he knows, and I would have to agree.

Here is an example that I just thought of (see if you can guess what I was doing):

public class CheeseCake {

private final Person chef;
private final Sink kitchen;
private CakeTin<CheeseCake> cake;

public CheeseCake (Person chef, Sink sink) {
this.chef = chef;
this.sink = sink;

public makeBase(Ingredients... ingredients) {
Bowl<Ingredients> bowl = new Bowl<Ingredients>();
for (Ingredients i : ingredients) {
CakeTin<CheeseCake> cake = new CakeTin<CheeseCake>();


Wow, we have quite a few smells in this code, and not the pleasant smells of cheesecake cooking either...
We'll ignore the Feature Envy and things for the moment, but I've only just written this code and already I'm having trouble telling what it does at a glance...

What about if the makeBase method looked like this:

public makeBase(Ingredients... ingredients) {
Bowl<Ingredients> bowl = addTo(new Bowl<Ingredients>(), ingredients);
cake.add(bowl.contents(); // Moved CakeTin creation into constructor

That looks much better, but not only that, we also find that the washHands() method is used extensively. Now each step in the recipe doesn't need to implement their own hands washing routine.

It's also much more obvious that washHands has absolutely nothing to do with CheeseCake, and much more to do with either Person or Sink... maybe we'll look at this next time :-)


Kris said...

I'm not a fan of a CheeseCake taking in a chef and a sink and instructing the chef to wash his hands, but perhaps I'm jumping ahead in your story. Anyway, I think I would rather see an initial refactoring look more like this:

public makeBase(Ingredients... ingredients) {

I'm not claiming this is perfect, but would be more in-line with a first phase refactoring:

1. Washing hands is a behaviour between the chef and sink. The details of which should not be exposed to the cheesecake.

2. The ingredients getting added to a bowl pushes the loop as a single operation into a single method.

3. Might as well move the construction of all the objects but allow them to be injected.

Andy said...

That's looking good :-)
The point of the post was that the first thing we should do is get to a consistent detail level.
I rolled up the "how we wash our hands" into a "what are we doing?" washHands method.
My feeling is that washing hands is nothing to do with the making of the base. The chef should make sure he washes his hands before he starts making the base, but we'll get there eventually :-)