Developer Testing: Strike!

March 20, 2007 - Strike!

According to the rules:

2.1.3 A strike is made when a full setup of pins is knocked down with the first delivery in a frame.

The test for that is...


      @Test
      public void aStrikeIsTenPinsDownWithFirstBall(){
        frame.addBall(10);
        assertThat(frame.isStrike(), is(true));
      }

...and after the familiar dance of the boolean return value we end up with:

      public boolean isStrike() {
        return firstBall != null && firstBall == 10;
      }

It is marked by an X


    @Test
    public void aStrikeIsMarkedByX(){
      frame.addBall(10);
      assertThat(frame.getFirstBall(), is("X"));
    }

...and the code to pass it is...


    public String getFirstBall() {
      if(isStrike()){
        return "X";
      }
      return formatBall(firstBall);
    }

The count for one strike is 10 plus the number of pins knocked down on the player’s next two deliveries.

That seems straightforward...


    @Test
    public void strikeShouldScoreTenPlusNextTwoDeliveries() {
      frame.addBall(10);
      frame.addBall(4);
      frame.addBall(5);

      assertThat(frame.getCumulativeScore(), is(19));
    }

...until the test rudely reminds me...

java.lang.IllegalArgumentException: Total pin count invalid

...that the total pin count in a frame cannot (currently) exceed 10. Also, a frame can only deal with two balls. Things are about to get more complex.

Once again, I hear the TDD angel on my shoulder telling me to remove that failing test before I add another but, once again, I'll pretend I did not hear him. One day, he'll tell me "I told you so" but, until then, I'll remain confident in my ability to deal with two (one high level, one low level) failing tests at the same time.

A New Concept in Frames

I need to introduce a new concept to distinguish between a frame is complete and a frame needs more balls. I also need a place to store that additional ball but - one thing at a time - I'll start with the frame complete idea. I'll consider a frame complete if it has two balls...

[from now on, assume that I always do the boolean method dance because, I do. Really, I do.]


    @Test
    public void aFrameIsCompleteIfItHasTwoBalls(){
      frame.addBall(4);
      frame.addBall(5);

      assertThat(frame.isComplete(), is(true));
    }

    @Test
    public void aFrameIsIncompleteUntilTheSecondBall(){
      frame.addBall(4);

      assertThat(frame.isComplete(), is(false));
    }

    public boolean isFrameComplete() {
      return secondBall != null;
    }

...unless the frame is a strike.


    @Test
    public void aStrikeFrameIsComplete(){
      frame.addBall(10);
      assertThat(frame.isComplete(), is(true));
    }

    public boolean isFrameComplete() {
      return secondBall != null || isStrike();
    }

Now I need to modify needsMoreBalls() for the strike situation.

    
    @Test
    public void aStrikeFrameNeedsThreeBalls(){
      frame.addBall(10);
      frame.addBall(5);
      assertThat(frame.needsMoreBalls(), is(true));
    }

I need to modify addBall() to not reject the second ball if the first one was a strike and then I can change needsMoreBalls() to make the test pass.


    public void addBall(int pins) {
      if(pins > PINS_IN_FRAME || pins <0) {
        throw new IllegalArgumentException("Invalid pin count - " + pins);
      }

      if(! isStrike()){
        if(firstBall != null && firstBall + pins > PINS_IN_FRAME) {
          throw new IllegalArgumentException("Total pin count invalid");
        }
      }

      if(firstBall == null) {
        firstBall = new Integer(pins);
      }
      else {
        secondBall = new Integer(pins);
      }
    }

    public boolean needsMoreBalls() {
      return isStrike() || secondBall == null;
    }

A Third Ball

I also need a place to store that third ball. I'll just add another field, thirdBall, for now but note that while two similar things might suggest that an array of balls would be a good idea, three similar things screams "USE AN ARRAY" at the top of its lungs. But I am already far enough from green that I'll postpone that refactoring until all the tests are passing.

I'll write the test that requires a third ball then use it in addBall() and needsMoreBalls()


    @Test
    public void aStrikeFrameDoesNotNeedFourBalls(){
      frame.addBall(10);
      frame.addBall(5);
      frame.addBall(4);
      assertThat(frame.needsMoreBalls(), is(false));
    }


    public boolean needsMoreBalls() {
      return isStrike() && thirdBall == null || secondBall == null;
    }

    public void addBall(int pins) {
      if(pins > PINS_IN_FRAME || pins <0) {
        throw new IllegalArgumentException("Invalid pin count - " + pins);
      }

      if(! isStrike()){
        if(firstBall != null && firstBall + pins > PINS_IN_FRAME) {
          throw new IllegalArgumentException("Total pin count invalid");
        }
      }

      if(firstBall == null) {
        firstBall = new Integer(pins);
      }
      else if(secondBall == null) {
        secondBall = new Integer(pins);
      }
      else {
        thirdBall = new Integer(pins);
      }
    }

I am finally back to a sane state. I have one failing test that is telling me

java.lang.AssertionError: Expected: is <19> got : <14> at kevin.lawrence.bowling.FrameTest.strikeShouldScoreTenPlusNextTwoDeliveries(FrameTest.java:108)

but I have been away from green for quite a long time and the angel on my left shoulder is whispering "I told you so". I had better make that test pass quickly and move on to a little refactoring. The test wants me to include the next two deliveries in the score for a strike which is easy enough.


    public int getCumulativeScore() {
      int cumulativeScore = 0;

      if(firstBall != null) {
        cumulativeScore += firstBall;
      }

      if(secondBall != null) {
        cumulativeScore += secondBall;
      }

      if(thirdBall != null) {
        cumulativeScore += thirdBall;
      }

      if(previousFrame != null){
        cumulativeScore += previousFrame.getCumulativeScore();
      }

      return cumulativeScore;
    }

Now everything is green once more and I can get rid of swathes of duplication by introducing an array for the balls. I'll just show the whole, post-refactoring Frame class.


  public class Frame {
    public static final int PINS_IN_FRAME = 10;

    private Integer[] balls = new Integer[3];
    private int ballCount;

    private Frame previousFrame;

    public Frame(Frame previousFrame) {
      this.previousFrame = previousFrame;
    }

    public String getFirstBall() {
      if(isStrike()){
        return "X";
      }
      return formatBall(balls[0]);
    }

    public String getSecondBall() {
      return formatBall(balls[1]);
    }

    private String formatBall(Integer ball) {
      if (ball == null) {
        return "";
      }
      else if(ball == 0){
        return "-";
      }
      else {
        return ball.toString();
      }
    }

    public void addBall(int pins) {
      if(pins > PINS_IN_FRAME || pins < 0) {
        throw new IllegalArgumentException("Invalid pin count - " + pins);
      }

      if(! isStrike()){
        if(ballCount > 0 && balls[0] + pins > PINS_IN_FRAME) {
          throw new IllegalArgumentException("Total pin count invalid");
        }
      }

      balls[ballCount++] = new Integer(pins);
    }

    public boolean needsMoreBalls() {
      return isStrike() && ballCount < 3 || ballCount < 2;
    }

    public String getScore() {
      return needsMoreBalls()
           ? ""
           : Integer.toString(getCumulativeScore());
    }

    public int getCumulativeScore() {
      int cumulativeScore = 0;

      for(Integer ball : balls) {
        if(ball != null){
          cumulativeScore += ball;
        }
      }

      if(previousFrame != null){
        cumulativeScore += previousFrame.getCumulativeScore();
      }

      return cumulativeScore;
    }

    public boolean isStrike() {
      return ballCount > 0 && balls[0] == 10;
    }

    public boolean isComplete() {
      return ballCount == 2 || isStrike();
    }

  }

Loose Ends

With that refactoring, it's easier to see what the code does and this seems like a good moment to review what work remains.

Although the Frame class is able to distinguish between frame complete and needs more balls, the Game class is not yet so sophisticated.
I don't display the second ball in a strike frame correctly (not to mention the third ball).
There is a chance that someone could send too many balls to a Frame. On a different day, I might add some defensive code to addBall() to disallow that but, since our Game already protects against it, I'll let it slide.
The code for formatting a roll is almost-but-not-quite tempting me to introduce a new class Roll. My old heuristic for introducing a new class was data + behavior -> new class but I am more conservative these days. If I have duplication - in other words, if the behavior starts to appear in multiple places - I'll introduce a new class. In this case, there is not much behavior, it's well encapsulated in Frame and there is no duplication so I'll leave it where it is.

I'll tidy those two loose ends and then I think I'll be done for this story.

I add this test to GameTest...

    
    @Test
    public void aStrikeFrameHasOnlyOneBall() {
      game.bowl(10);
      game.bowl(2);
      game.bowl(3);

      Iterator<Frame> frames = game.iterator();

      Frame frameOne = frames.next();
      assertThat(frameOne.getFirstBall(), is("X"));

      Frame frameTwo = frames.next();
      assertThat(frameTwo.getFirstBall(), is("2"));
      assertThat(frameTwo.getSecondBall(), is("3"));
    }

...and make it pass by adding a new loop termination condition to bowl(). The current frame is the first frame which needs more balls but is not complete. If I could find a way to express that more clearly in the code, I would. But, since I can't, I add a rare comment.


  public void bowl(int pins) {
    ballCount++;

    // The current frame is the first one that needs more balls
    // but is not complete

    for(Frame frame : frames){
      if(frame.needsMoreBalls()){
        boolean isCurrentFrame = ! frame.isComplete();

        frame.addBall(pins);

        if(isCurrentFrame) {
          break;
        }
      }
    }
  }

It's Not Over Yet

While looking at the Game code, I realize that my rule for detecting game over is wrong. I can no longer just count the balls because there might be a bonus ball in the last frame. I'll write a test for that but, rather than add it to the existing GameTest, I'll start a new fixture so I can do all the setup for my last frame tests in one place.

    
    public class LastFrameTest {
      private Game game;

      @Before
      public void createGameWithNineFramesOfGutterBalls() {
        game = new Game();

        for(int i = 0; i <9; i++) {
          game.bowl(0);
          game.bowl(0);
        }
      }

      @Test
      public void theLastFrameRequiresThreeBallsIfItIsStrike() {
        game.bowl(10);
        game.bowl(2);
        assertThat(game.isGameOver(), is(false));

        game.bowl(3);
        assertThat(game.isGameOver(), is(true));
      }

    }

I violated the one logical assert per test rule here because I think it's clearer to show the progression from game is not over to game is over in a single test.

In a rare flash of inspiration, I realize that the game is over when the last frame requires no more balls...


    public boolean isGameOver() {
      return ! getLastFrame().needsMoreBalls();
    }

...which allows me to eliminate a field, ballCount, and a constant, BALLS_PER_FRAME.

Big Steps and Littl'uns

Let's take care of displaying the extra balls in a strike frame...


  @Test
  public void shouldNotShowExtraBallsInStrikeFrame(){
    frame.addBall(10);
    frame.addBall(4);

    assertThat(frame.getFirstBall(), is("X"));
    assertThat(frame.getSecondBall(), is(""));
  }

  public String getSecondBall() {
    if(isStrike()){
      return "";
    }
    return formatBall(balls[1]);
  }

...not forgetting to special case the last frame. I'll add this test to LastFrameTest so I can test the game and frame in a single shot.


  @Test
  public void shouldShowTheExtraBallsIfLastFrameIsStrike() {
    game.bowl(10);
    game.bowl(2);
    game.bowl(3);

    assertThat(lastFrame.getFirstBall(), is("X"));
    assertThat(lastFrame.getSecondBall(), is("2"));
    assertThat(lastFrame.getThirdBall(), is("3"));
  }

Here are the steps to pass it. First, I assume a flag to indicate the last frame...


  public Frame(Frame previousFrame) {
    this.previousFrame = previousFrame;
    this.lastFrame = false;
  }

  public String getSecondBall() {
    if(isStrike() && ! lastFrame){
      return "";
    }
    return formatBall(balls[1]);
  }

...then I Introduce Parameter with a default value of false...


  public Frame(Frame previousFrame, boolean lastFrame) {
    this.previousFrame = previousFrame;
    this.lastFrame = lastFrame;
  }

...and modify Game to pass in the correct value...

    
    public Game() {
      frames = new ArrayList<Frame>();

      Frame frame = null;
      for(int i = 0; i < FRAMES_PER_GAME; i++) {
        frame = new Frame(frame, i == FRAMES_PER_GAME - 1);
        frames.add(frame);
      }
    }

Now I can implement Frame.getThirdBall()...

    
    public String getThirdBall() {
      return formatBall(balls[2]);
    }

...to make the test pass. That seemed like a big step but the only bit I really short-circuited was testing the Game and Frame in one shot. I could have tested the Frame part separately, but then I would have written almost the exact same test twice.

That was a very long session for one little, three-sentence story though so I'll leave the acceptance tests for my next post.

Posted by Kevin Lawrence at March 20, 2007 02:37 PM

Trackback Pings

TrackBack URL for this entry: