Kyle Cordes

Ron Jeffries recently posted a couple of articles on simple design, with the
specific example of code to score a bowling game:

• First article,
  with lots of classes
• Second
  article, a much shorter procedural solution

In a thread on the subject on the XP Mailing List, various posters expressed
a preference for one solution or the other.

With the caveat that I’m normally a very object-oriented guy, I prefer the
second (procedural) solution, in that I find has approriately simple design:
it work, expresses the programmer’s intentions, has (almost) no duplication
of ideas, and has the minimal amount of code needed to do all those things.

The point was made, though, that perhaps the procedural solution would fall
apart once the complexity of the problem increased. To test this idea, I decided
to extend it with the following features:

• Know what frame the bowler is current done with / working one
• Know the score of each frame
• Know if the game is over
• Reject erroneous input data

My first step was to convert the example C# code to Java. (I like C# at least
as much as Java, but for the moment I’m happier with the code-editing features
of Eclipse compared to VS.NET.) Here’s the Java code, the starting point before
adding any features:

/*
 * Trivial port of Ron Jeffries' Bowling to Java
 */
package bowling;

import java.util.ArrayList;

public class BowlingGame {

  ArrayList rolls = new ArrayList();

  public void roll(int roll) {
    rolls.add(new Integer(roll));
  }

  public int score() {
    int rollIndex = 0;
    int total = 0;
    for (int frame = 0; frame < 10; frame++) {
      if (strike(rollIndex)) {
        total += 10 + pinCount(rollIndex + 1) + pinCount(rollIndex + 2);
        rollIndex++;
      } else if (spare(rollIndex)) {
        total += 10 + pinCount(rollIndex + 2);
        rollIndex += 2;
      } else {
        total += pinCount(rollIndex) + pinCount(rollIndex + 1);
        rollIndex += 2;
      }
    }
    return total;
  }

  private boolean strike(int rollIndex) {
    return pinCount(rollIndex) == 10;
  }

  private boolean spare(int rollIndex) {
    return pinCount(rollIndex) + pinCount(rollIndex + 1) == 10;
  }

  private int pinCount(int pinPosition) {
    return ((Integer) rolls.get(pinPosition)).intValue();
  }
}

I also converted the NUnit tests to JUnit. (Formerly this was available for download,
but I’ve misplaced the file since then.)

With this in hand, I started adding tests for my new features. An example test:

  public void testStrikeWithFrameCounting() {
    game.roll(10);
    assertEquals(1, game.finishedFrames());
    assertEquals(0, game.scoredFrames());
    game.roll(5);
    assertEquals(1, game.finishedFrames());
    assertEquals(0, game.scoredFrames());
    game.roll(3);
    assertEquals(2, game.finishedFrames());
    assertEquals(2, game.scoredFrames());
    game.roll(2);
    assertEquals(2, game.finishedFrames());
    game.roll(1);
    assertEquals(3, game.finishedFrames());
    assertEquals(3, game.scoredFrames());
    rollMany(14, 0);
    assertEquals(10, game.finishedFrames());
    assertEquals(10, game.scoredFrames());
    assertEquals(29, game.score());
  }

This test, and others like it, verify that after each roll, the game object
know what frame has been completed, what frame is scorable, and when the game
is over. (I also added the feature to report the score of each frame, but didn’t
get around to testing it explicitly. I found that to get things right, I needed
a variety of tests for things happening in the last frame, since there are special
rules there.

Without further ado, here is working code to score the game, with the new features:

/*
 * Bowling Scorer, converted to Java, extended to know what
 * frame we are on, what frame has been scored, whether the
 * game is over, and the score of each frame
 */
package bowling;

public class BowlingGame {

  private static final int NUM_FRAMES = 10;
  private static final int NUM_PINS = 10;
  private static final int MAX_ROLLS_IN_GAME = NUM_FRAMES*2 + 1;

  // Input state:
  private int[] rolls = new int[MAX_ROLLS_IN_GAME];
  private int rollSoFar = 0;

  // Output state:
  private int[] frameScores = new int[10];
  private int scoredFrame;
  private int finishedFrame;

  // Processing variables; these would be locals, but
  // this class essentially is a "method object", so we
  // use instance variables instead of param passing
  private int scoringFrame;
  private int scoringRoll;

  public void roll(int roll) {
    if(roll<0 || roll>NUM_PINS)
      throw new RuntimeException("Roll out of range");

    if(gameOver())
      throw new RuntimeException("The game is over, no more rolls allowed.");

    rolls[rollSoFar++] = roll;
    calculate();
  }

  private void calculate() {
    scoredFrame = 0;
    finishedFrame = 0;
    scoringRoll = 0;
    for (scoringFrame = 1; scoringFrame <= NUM_FRAMES; scoringFrame++) {
      if (isStrike()) {
        scoreStrike();
      } else if (isSpare()) {
        scoreSpare();
      } else {
        scoreNormal();
      }
    }
  }

  private boolean isStrike() {
    return rolls[scoringRoll] == NUM_PINS;
  }

  private void scoreStrike() {
    storeFrameScore(NUM_PINS + rolls[scoringRoll + 1] + rolls[scoringRoll + 2]);
    frameIsScoredIfWeHaveRollOffset(2);

    if(scoringTheLastFrame())
      frameIsDoneIfWeHaveRollOffset(2);
    else
      frameIsDoneIfWeHaveRollOffset(0);
  }

  private boolean isSpare() {
    return rolls[scoringRoll] + rolls[scoringRoll + 1] == NUM_PINS;
  }

  private void scoreSpare() {
    storeFrameScore(NUM_PINS + rolls[scoringRoll + 2]);
    frameIsScoredIfWeHaveRollOffset(2);

    if(scoringTheLastFrame())
      frameIsDoneIfWeHaveRollOffset(2);
    else
      frameIsDoneIfWeHaveRollOffset(1);
  }

  private void scoreNormal() {
    storeFrameScore(rolls[scoringRoll] + rolls[scoringRoll + 1]);
    frameIsScoredIfWeHaveRollOffset(1);
    frameIsDoneIfWeHaveRollOffset(1);
  }

  private boolean scoringTheLastFrame() {
    return scoringFrame == NUM_FRAMES;
  }

  private void storeFrameScore(int frameScore) {
    frameScores[scoringFrame - 1] = frameScore;
  }

  private void frameIsDoneIfWeHaveRollOffset(int rollOffset) {
    if(scoringRoll + rollOffset < rollSoFar) {
      finishedFrame = scoringFrame;
    }
    // Continue scoring at the roll after the last one
    // on this frame:
    scoringRoll += rollOffset + 1;
  }

  private void frameIsScoredIfWeHaveRollOffset(int rollOffset) {
    if(scoringRoll + rollOffset < rollSoFar) {
      scoredFrame = scoringFrame;
    }
  }

  // The public interface has a few more methods for the new features:

  public int score() {
    int totalScore = 0;
    for(int i=0; i<frameScores.length; i++)
    totalScore += frameScores[i];
    return totalScore;
  }

  public int scoredFrames() {
    return scoredFrame;
  }

  public int finishedFrames() {
    return finishedFrame;
  }

  public int scoreForFrame(int frame) {
    return frameScores[frame-1];
  }

  public boolean gameOver() {
    return finishedFrame == NUM_FRAMES;
  }
}

Here is what I like about this solution:

• There are tests to show that it actually works
• The method names are, at least to me, intention-revealing
• The methods are short and straightforward
• The main scoring loop, which still happens in one straight-line pass, is
  simply this:

      for (scoringFrame = 1; scoringFrame <= NUM_FRAMES; scoringFrame++) {
        if (isStrike()) {
          scoreStrike();
        } else if (isSpare()) {
          scoreSpare();
        } else {
          scoreNormal();
        }
      }

• There’s not much duplication left in there; perhaps someone can suggest
  a way to get rid of the bits that remain

The real question, of course, is whether this is too much for one class to
do – whether there are any parts of this code that should be a separate class.
The obvious candidates are the pairs of methods: should isStrike() and scoreStrike()
somehow be in a Strike class? They have an obvious parallel structure, a form
of duplication that I might be able to get rid of by adding classes.

I thought these new features would push me there – but they didn’t. Perhaps
a future story would do so. This implementation is still not quite “finished”,
in that I know there are more kinds of error conditions to consider, test for,
and implement. I don’t see any reason to think that adding those now would add
much to the conversation, so I didn’t add them.

One lesson of this exercrise, to me, is a common one: the problem space is
not the solution space. Just because we have a thing called a Foo in the problem
domain, doesn’t mean we need a class Foo in the solution. We might, we might
not; with test-driven design, the need for a Foo should eventually become obvious.

Comments welcome, via email (address below) or on the XP mailing list.