In which our heros find a straightforward way to replace conditional logic in the Frame with series of Kicker objects. The design so far is quite "object oriented", and is turning out as was envisioned. But is it better? That remains to be seen.

A New Day Dawns

We’re at Amer’s Deli in Brighton, and were just talking about what to do next. Reviewing the tests and code, we see that it’s time to work on spares, if we proceed as we usually do. We note that we can probably do much of our future work in the Frame and Frame tests, since all the scoring and decision making is in there. Bill Wake suggested something similar in an email last night, so perhaps he gets credit for the idea. Discussing whether to refactor the Kicker in now, or to build the code up procedurally and “discover” the need for the kicker, we’ve decided that the latter is the way to go. Here are the Frame tests now:

using System;
using NUnit.Framework;

namespace BowlingForKickers {
  [TestFixture] public class FrameTestFixture {

    public FrameTestFixture() {
    }

    [SetUp] public void SetUp() {
    }

    [Test] public void OpenFrame() {
      Frame frame = new Frame();
      Assert.IsTrue(frame.RollConsumed(3));
      Assert.IsTrue(frame.RollConsumed(4));
      Assert.IsFalse(frame.RollConsumed(2));
      Assert.AreEqual(7, frame.Score());
    }
  }
}

We’ll do a spare test, and move the creation of the Frame instance to SetUp. A big bite, but I think we’re up to it.

    [Test] public void SpareFrame() {
      Assert.IsTrue(frame.RollConsumed(6));
      Assert.IsTrue(frame.RollConsumed(4));
      Assert.IsFalse(frame.RollConsumed(2));
      Assert.AreEqual(12, frame.Score());
    }

We expect that the first three asserts will pass, meaning that the ball passing is correct, but that the score will be wrong. We expect 10, not 12. Let’s see.

Wrong. One of the IsTrue/IsFalses failed. We’d better put comments on those:

    [Test] public void SpareFrame() {
      Assert.IsTrue(frame.RollConsumed(6), "Done too soon, roll 1");
      Assert.IsTrue(frame.RollConsumed(4), "Done too soon, roll 2");
      Assert.IsFalse(frame.RollConsumed(2), "Not done soon enough, roll 3");
      Assert.AreEqual(12, frame.Score());
    }

This tells us that it’s failing on the very first roll. The bug was that, despite my great confidence, we screwed up the moving of the Frame to SetUp. Instead of pasting the creation into the SetUp method, I put it in the constructor. Since NUnit doesn’t create a new instance on every run – are you guys ever going to fix that bug? How about at least an option on TestFixture?? – we have to be sure it’s in SetUp. Now we’re getting the error we expect, 10 instead of 12.

We need to improve Frame to absorb the value of a third ball, while returning False, to pass it on as well. Here’s the implementation:

     public bool RollConsumed(int pins) {
      if (IsSpare()) {
        rolls.Add(pins);
        return false;
      }
      if (rolls.Count >= 2) 
        return false;
        rolls.Add(pins);
      return true;
      }

    private bool IsSpare() {
      return rolls.Count == 2 && Score() == 10;
    }

Nothing special here, but it’s getting ugly, a good reason to refactor to an object. Note the check of rolls.Count in IsSpare. I realized that if I didn’t do that, a Spare Frame would keep reading rolls until it got a non-zero one, thus using the next non-zero roll as its bonus instead of the next roll.

Now then: am I allowed to write that code, since I don’t have a failing test for it? You could argue that I can’t write it without a test. On the other hand, I’m supposed to write “the simplest code that could possibly work”. I can see that without the rolls.Count check, the code can’t possibly work. So I think it’s a righteous implementation. However, now that you’ve brought it up, we’ll write the test, see that it works, then remove that code to see if it breaks. Hmm.

    [Test] public void SpareFrameWithZeroBonus() {
      Assert.IsTrue(frame.RollConsumed(6));
      Assert.IsTrue(frame.RollConsumed(4));
      Assert.IsFalse(frame.RollConsumed(0));
      Assert.IsFalse(frame.RollConsumed(2));
      Assert.AreEqual(10, frame.Score());
    }

Here, the Frame should score ten, because the fool bowler missed all the pins on his bonus roll. But without the Count check, it’ll get twelve instead. We verify both cases.

It’s worth noting this: if I had noticed the need for the code and left it out, we could have written the test, seen it red, and fixed it by putting in the code. Because we chose to verify, we wrote the code, took it out, and put it back in. Putting the check in on spec was more work, given that I was going to really test that it was necessary. Lesson learned? You decide.

Improving the Code

We’re on a green bar, and the Frame code is creating a near vacuum here in Amers, being as it is so bad. Take a look:

using System;
using System.Collections;

namespace BowlingForKickers {
    public class Frame {
      ArrayList rolls;
        public Frame() {
      rolls = new ArrayList();
        }

      public bool RollConsumed(int pins) {
      if (IsSpare()) {
        rolls.Add(pins);
        return false;
      }
      if (rolls.Count >= 2) 
        return false;
        rolls.Add(pins);
      return true;
      }

    private bool IsSpare() {
      return rolls.Count == 2 && Score() == 10;
    }

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

That highlighted bit is really awful. The code needs to be refactored somehow, and our idea is just the thing for it: the Kicker. We envision that it’s some kind of Strategy object. Let’s see what we can figure out.

We began by just expressing our intention, which is that the Frame has-a Kicker, and creating a Kicker class and moving the Frame’s RollConsumed into it. The result so far is the following, and the tests still run.

using System;
using System.Collections;

namespace BowlingForKickers {
  public class Frame {
    internal ArrayList rolls;
    Kicker kicker;

    public Frame() {
      rolls = new ArrayList();
      kicker = new Kicker();
    }

    public bool RollConsumed(int pins) {
      return kicker.RollConsumed(this, pins);
    }

    internal bool IsSpare() {
      return rolls.Count == 2 && Score() == 10;
    }

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

using System;

namespace BowlingForKickers {
  public class Kicker {
    public Kicker() {
    }

    public bool RollConsumed(Frame frame, int pins) {
      if (frame.IsSpare()) {
        frame.rolls.Add(pins);
        return false;
      }
      if (frame.rolls.Count >= 2) 
        return false;
      frame.rolls.Add(pins);
      return true;
    }
  }
}

This passes the tests. It has removed all the ugly code to another class, which isn’t exactly an improvement. Time to break up the behavior into unique Kicker objects. We need an interface, IKicker.

namespace BowlingForKickers {
  public interface IKicker {
    bool RollConsumed(Frame frame, int pins);
  }
}

And we need to refer to it, and to make our Kicker implement it:

  public class Frame {
    internal ArrayList rolls;
    IKicker kicker;

    public Frame() {
      rolls = new ArrayList();
      kicker = new Kicker();
    }
...
  public class Kicker : IKicker {
...

OK, we’re good. Now what? Something about having two kinds of kickers (so far), a Spare kicker and a regular open Frame kicker.

We’re debating which way to go. I want to have the Kicker call back to the frame, telling it to change the Kicker if the Kicker decides we need a new one. Chet is leaning toward having the Frame save the result of the RollConsumed, and then ask the Kicker for a new Kicker. I’m whining that that puts three or four lines of code in the Frame where now we have just one. Does it matter? Should we just try one, or try both? Chet’s worried about the fact that the Kicker will change in the Frame, just when he least expects it. We’ll go ahead and see what it looks like.

I’m inclined to proceed by creating an OpenFrame kicker that doesn’t work for Spares, and then fix the bug by making that simpler Kicker replace itself. Chet has a feeling we should do something different, but it’s not clear what.

As part of the discussion, we drew the following little state diagram. There will be five states (Kickers) when we’re done. The FirstRollKicker records a roll and transitions either to a TwoBonusKicker (if it’s a Strike) or to a SecondRollKicker. The SecondRollKicker records a roll and transitions either to an IgnoreKicker or to a OneBonusKicker. The TwoBonusKicker records a roll and transitions to OneBonus; the OneBonus records a roll and goes to Ignore.

image

I was concerned that the five minutes we spent talking were Big Design Up Front, but Chet suggests that maybe it was Big Analysis Up Front instead. Maybe that’s more OK. Either way, we really don’t think there’s anything wrong with thinking a bit before we work. I’m inclined to rename the current Kicker to FirstRollKicker, take out all its code, and let everything break, producing a red bar from which we can code. When all the tests are green, we’ll move on to handle strikes and whatever comes next. Chet’s not entirely comfortable here, but I am. We decide to forge ahead, because it’s my computer.

Rename Kicker to FirstRollKicker, remove all logic, and let it break:

using System;

namespace BowlingForKickers {
  public class FirstRollKicker : IKicker {
    public FirstRollKicker() {
    }

    public bool RollConsumed(Frame frame, int pins) {
      return true;
    }
  }
}

Let’s see what the tests do … GutterBalls works – zero is zero is zero – but everything else breaks. The easiest one to fix is probably OpenFrame in FrameTestFixture, which will probably fix the OpenFrames test in BowlingTestFixture as well. The test is:

    [Test] public void OpenFrame() {
      Assert.IsTrue(frame.RollConsumed(3));
      Assert.IsTrue(frame.RollConsumed(4));
      Assert.IsFalse(frame.RollConsumed(2));
      Assert.AreEqual(7, frame.Score());
    }

I predict that we can’t make that work in one step: we’ll need the SecondBallKicker and the IgnoreKicker. That’s OK with me, but it’s a bit of a big bite. Let’s see if I get in trouble. First part of the implementation to get the bar green is this:

    public bool RollConsumed(Frame frame, int pins) {
      frame.rolls.Add(pins);
      frame.SetKicker(new SecondRollKicker());
      return true;
    }

That requires us to write SecondRollKicker, and the SetKicker method. We thought about just doing the Add and not resetting the kicker, but there’s no real point to it.

using System;

namespace BowlingForKickers {
    public class SecondRollKicker : IKicker {
        public SecondRollKicker() {
        }

      public bool RollConsumed(Frame frame, int pins) {
        frame.rolls.Add(pins);
        return true;
      }
    }
}

… with, in Frame …

    public void SetKicker(IKicker aKicker) {
      kicker = aKicker;
    }

After deciding the SecondRollKicker should return true, as shown above, instead of false, which we had on the diagram but typed incorrectly, the OpenFrames test passes in the Bowling fixture, but all the Frame tests wtill fail. We need to do the IgnoreKicker. At least that’s my theory. I’m still on plan, but if that doesn’t work, we may need to reboot.

  public class SecondRollKicker : IKicker {
    public SecondRollKicker() {
    }

    public bool RollConsumed(Frame frame, int pins) {
      frame.rolls.Add(pins);
      frame.SetKicker(new IgnoreKicker());
      return true;
    }
  }

    public class IgnoreKicker: IKicker {
    public IgnoreKicker() {
    }

    public bool RollConsumed(Frame frame, int pins) {
      return false;
    }
  }

Now my plan is that this should make something work. Let’s see.

Makes the OpenFrame test work, as planned. Also makes the SpareFrameWithZeroBonus work, but that’s an artifact of the bonus value being zero. We’re treating the spares as opens, and that test’s spare doesn’t get any bonus, so it just happens to work.

Chet has left, but I have permission to make all the tests green and do some cleanup. To make the tests run, I’ll have to have the SecondRollKicker recognize a spare and conditionally produce a OneBonusKicker. To recognize the spare, the SecondRollKicker needs to know the frame score so far. We could have him ask the frame, and I’ll build it that way. But that’s not a good situation, in my opinion, and Chet and I have already talke about how to fix it, namely by creating the SecondRollKicker to know what the first roll was. We’ll refactor that change in later. For now, make it work:

    public bool RollConsumed(Frame frame, int pins) {
      frame.rolls.Add(pins);
      if (frame.IsSpare())
        frame.SetKicker(new OneBonusKicker());
      else
        frame.SetKicker(new IgnoreKicker());
      return true;
    }

This, of course, requires a new Kicker that just reads one roll and then goes to Ignore:

  public class OneBonusKicker: IKicker {
    public OneBonusKicker() {
    }

    public bool RollConsumed(Frame frame, int pins) {
      frame.rolls.Add(pins);
      frame.SetKicker(new IgnoreKicker());
      return false;
    }
  }

I kind of expect this to work. Let’s see. It does! The tests are green, and my plan is vindicated: it worked and it wasn’t too big a bite because there weren’t any surprises. I’ll do some observations, maybe some refactoring, then show you the current code at the bottom.

Observations So Far

The return of true and false is confusing. They should be given meaningful names, either by defining some constants or maybe an enum. Must think about that, probably tomorrow.

I don’t like having to get rolls from the Frame and then adding to it. We should make a method on Frame, AddRoll(), to do that. I’ll make that change before displaying all the code. If I make rolls private, that should highlight all the places to change, so it should be pretty easy. And it was: tests all green again.

Also, I don’t like asking the Frame if it’s a Spare. To improve that, I need to create the SecondRollKicker with knowledge of the first roll, then check the value of the second to see if the total is ten. Looks like this:

  public class FirstRollKicker : IKicker {
    public FirstRollKicker() {
    }

    public bool RollConsumed(Frame frame, int pins) {
      frame.AddRoll(pins);
      frame.SetKicker(new SecondRollKicker(pins));
      return true;
    }
  }

  public class SecondRollKicker : IKicker {
    int firstRoll;
    public SecondRollKicker(int pins) {
      firstRoll = pins;
    }

    public bool RollConsumed(Frame frame, int secondRoll) {
      frame.AddRoll(secondRoll);
      if (firstRoll + secondRoll == 10)
        frame.SetKicker(new OneBonusKicker());
      else
        frame.SetKicker(new IgnoreKicker());
      return true;
    }
  }

Now I can remove the IsSpare method from Frame. That’s all good.

General Reflection

So far, while I think this is a really cool implementation, and I’m proud of it for that reason, it’s important to notice that we have six classes and an interface, and we aren’t done yet. There will be at least one more Kicker class, the TwoBonusKicker. Each of the classes is very simple, and most of the Kickers have no conditionals in them. All very neat.

But … the object-oriented implementation is about 220 lines long, while the procedural one was about 50 or 60. Almost four times as much code to read and understand, and so far with no additional functionality. Now it may be that if we do separate frame scoring, or other fancy stuff, that we’ll be happy that we have all this stuff. Right now, my take is that it’s technically neat but that it isn’t paying off.

More on that tomorrow, when we wrap up. For now, here’s where we stand:

Today's Code

using System;
using NUnit.Framework;

namespace BowlingForKickers {
  [TestFixture] public class BowlingTestFixture {
    Game game;

    public BowlingTestFixture() {
    }

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

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

    public void RollAll(int[] rolls) {
      foreach ( int pins in rolls ) {
        game.Roll(pins);
      }
    }

    [Test] public void GutterBalls() {
      RollMany(20,0);
      Assert.AreEqual(0, game.Score());
    }

    [Test] public void OpenFrames() {
      RollAll( new int[] {1,2, 3,4, 1,2, 3,4, 1,2, 3,4, 1,2, 3,4, 1,2, 3,4 });
      Assert.AreEqual(50, game.Score());
    }

  }
}

using System;
using NUnit.Framework;

namespace BowlingForKickers {
  [TestFixture] public class FrameTestFixture {
    Frame frame;

    public FrameTestFixture() {
    }

    [SetUp] public void SetUp() {
      frame = new Frame();
    }

    [Test] public void OpenFrame() {
      Assert.IsTrue(frame.RollConsumed(3));
      Assert.IsTrue(frame.RollConsumed(4));
      Assert.IsFalse(frame.RollConsumed(2));
      Assert.AreEqual(7, frame.Score());
    }

    [Test] public void SpareFrame() {
      Assert.IsTrue(frame.RollConsumed(6), "Done too soon, roll 1");
      Assert.IsTrue(frame.RollConsumed(4), "Done too soon, roll 2");
      Assert.IsFalse(frame.RollConsumed(2), "Not done soon enough, roll 3");
      Assert.AreEqual(12, frame.Score());
    }

    [Test] public void SpareFrameWithZeroBonus() {
      Assert.IsTrue(frame.RollConsumed(6));
      Assert.IsTrue(frame.RollConsumed(4));
      Assert.IsFalse(frame.RollConsumed(0));
      Assert.IsFalse(frame.RollConsumed(2));
      Assert.AreEqual(10, frame.Score());
    }

  }
}

using System;

namespace BowlingForKickers {
  public class Game {
    Frame[] frames;

    public Game() {
      frames = new Frame[10];
      for ( int i = 0; i < 10; i++ ) {
        frames[i] = new Frame();
      }
    }

    public void Roll(int pins) {
      int frameCounter = 0;
      while ( !frames[frameCounter].RollConsumed(pins))
        frameCounter++;
    }

    public int Score() {
      int score = 0;
      foreach ( Frame frame in frames ) {
        score += frame.Score();
      }
      return score;
    }
  }
}

using System;
using System.Collections;

namespace BowlingForKickers {
  public class Frame {
    private ArrayList rolls;
    IKicker kicker;

    public Frame() {
      rolls = new ArrayList();
      kicker = new FirstRollKicker();
    }

    public void SetKicker(IKicker aKicker) {
      kicker = aKicker;
    }

    public void AddRoll(int pins) {
      rolls.Add(pins);
    }

    public bool RollConsumed(int pins) {
      return kicker.RollConsumed(this, pins);
    }

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

namespace BowlingForKickers {
  public interface IKicker {
    bool RollConsumed(Frame frame, int pins);
  }
}

using System;

namespace BowlingForKickers {
  public class FirstRollKicker : IKicker {
    public FirstRollKicker() {
    }

    public bool RollConsumed(Frame frame, int pins) {
      frame.AddRoll(pins);
      frame.SetKicker(new SecondRollKicker(pins));
      return true;
    }
  }
}

using System;

namespace BowlingForKickers {
  public class SecondRollKicker : IKicker {
    int firstRoll;
    public SecondRollKicker(int pins) {
      firstRoll = pins;
    }

    public bool RollConsumed(Frame frame, int secondRoll) {
      frame.AddRoll(secondRoll);
      if (firstRoll + secondRoll == 10)
        frame.SetKicker(new OneBonusKicker());
      else
        frame.SetKicker(new IgnoreKicker());
      return true;
    }
  }
}

using System;

namespace BowlingForKickers {
  public class IgnoreKicker: IKicker {
    public IgnoreKicker() {
    }

    public bool RollConsumed(Frame frame, int pins) {
      return false;
    }
  }
}

using System;

namespace BowlingForKickers {
  public class OneBonusKicker: IKicker {
    public OneBonusKicker() {
    }

    public bool RollConsumed(Frame frame, int pins) {
      frame.AddRoll(pins);
      frame.SetKicker(new IgnoreKicker());
      return false;
    }
  }
}