I was wondering what would happen if I injected a Frame object into the bowling example, early on. What happened is that I got in a bit over my head. Come along and point and laugh.

Frame

In the bowling example as I usually do it, even within all the variations I do, I usually wind up with a rather procedural solution. This doesn’t trouble me much, since the procedural part is about 10 lines and will surely be much larger when converted to a more object-oriented solution. And I know that I would move to more objects if the forces got stronger. But I was wondering what would happen if, as soon as I know I need the frame idea, I put a Frame object in. So let’s find out.

Ready to Go

I built the first three tests this way:

  [TestFixture] public class BowlingTest: Assertion {
    Game game = new Game();

    [SetUp] public void SetUp() {
    }

    [Test] public void HookUp() {
      Assert(true);
    }

    [Test] public void Gutters() {
      RollMany(20,0);
      AssertEquals(0, game.Score());
    }

    [Test] public void Opens() {
      for ( int frame = 0; frame < 10; frame++ ) {
        game.Roll(5);
        game.Roll(4);
      }
      AssertEquals(90, game.Score());
    }

    private void RollMany(int count, int pins) {
      for ( int i = 0; i < count; i++ ) {
        game.Roll(pins);
      }
    }

  }

And the supporting code:

public class Game {
    ArrayList pins = new ArrayList();

    public void Roll(int pinCount) {
      pins.Add(pinCount);
    }

    public int Score() {
      int score = 0;
      foreach ( int pin in pins ) {
        score += pin;
      }
      return score;
    }
  }

(Often in demos, I first sum the pins in the Roll method and only later go to the pins ArrayList. This time I put the list in. I refactor to that list before the next test runs anyway, so we should be OK.

    [Test] public void Spare() {
      game.Roll(6);
      game.Roll(4);
      game.Roll(3);
      game.Roll(2);
      RollMany(16,0);
      AssertEquals(18, game.Score());
    }

Spares Introduce Frame

The need for the Frame occurs when we do the Spare test, because it needs to look at the first roll of the next frame. In the procedural form, we often do this by just indexing through the pins list by twos. Here we’ll introduce a Frame object. First the test:

    [Test] public void Spare() {
      game.Roll(6);
      game.Roll(4);
      game.Roll(3);
      game.Roll(2);
      RollMany(16,0);
      AssertEquals(18, game.Score());
    }

As planned, the test doesn’t run. “Hmmm, when we score spares, we need to go frame by frame. Let’s create a Frame object that handles that situation:

    public int Score() {
      int score = 0;
      Frame frame = new Frame(pins);
      while (frame.HasNext()) {
        score += frame.FrameScore();
      }
      return score;
    }

I just programmed up some intention. Make a Frame, and while it has something to give us, add the FrameScore into the full score. Now we have to make it work. Right now it won’t compile, and none of our tests will run. This is scary. But it should only be a few lines of code:

  public class Frame {
    ArrayList pins;
    int frameStart;

    public Frame( ArrayList pins) {
      this.pins = pins;
      frameStart = 0;
    }

    public bool HasNext() {
      return frameStart < pins.Count;
    }

    public int FrameScore() {
      int result = Pins(frameStart) + Pins(frameStart + 1);
      frameStart += 2;
      return result;
    }

    private int Pins(int rollNumber) {
      return (int) pins[rollNumber];
    }
  }

Now that’s a pretty big bite. We’ll have to think about that. But the tests run again (except for Spare, of course). Now to make Spare run, we should be able to do a quick mod to Frame:

    public int FrameScore() {
      int result;
      if (Spare()) {
        result = 10 + Pins(frameStart+2);
        frameStart += 2;
      }
      else {
        result = Pins(frameStart) + Pins(frameStart + 1);
        frameStart += 2;
      }
      return result;
    }

    private bool Spare() {
      return Pins(frameStart) + Pins(frameStart+1) == 10;
    }

Now I was nervous again writing this, for some reason, and it turns out to be for good reason. Expected 18 but was … 108!!! What’s up with that?? I sure don’t see what’s wrong with that. Should I delete the code and write it again, or debug it? I’m going to print some output.

Wow. 109 is 90 + 18. The pins are accumulating: the tests are interacting. Someone is holding on to something. Where?? Ha! Notice that I initialize Game game in the TestFixture as part of the declaration. NUnit doesn’t create a new instance of Game unless I put it in SetUp:

    [SetUp] public void SetUp() {
      game = new Game();
    }

That makes my test run. The code is good. Whew!

One thing that I’m not liking is that the if statements in the Frame are all going to need to update frameStart, and that the result is then returned outside the ifs. I had hoped that it would be a bit cleaner inside. Let’s put in the Strike, and then look again:

    [Test] public void Strike() {
      game.Roll(10);
      game.Roll(5);
      game.Roll(3);
      game.Roll(4);
      game.Roll(5);
      RollMany(14, 0);
      AssertEquals(35, game.Score());
    }

With the resulting code:

    public int FrameScore() {
      int result;
      if (Strike()) {
        result = 10 + Pins(frameStart+1) + Pins(frameStart+2);
        frameStart++;
      }
      else if (Spare()) {
        result = 10 + Pins(frameStart+2);
        frameStart += 2;
      }
      else {
        result = Pins(frameStart) + Pins(frameStart + 1);
        frameStart += 2;
      }
      return result;
    }

    private bool Strike() {
      return Pins(frameStart) == 10;
    }

This works after I fix my obligatory error of leaving the “else” off the if(Spare()). I need to reflect on a way to avoid this error.

Anyway, I’m really on about the complexity of the code there. What about this:

    public int FrameScore() {
      int result;
      if (Strike()) {
        result = 10 + Pins(frameStart+1) + Pins(frameStart+2);
      }
      else if (Spare()) {
        result = 10 + Pins(frameStart+2);
      }
      else {
        result = Pins(frameStart) + Pins(frameStart + 1);
      }
      IncrementFrame();
      return result;
    }

    private void IncrementFrame() {
      if (Strike())
        frameStart++;
      else
        frameStart += 2;
    }

That looks promising. What about extracting that if nest now:

    public int FrameScore() {
      int result = FrameValue();
      IncrementFrame();
      return result;
    }

    private int FrameValue() {
      int result;
      if (Strike()) {
        result = 10 + Pins(frameStart+1) + Pins(frameStart+2);
      }
      else if (Spare()) {
        result = 10 + Pins(frameStart+2);
      }
      else {
        result = Pins(frameStart) + Pins(frameStart + 1);
      }
      return result;
    }

Which we simplify to:

    private int FrameValue() {
      if (Strike()) {
        return 10 + Pins(frameStart+1) + Pins(frameStart+2);
      }
      else if (Spare()) {
        return 10 + Pins(frameStart+2);
      }
      else {
        return Pins(frameStart) + Pins(frameStart + 1);
      }
    }

Now I can’t resist trying my favorite trick, observing how those constant “10” values are made up:

    private int FrameValue() {
      if (Strike()) {
        return Pins(frameStart) + Pins(frameStart+1) + Pins(frameStart+2);
      }
      else if (Spare()) {
        return Pins(frameStart) + Pins(frameStart+1) + Pins(frameStart+2);
      }
      else {
        return Pins(frameStart) + Pins(frameStart + 1);
      }
    }

We could extract some duplication …

    private int FrameValue() {
      int result = Pins(frameStart) + Pins(frameStart + 1);
      if (Strike()) {
        result += Pins(frameStart+2);
      }
      else if (Spare()) {
        result += Pins(frameStart+2);
      }
      return result;
    }

And thence to:

    private int FrameValue() {
      int result = Pins(frameStart) + Pins(frameStart + 1);
      if (Strike() || Spare()) {
        result += Pins(frameStart+2);
      }
      return result;
    }

I would say that three out of four conscientious programmers hate this last formulation, and are pretty concerned about the previous one. The reason is that as soon as we start removing those values of 10, we have code that no longer expresses the rules of the game, but instead some mathematical equivalent to the rules. It’s equivalent, but it’s not clear!

Worse Yet, It Doesn't Work!

Had you noticed that I started refactoring the Frame object based on the tests in place, and that the system wasn’t fully tested yet? Well, in all the excitement, I hadn’t noticed either. So as my last act last night, I wrote the PerfectGame test:

    [Test] public void PerfectGame() {
      for ( int i = 0; i < 12; i++ ) {
        game.Roll(10);
      }
      AssertEquals(300, game.Score());
    }

It doesn’t run! I started with the assumption that my refactorings hadn’t worked, so I started backing them out. And it didn’t run, and it didn’t run. I’m all the way back to here now:

  public class Frame {
    ArrayList pins;
    int frameStart;

    public Frame( ArrayList pins) {
      this.pins = pins;
      frameStart = 0;
    }

    public bool HasNext() {
      return frameStart < pins.Count;
    }

    public int FrameScore() {
      int result = FrameValue();
      IncrementFrame();
      return result;
    }

    private int FrameValue() {
      if (Strike()) {
        return 10 + Pins(frameStart+1) + Pins(frameStart+2);
      } else if (Spare()) {
        return 10 + Pins(frameStart+2);
      } else
        return Pins(frameStart) + Pins(frameStart + 1);
    }

    private void IncrementFrame() {
      if (Strike())
        frameStart++;
      else
        frameStart += 2;
    }

    private bool Strike() {
      return Pins(frameStart) == 10;
    }

    private bool Spare() {
      return Pins(frameStart) + Pins(frameStart+1) == 10;
    }

    private int Pins(int rollNumber) {
      return (int) pins[rollNumber];
    }
  }

and it still doesn’t run. Finally I saw the truth. HasNext() doesn’t work: it will run off the end of the list. It’s not counting Frames, it’s counting pins, so bonus pins look like frames to it. The entire idea of how Frame works is just wrong! I was so depressed that I erased this article and went to bed.

Finally, sometime today, I decided that my “warts and all” theory says that I have to ‘fess up. So I resurrected the article from the trash, and here it is. Now the truth is, it’s probably easy enough to fix. Certainly I can just make the Frame object count the frames instead of stopping on the size of the rolls list. I’ll try that:

  public class Frame {
    ArrayList pins;
    int frameNumber;
    int frameStart;

    public Frame( ArrayList pins) {
      this.pins = pins;
      frameNumber = 0;
      frameStart = 0;
    }

    public bool HasNext() {
      return frameNumber < 10;
    }

    ...
    private void IncrementFrame() {
      if (Strike())
        frameStart++;
      else
        frameStart += 2;
      frameNumber++;
    } ...

OK, now that’s not so bad, really. But I was thinking of the Frame object as a kind of a “PinsReader”, with an EOF condition, and that vision was just not going to work. So when I was kicked in the head by my sweet little theory blowing up, it was enough to stop me. I’m glad I came back, things might not be as bad as I thought. But there’s something to think about:

Reflection

I had been thinking about this nice little Frame / Reader object for a while. It never seemed easy to extract it from the procedural versions, but it seemed like it would be a very neat implementation. So in this exercise, I took the chance to put it in. And my love for the neatness of the idea blinded me to the fact that it didn’t work, and I put in a lot of time, which I’ve spared you here, trying to look in the wrong place to find the problem. I wanted my clever refactoring of the FrameValue method to have introduced some subtle bug. Wrong.

Was I wrong to introduce Frame when I did? Well, it had this flaw and it caused me to waste some time, so maybe it was wrong. On the other hand, the defect wasn’t very big and it was easy to fix, so maybe it was OK. What I see is that instead of refactoring bit by bit, I took the opportunity to put in a favorite design idea more or less all at once. I didn’t build Frame with TDD, I just used my existing tests to check it, and they weren’t robust enough to make it work. So what I’m learning, yet again, is:

  • I shouldn't fall too easily in love with a neat idea. I'll do better to work toward it rather than put the whole thing in at once.
  • I shouldn't go around writing a whole class without tests.
  • Think whether my existing tests are robust enough before doing anything fancy.

Now these may not be the right lessons. They’re consistent with what I teach and what I believe. So they confirm what I think I already know. I’m comfortable with them. But I could be missing something equally or more important. The good news is that if I am, it will surely come back and bite me again, giving me another chance to learn it.

My tests are working now. I’ll write the Alternating one, then take a rest and come back for some code cleanup if it works, and to make it work if it doesn’t. I think it will though:

    [Test] public void Alternating() {
      for ( int twoFrames = 0; twoFrames < 5; twoFrames++ ) {
        game.Roll(10);
        game.Roll(9);
        game.Roll(1);
      }
      game.Roll(10);
      AssertEquals(200, game.Score());
    }

Good news! Alternating works. I think the code is good, and complete. Now I’ll take that rest. See you soon!