Bowling Without Prefactoring

In Paul Pagel's blog about prefactoring, he talks about when you refactor in advance of adding a new feature with the idea that you're making the code easier to add that feature. The problem, as Paul points out, is that you're really designing up front when you do that.

I was discussing this with some students this week after we had done the bowling game exercise. As I was talking about the pit falls of prefactoring, I realized that there is a step we typically do in that exercise where we prefactor. It's when we are preparing to solve for spares. Before we do that, the test and code look something like this:

public class GameTest extends TestCase {

    private Game game;

    public void setUp() {
        game = new Game();
    }

    public void testAllGuttersScoreShouldBeZero() {
        rollPinsNTimes(0, 20);
        assertEquals(0, game.score());
    }

    public void testAllOnesScoreShouldBe20() {
        rollPinsNTimes(1, 20);
        assertEquals(20, game.score());
    }

    private void rollPinsNTimes(int numPins, int numRolls) {
        for (int i = 0; i < numRolls; i++) {
            game.roll(numPins);
        }
    }
}

public class Game {
    private int score;

    public void roll(int pins) {
        score += pins;
    }

    public int score() {
        return score;
    }
} 

The next step is typically to prefactor the Game class to handle an array of ints:

public class Game {

    private int[] rolls = new int[20];
    private int currentRoll;

    public void roll(int pins) { rolls[currentRoll++] = pins; }

    public int score() {
        int score = 0;
        for (int roll = 0; roll < rolls.length; roll++) {
            score += rolls[roll];
        }
        return score;
    }
} 

Now all the tests pass and we are allegedly set up to handle spares, but when we add the test for spares we end up changing the nature of the loop to solve the problem. Instead of looping from 0 to the rolls.length, we iterate from 0 to 10, representing frames. I remember when I first encountered this and thought how strange it felt to change the nature of the loop at that point.

I've come to believe that the oddness was related to this notion of prefactoring. We changed the design thinking we were going to support spares but, in fact, we got it wrong. We designed the wrong loop. Thinking about this brings up a number of questions. Aren't we designing up front in that case? Isn't that something to avoid? Isn't the idea to refactor to improve the structure?

So let's try a different approach. Let's go back to our implementation as it was after testing for all gutters and all ones.

public class Game {
    private int score;
    public void roll(int pins) { score = pins; }
    public int score() { return score; }
} 

Now, rather than prefactoring for spares, we just add the test...

public void testAllFivesScoreShouldBe150() {
    rollPinsNTimes(5, 21);
    assertEquals(150, game.score());
} 

... and run it and watch it fail. Now we have to make it pass without the other tests failing, right? We know that our tests use all 0's, all 1's and all 5's. The tests using 0's and 1's are passing. The only failing test is the test that uses 5's. We can use that temporarily to help us isolate the code to pass that test from the other code. First we modify Game as follows:

public class Game {
    private boolean handlingSpares;
    ...
    public int score() {
        if (handlingSpares)
            return 150;
        return score;
    }
} 

We run the test and it still fails because the game doesn't know we're testing spares. So we give it a way to know that it is...

public class Game {

    private int score;
    private boolean handlingSpares;

    public void roll(int pins) {
        score += pins;
        if (pins == 5)
            handlingSpares = true;
    }

    public int score() {
        if (handlingSpares)
            return 150;
        return score;
    }
} 

At first glance, this seems a bit crazy. We're introducing this variable that bears all sorts of assumptions. Well, how different is that, as a temporary measure, from hard coding a return value to get a test to pass. When we tell score() to return 0 to get the all gutters test to pass, aren't we doing something very similar? This is the simplest thing we can do to get our tests to pass.

And in fact, all the tests do pass.

So what's the next step? Refactor? Well, it might be, but there's not really much clear duplication here (at least I don't see it yet), and we really don't have guidance as to what to refactor to. So, next test:

public void testOneSpareGameScore() {
    rollPinsNTimes(5, 3);
    assertEquals(20, game.score());
} 

We now have two tests on spares that have to produce different scores. This is the first time that we are forced to collect the rolls and keep track of them in any way. So we introduce our integer array and our loop

public class Game {
    private int[] rolls = new int[21];
    ...
    public int score() {
        if (handlingSpares) {
            score = 0;
            int roll = 0;
            for (int frame = 0; frame < 10; frame++) {
                score += rolls[roll] + rolls[roll + 1] + rolls[roll + 2];
                roll += 2;
            }
        }
        return score;
    }
} 

Running the tests at this point fails both spares tests. The results are 0 for both games instead of 150 and 15. Why? Well we haven't started populating rolls yet. So we add it, leaving the whole class looking like this:

public class Game {

    private int score;
    private boolean handlingSpares;
    private int[] rolls = new int[21];
    private int currentRoll;

    public void roll(int pins) {
        score += pins;
        rolls[currentRoll++] = pins;
        if (pins == 5)
            handlingSpares = true;
    }

    public int score() {
        if (handlingSpares) {
            score = 0;
            int roll = 0;
            for (int frame = 0; frame < 10; frame++) {
                score += rolls[roll] + rolls[roll + 1] + rolls[roll + 2];
                roll += 2;
            }
        }
        return score;
    }
} 

At this point all four tests pass. And NOW, for the first time, we have clear duplication that we can factor out. We have two different ways of tallying the totals. The cool thing about this approach, is that the first time we introduce a loop through an array of ints, it's the right loop. We never needed to just loop through all the ints and tally them, so iterating from 0 to rolls.length was never required.

So now, we want to factor out the duplication in very small steps, with green bar between each. Here's a first step:

public int score() {
    score = 0;
    int roll = 0;
    for (int frame = 0; frame < 10; frame++) {
        if (handlingSpares) {
            score += rolls[roll] + rolls[roll + 1] + rolls[roll + 2];
        } else {
            score += rolls[roll] + rolls[roll + 1];
        }
        roll += 2;
    }
    return score;
} 

All the tests pass. Next we replace handlingSpares with the spare calculation:

public int score() {
    score = 0;
    int roll = 0;
    for (int frame = 0; frame < 10; frame++) {
        if (rolls[roll] + rolls[roll + 1] == 10) {
            score += rolls[roll] + rolls[roll + 1] + rolls[roll + 2];
        } else {
            score += rolls[roll] + rolls[roll + 1];
        }
        roll += 2;
    }
    return score;
} 

All the tests pass. At this point we can get rid of handlingSpares entirely and make score local to the score() method. Always running the tests between each step. This leaves the code looking like this:

public class Game {

    private int[] rolls = new int[21];
    private int currentRoll;

    public void roll(int pins) {
        rolls[currentRoll++] = pins;
    }

    public int score() {
        int score = 0;
        int roll = 0;
        for (int frame = 0; frame < 10; frame++) {
            if (rolls[roll] + rolls[roll + 1] == 10) {
                score += rolls[roll] + rolls[roll + 1] + rolls[roll + 2];
            } else {
                score += rolls[roll] + rolls[roll + 1];
            }
            roll += 2;
        }
        return score;
    }
} 

This is pretty much how the code usually ends up after solving for spares, and before refactoring out methods like isSpare(int roll) and nextRollForSpare(roll).

So there it is. I had a lot of fun with this experiment. Perhaps for some of you this is news, perhaps not so much for others. For me it was. For me this changes the way I think about the TDD cycle. For those of you for whom this IS news, I encourage you to experiment with it and post your experiences. The notion of prefactoring to add a new feature runs counter to the TDD flow. It's Test-Code-Refactor, not Refactor-Test-Code. Prefactoring is designing up front, and as with all designs up front, sometimes you get it right. Sometimes not so much.

---

!commentForm