Let's proceed with making the Haskell-inspired Java version "better", focusing on writing tests for a new Frame object. The code gets odd, but turns out better after a while ...

Single Frame Scoring

As discussed in the previous article, the Frame object doesn’t seem quite right. Our mission, should we decide to accept it, is to clean it up. Here goes. The Frame class looks like this:

public class Frame {
    Integer frameScore;

    public Integer score() {
        return frameScore;
    }

    public int scoreRolls(int runningScore, List<Integer> rolls) {
        frameScore = runningScore + rolls.get(0) + rolls.get(1);;
        return frameScore;
    }

    public List<Integer> remainingRolls(List<Integer> rolls) {
        return rolls.subList(2, rolls.size());
    }
}

We’ll remove the running score, and let them just compute their own frame score. That will break the tests, and we’ll fix that.

Frame:
    public int scoreRolls(List<Integer> rolls) {
        frameScore = rolls.get(0) + rolls.get(1);;
        return frameScore;
    }

BowlingGame:    
    private void loadFrames(List<Integer> rolls) {
        for (Frame frame: frames) {
            frame.scoreRolls(rolls);
            rolls = frame.remainingRolls(rolls);
        }
    }

Nice. As expected the open frames test fails: it’s only returning the score of the last frame. For now, we’ll fix score():

   private Integer scoreFrames() {
        Integer score = 0;
        for (Frame frame : frames) {
            score += frame.score();
        }
        return score;
    }

Tests run. We could clean up loadFrames a bit, since now it doesn’t need to return the score and could return the rolls list from its scoreRolls() method, obviating the need for the remainingRolls call. That’s not really where we’re going, but before we change things, let’s get a bit more clean:

BowlingGame:
    private void loadFrames(List<Integer> rolls) {
        for (Frame frame: frames) {
            rolls = frame.scoreRolls(rolls);
        }
    }

Frame:
    public List<Integer> scoreRolls(List<Integer> rolls) {
        frameScore = rolls.get(0) + rolls.get(1);;
        return remainingRolls(rolls);
    }

    private List<Integer> remainingRolls(List<Integer> rolls) {
        return rolls.subList(2, rolls.size());
    }

That’s nicer. Now to drain the swamp. We want to change things so that the rolls are passed in one at a time, and the Frames tell us whether they’re hungry, and whether they have totally consumed the roll. I’m imagining code like this (not compiled or tested):

    boolean consumed;
    for(Frame frame: frames) {
      if (frame.isHungry()) consumed = frame.roll(roll);
      if (consumed) break;
    }

I’m not fond of the break, and I’ll try to find a better way. But that’s the basic idea: Frames know if they are hungry, and whether they have consumed the roll. (A Frame looking for a bonus roll is hungry, but doesn’t consume the roll: it just takes a lick. A frame looking for a real roll is hungry, and consumes the roll.)

Let’s TDD this new behavior on Frame separately, and then merge it in.

Incremental Frames

I’ll build a FrameTest class now, and start filling it in. First test:

public class FrameTest {
    Frame frame;

    @Before public void setUp() throws Exception {
        frame = new Frame();
    }

    @Test public void emptyIsHungry() throws Exception {
        assertTrue(frame.isHungry());
    }
}

With the “fake it till you make it” implementation in Frame:

   public boolean isHungry() {
        return true;
    }

I’m about to break a rule. Many folks will tell you that tests should be Arrange, Act, Assert, with only one assertion (or assertion batch) per test. I’m very tempted not to do that. Thanks for pairing with me … I’ll hold back and try to do just one test at a time. I’m glad I confessed.

   @Test public void singleRollIsHungry() throws Exception {
        frame.roll(5);
        assertTrue(frame.isHungry());
    }

This doesn’t compile now, and though it would run, it’s just because isHungry is fake. I’ll write a test that should fail before even running. Do you ever write two tests at a time? I don’t either, but here I need to bracket what works and what doesn’t.

   @Test public void openFrameNotHungry() throws Exception {
        frame.roll(5);
        frame.roll(4);
        assertFalse(frame.isHungry());
    }

That should guarantee a red bar. Now to build rolll() in Frame. It looks to me as if Frame will either need to keep a roll count and a running score, or maybe just to keep a little list of rolls. Let’s go that way: it seems simpler to me.

public class Frame {
    Integer frameScore;
    List<Integer> rolls;

    public Frame() {
        rolls = new ArrayList<Integer>();
    }

    ...

    public boolean isHungry() {
        return true;
    }    

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

This fails as planned: openFrameNotHungry is red. We need to improve isHungry:

   public boolean isHungry() {
        return rolls.size() < 2;
    }

Tests all run. We’ll need more, with strikes, spares, and of course the notion of consumed, but it’s time for lunch. Meet me at Zukey Lake Tavern – I’m buying.

Delicious ...

… although you don’t usually have to sign a release when you go somewhere for lunch. Seems they were filming a commercial and needed a release from anyone they might film. Watch for me.

Having consumed lunch, now we need to consume rolls. I’m tempted to enhance my existing tests, but your advice has worked well so far, so I’ll write a new one:

   @Test public void firstRollConsumed() throws Exception {
        assertTrue(frame.roll(4));
    }

Changed Frame.roll() to make that work:

   public boolean roll(Integer pins) {
        rolls.add(pins);
        return true;
    }

Now I need it to consume the second one – but it already will. And since then it won’t be hungry, I won’t send it any more rolls, according to proper use. It might be advisable to protect against being sent rolls when not hungry, but I’m a happy path kind of guy, so I’m not going there yet. In addition, I’m not entirely happen with the isHungry being exported. It seems like an efficiency hack to use it. It may be better to just send the roll to everyone until someone consumes it. We’ll hold that thought. For now, I think I’ll go after strikes and spares, keeping hungry and consumed status going right. First, spare:

   @Test public void spareWantsThree() throws Exception {
        frame.roll(8);
        frame.roll(2);
        assertTrue(frame.isHungry());
    }

Implementing …

   public boolean isHungry() {
        if (rolls.size() < 2) return true;
        if (spare() && rolls.size() < 3) return true;
        return false;
    }

    private boolean spare() {
        return !strike() 
          && rolls.size() == 2 
          && rolls.get(0)+rolls.get(1) == 10;
    }

    private boolean strike() {
        return rolls.get(0) == 10;
    }

That was a lot, and a bit tricky. I haven’t run the tests yet … but they’re good. Let’s give a spare three, make sure he doesn’t consume the last one, and that he isn’t hungry any more. While we’re at it, let’s get him to tell us his score:

   @Test public void spare() throws Exception {
        frame.roll(8);
        frame.roll(2);
        assertTrue(frame.isHungry());
        assertFalse(frame.roll(5));
        assertFalse(frame.isHungry());
        assertEquals(15, frame.frameScore());
    }

I coded frameScore … before running the tests. Bad Ron! Anyway …

   public Integer frameScore() {
        Integer score = 0;
        for (Integer roll: rolls)
            score += roll;
        return score;
    }

But the tests fail on the second isHungry() test. And we can see here that while it was easier to write that test than to do each assert more or less separately, it was a little harder to see what’s going on. Must beef up roll to return false appropriately:

   public boolean roll(Integer pins) {
        rolls.add(pins);
        return rolls.size() <= 2;
    }

That works – for now. Even though I felt I needed to define the strike condition to make the definition of spare be reasonable, we don’t have any strike situations, and this line will surely change. I could figure out what it will need to be but that’s not my TDD way. I need a test for that.

   @Test public void strike() throws Exception {
        frame.roll(10);
        assertTrue(frame.isHungry());
        assertFalse(frame.roll(4));
        assertTrue(frame.isHungry());
        assertFalse(frame.roll(5));
        assertEquals(19, frame.frameScore());
    }

This continual need to check isHungry is ticking me off. And it’s as if this test is testing a bunch of state information that I’d rather not know. A better design would be just to throw sequences of rolls at frames, making sure they respond with consumed, and checking the final answer.

That test fails, of course, on the highlighted line above. Why is the jUnit message “Assertion Error: null”? That seems odd. And again, it was hard to spot just where the error was … well, all I had to do was look at line 53, actually, but still. We need to fix the return from roll again:

   public boolean roll(Integer pins) {
        rolls.add(pins);
        if (strike()) return false;
        return rolls.size() <= 2;
    }

That gets us to the next line, isHungry(). Apparently a strike isn’t hungry after the second roll. What’s up with that? Ah. We only check for spare. We need to check for strike also:

   public boolean isHungry() {
        if (rolls.size() < 2) return true;
        if (mark() && rolls.size() < 3) return true;
        return false;
    }

    private boolean mark() {
        return strike() || spare();
    }

What Now?

I had planned, when I wrote the paragraph above, to get back to the program in a few hours, next morning at worst. It looks like the FrameTest view of the Frame is done. However …

I was chatting with Chet at the end of last week about how this exercise with the FrameTest feels. It feels mushy to me. It’s like driving in deep slush, where the car resists turning, resists straightening out, feeling sluggish and unresponsive. It’s like flying just above stalling speed, a sort of washed-out disconnected feeling.

I know that the Frame is doing exactly what I’ve told it to do, and my tests are more direct than they usually are, when I send them all through the Game. But I don’t know whether the Frame is really going to be what the Game wants. I hypothesize that people who write objects in anticipation of need find themselves in this situation all the time, and may be used to it. I’m not used to it, and my sense is that this vehicle isn’t responding to the controls as crisply as it should.

In retrospect, I “should” have plugged the Frame back into the Game before doing spare and strike. But I was on a “roll”. Sorry. In any case, it’s time to hook the two classes back together, using the new Frame features in the old Game.

First, a review of the code. Here, for your convenience, is all the code:

package bowlingWithFrames;

import static org.junit.Assert.*;

import java.util.ArrayList;
import java.util.List;

import org.junit.Before;
import org.junit.Test;

public class BWFTestCase {
    List<Integer> rolls;
    BowlingGame game;

    @Before public void setUp() throws Exception {
        rolls = new ArrayList<Integer>();
        game = new BowlingGame();
    }

    @Test public void gutterballs() throws Exception {
        roll(20,0);
        Integer score = game.score(rolls);
        assertEquals(0, score);
    }

    @Test public void opens() throws Exception {
        rollFrame(3,4);
        rollFrame(2,1);
        rollFrame(4,5);
        rollFrame(1,3);
        rollFrame(2,5);
        rollFrame(7,1);
        rollFrame(2,2);
        rollFrame(4,3);
        rollFrame(2,3);
        rollFrame(9,0);
        assertEquals(63, game.score(rolls));
    }

    private void rollFrame(int firstRoll, int secondRoll) {
        roll(firstRoll);
        roll(secondRoll);
    }

    private void roll(int pins) {
        rolls.add(pins);
    }

    private void roll(int count, int pins) {
        for(int roll = 0; roll < count; roll++)
            roll(pins);
    }
}

package bowlingWithFrames;

import static org.junit.Assert.*;
import org.junit.Before;
import org.junit.Test;

public class FrameTest {
    Frame frame;

    @Before public void setUp() throws Exception {
        frame = new Frame();
    }

    @Test public void emptyIsHungry() throws Exception {
        assertTrue(frame.isHungry());
    }

    @Test public void singleRollIsHungry() throws Exception {
        frame.roll(5);
        assertTrue(frame.isHungry());
    }

    @Test public void openFrameNotHungry() throws Exception {
        frame.roll(5);
        frame.roll(4);
        assertFalse(frame.isHungry());
    }

    @Test public void firstRollConsumed() throws Exception {
        assertTrue(frame.roll(4));
    }

    @Test public void spareWantsThree() throws Exception {
        frame.roll(8);
        frame.roll(2);
        assertTrue(frame.isHungry());
    }

    @Test public void spare() throws Exception {
        frame.roll(8);
        frame.roll(2);
        assertTrue(frame.isHungry());
        assertFalse(frame.roll(5));
        assertFalse(frame.isHungry());
        assertEquals(15, frame.frameScore());
    }

    @Test public void strike() throws Exception {
        frame.roll(10);
        assertTrue(frame.isHungry());
        assertFalse(frame.roll(4));
        assertTrue(frame.isHungry());
        assertFalse(frame.roll(5));
        assertEquals(19, frame.frameScore());
    }
}

package bowlingWithFrames;

import java.util.List;

public class BowlingGame {
    Frame[] frames;

    public Integer score(List<Integer> rolls) {
        createFrames();
        loadFrames(rolls);
        Integer score = scoreFrames();
        return score;
    }

    private void createFrames() {
        frames = new Frame[10];
        for(int frame = 0; frame < 10; frame++)
            frames[frame] = new Frame();
    }

    private void loadFrames(List<Integer> rolls) {
        for (Frame frame: frames) {
            rolls = frame.scoreRolls(rolls);
        }
    }

    private Integer scoreFrames() {
        Integer score = 0;
        for (Frame frame : frames) {
            score += frame.score();
        }
        return score;
    }
}

package bowlingWithFrames;

import java.util.ArrayList;
import java.util.List;

public class Frame {
    Integer frameScore;
    List<Integer> rolls;

    public Frame() {
        rolls = new ArrayList<Integer>();
    }

    public Integer score() {
        return frameScore;
    }

    public List<Integer> scoreRolls(List<Integer> rolls) {
        frameScore = rolls.get(0) + rolls.get(1);;
        return remainingRolls(rolls);
    }

    private List<Integer> remainingRolls(List<Integer> rolls) {
        return rolls.subList(2, rolls.size());
    }

    public boolean isHungry() {
        if (rolls.size() < 2) return true;
        if (mark() && rolls.size() < 3) return true;
        return false;
    }

    private boolean mark() {
        return strike() || spare();
    }

    private boolean spare() {
        return !strike() 
          && rolls.size() == 2 
          && rolls.get(0)+rolls.get(1) == 10;
    }

    private boolean strike() {
        return rolls.get(0) == 10;
    }

    public boolean roll(Integer pins) {
        rolls.add(pins);
        if (strike()) return false;
        return rolls.size() <= 2;
    }

    public Integer frameScore() {
        Integer score = 0;
        for (Integer roll: rolls)
            score += roll;
        return score;
    }
}

Now what’s happening here, in the highlighted lines, is that the Game calls score(), which returns the frameScore member variable, which is calculated on the fly in the scoreRolls() method. But the FrameTest uses the roll() method and the isHungry method, to pass the rolls to the Frame, and then calls the frameScore() method to get the result. I’d like to change the Game to use the roll() method and the frameScore() method. I sketched that idea back before I started, this way:

    boolean consumed;
    for(Frame frame: frames) {
      if (frame.isHungry()) consumed = frame.roll(roll);
      if (consumed) break;
    }

I still don’t think that code is pretty, but it looks like it would work. And the isHungry call looks like Inappropriate Intimacy to me as well. Let’s make it work, then make it right. I’ll try it as written, in the loadFrames() method of Game. It looks like this:

   private void loadFrames(List<Integer> rolls) {
        for (Frame frame: frames) {
            rolls = frame.scoreRolls(rolls);
        }
    }

And we’ll turn it into this:

   private void loadFrames(List<Integer> rolls) {
        for (Integer pins: rolls) {
            boolean consumed = false;
            for (Frame frame: frames) {
                if (frame.isHungry()) consumed = frame.roll(pins);
                if (consumed) break;
            }           
        }
    }

… in conjunction with this, to call the right scoring method:

   private Integer scoreFrames() {
        Integer score = 0;
        for (Frame frame : frames) {
            score += frame.frameScore();
        }
        return score;
    }

The tests actually run. Frankly, that concerns me: I’m not over my feeling that this is kind of soft. But if I remove the now redundant items from Frame, that will reassure me, and it’s the next step anyway. I should be able to get rid of the frameScore variable, and its usage – maybe even the whole method that uses it. Here goes:

public class Frame {
    List<Integer> rolls;

    public Frame() {
        rolls = new ArrayList<Integer>();
    }

    public boolean isHungry() {
        if (rolls.size() < 2) return true;
        if (mark() && rolls.size() < 3) return true;
        return false;
    }

    private boolean mark() {
        return strike() || spare();
    }

    private boolean spare() {
        return !strike() 
          && rolls.size() == 2 
          && rolls.get(0)+rolls.get(1) == 10;
    }

    private boolean strike() {
        return rolls.get(0) == 10;
    }

    public boolean roll(Integer pins) {
        rolls.add(pins);
        if (strike()) return false;
        return rolls.size() <= 2;
    }

    public Integer frameScore() {
        Integer score = 0;
        for (Integer roll: rolls)
            score += roll;
        return score;
    }
}

My tests still run. I must be smarter than I feared – or luckier. Let’s add some spare and strike tests to the Game’s tests:

   @Test public void spare() throws Exception {
        rollFrame(4,6);
        rollFrame(5,4);
        roll(16,0);
        assertEquals(24, game.score(rolls));
    }

    @Test public void strike() throws Exception {
        roll(10);
        rollFrame(5,4);
        rollFrame(4,5);
        roll(14,0);
        assertEquals(37, game.score(rolls));
    }

The spare test passes … but the strike test fails, with a score of 190! That makes me guess that some frame didn’t consume its roll, and everyone downstream ate it. But that’s just a guess: the test isn’t very diagnostic. I think I’ll write a more focused test in the FrameTest class, if I can think of one. The strike test makes me wonder:

   @Test public void strike() throws Exception {
        frame.roll(10);
        assertTrue(frame.isHungry());
        assertFalse(frame.roll(4));
        assertTrue(frame.isHungry());
        assertFalse(frame.roll(5));
        assertEquals(19, frame.frameScore());
    }

There’s no test for consumed in the first roll. Perhaps there should be. I’ll change that:

   @Test public void strike() throws Exception {
        assertTrue(frame.roll(10));
        assertTrue(frame.isHungry());
        assertFalse(frame.roll(4));
        assertTrue(frame.isHungry());
        assertFalse(frame.roll(5));
        assertEquals(19, frame.frameScore());
    }

Sure enough, that fails. Why doesn’t a strike roll return true (= consumed)? And isn’t that whole interface a bit confusing? I think so. But first to make it work.

   public boolean roll(Integer pins) {
        rolls.add(pins);
        if (strike()) return <nc>true</nc>;
        return rolls.size() <= 2;
    }

That used to say false, for some reason. Did I do that on purpose? No matter. The test now runs a bit further, but fails on line 51, the assertFalse, saying that the first bonus roll was consumed … but it shouldn’t have been. Ha. The logic above isn’t smart enough. If we have a strike, then the first roll is consumed … but subsequent ones are not. So we have to enhance that if(strike()) a bit. This is going to get uglier before it gets prettier, I’m afraid:

   public boolean roll(Integer pins) {
        rolls.add(pins);
        if (strike()) return rolls.size() == 1;
        return rolls.size() <= 2;
    }

Tests all run. If a frame is a strike, it consumes only the first ball. If not, it consumes the first and second roll, and no others. I’m finding this notion of returning “consumed” to be awkward. One objection might be that we have a side effect in this function, recording a roll and answering a question about it. But I actually like that part. What I don’t like is that the code isn’t clear. Let me put in my standard ending tests, and maybe the new ones about the end of the game, and then we’ll try to improve the code. These all run:

   @Test public void perfect() throws Exception {
        roll(12,10);
        assertEquals(300, game.score(rolls));
    }

    @Test public void alternateStrikeFirst() throws Exception {
        for (int i = 0; i < 5; i++ ) {
            roll(10);
            rollFrame(6,4);
        }
        roll(10);
        assertEquals(200, game.score(rolls));
    }

    @Test public void alternateSpareFirst() throws Exception {
        for (int i = 0; i < 5; i++ ) {
            rollFrame(6,4);
            roll(10);
        }
        rollFrame(6,4);
        assertEquals(200, game.score(rolls));
    }

Truth is, I’m convinced that things work now, and I don’t want to write the tests that Pit found. But that would be bad. If Chet were paying attention he’d tell me to write them. So I will.

   @Test public void strikeLastFrame() throws Exception {
        roll(18,0);
        roll(10);
        roll(2);
        roll(3);
        assertEquals(15, game.score(rolls));
    }

    @Test public void strikeNinthFrame() throws Exception {
        roll(16,0);
        roll(10);
        rollFrame(2,3);
        assertEquals( 20, game.score(rolls));
    }

    @Test public void spareNinthFrame() throws Exception {
        roll(16,0);
        rollFrame(0,10);
        rollFrame(2,3);
        assertEquals( 17, game.score(rolls));
    }

That last test was offered on the xp list by Donald Roby. I like it because it shows that there’s yet another way to end a game with 10, 2, 3, and that it gets yet a different score. I find that pretty, somehow.

In any case, we now have a very solid bowling game, with a Frame object, used in a fairly clear way. There are things not to like. We’ll do that next time, in Single Frame Cleanup.