We build some superclasses to remove the duplication from the Kicker classes. One false start sends us down an unpleasant path, and we decide to start over more cleverly. As for the end result, we have our doubts. The proof will be in the updating, later on. Right now, it's hard to see why five times bigger is better, just because it looks like Object-Oriented.

Discussing Our Plans

We have in mind, in this episode, to build a class hierarchy to remove duplication among the Kickers. Some discussion on the mailing lists suggests that the top level class should have only a template method, and not the most common versions of the class-specific behaviors. Kevlin Henney is being blamed as the source of this idea. We’re not sure just why it’s important that the top-level class shouldn’t contain any actually useful methods, but we suspect it may be some kind of framework-designer idea. I (Ron) have, in my past, favored completely abstract classes at the tops of my hierarchies, but in more recent years I find that I don’t stand on ceremony either way.

We would agree that if each subclass does something different, there’s no reason to have a real implementation in the superclass. But if there is duplication, we’re not sure why a superclass – even the top one – couldn’t be used to contain it. Anyway, we’ll look at it.

Our Kickers have three separate actions that they perform:

  1. Decide whether or not to add the roll into their Frame;
  2. Decide what Kicker comes next;
  3. Indicate whether the roll has been consumed, or should be passed on to the next Frame.

Check out the Kicker implementations, and meet us on the other side:

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

    public class FirstRollKicker : IKicker {

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

    private void SetKicker(Frame frame, int pins) {
      if ( IsStrike(pins) )
        frame.SetKicker(new TwoBonusKicker());
      else
        frame.SetKicker(new SecondRollKicker(pins));
    }

    private bool IsStrike(int pins) {
      return pins == 10;
    }
  }

    public class SecondRollKicker : IKicker {
    int firstRoll;

    public SecondRollKicker(int pins) {
      firstRoll = pins;
    }

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

    private void SetKicker(Frame frame, int secondRoll) {
      if (IsSpare(secondRoll))
        frame.SetKicker(new OneBonusKicker());
      else
        frame.SetKicker(new IgnoreKicker());
    }

    private bool IsSpare(int secondRoll) {
      return firstRoll + secondRoll == 10;
    }
  }

    public class TwoBonusKicker: IKicker {

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

    public class OneBonusKicker: IKicker {

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

    public class IgnoreKicker: IKicker {

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

We note that FirstRoll and SecondRoll return true (they have consumed the roll) and the rest return false. (We still don’t like those codes – maybe we should fix those first.) Each Kicker returns a different new Kicker, except for Ignore, which doesn’t change the Kicker at all. Two of the new Kickers are conditional on the current situation, two are constant. All of the Kickers, except for Ignore, add the current roll into the current Frame. Lots of duplication, none of it very large.

Yesterday, Chet was suggesting creating a superclass and putting a “Template Method” in it, to show the overall flow of the Kicker’s operation, and then putting implementations of the template’s methods down below. We think that’s still a good idea today. We foresee some interesting naming issues if nothing else.

Here Goes

We’ll add a superclass. The name Kicker is available. Here’s our first cut:

public class Kicker: IKicker {

    public bool RollConsumed(Frame frame, int pins) {
      AddRoll(pins);
      SetKicker(NextKicker());
      return BallConsumed();
    }
  }

We’re basically “programming by intention” here, imagining four methods, AddRoll, SetKicker, NextKicker, and BallConsumed, that will do the job. I think we have the word SetKicker in use, but we’ll fix that as we go. Now we’ll make all our other guys subclasses and see what goes wrong. Something surely will. We’re not sure if we should implement the methods first – we’ll try it without because we might be able to pull some of them up. Here goes …

We get diagnostics on the missing methods, and complaints about the lack of “new” on our existing implementations of RollConsumed. Let’s just build the methods and remove the RollConsumeds, and see what happens? We find that we’re not clear on abstract, virtual, and so on. This compiles, and all the tests fail because we’re using a null Kicker everywhere:

  public class Kicker: IKicker {

    public bool RollConsumed(Frame frame, int pins) {
      AddRoll(frame, pins);
      SetKicker(frame, NextKicker());
      return BallConsumed();
    }

    protected void AddRoll(Frame frame, int pins) {
      frame.AddRoll(pins);
    }

    protected void SetKicker(Frame frame, IKicker kicker) {
      frame.SetKicker(kicker);
    }

    protected bool BallConsumed() {
      return false;
    }

    protected virtual IKicker NextKicker() {
      return null;
    }
  }

Now, I guess, we can start editing the Kickers. This feels a bit off, but we’re not sure why.

[Almost an hour later…] This has turned out to be awful. I’ll spare you the details. We had to get almost all the Kickers reimplemented before any of the tests started to run. The compiler didn’t help us much. That was, we think, because the only way to force an implementation into a subclass is with “abstract”, and “abstract” can only be used in an abstract class, and we made our class concrete. Thus Kevlin’s advice was good. We think if we had made our top-level superclass abstract, the compiler would have forced implementations where they were needed. We could have put secondary subclasses in to handle common defaults, though we might still have wound up with some cases where we had to override some of the methods more than once. Or, maybe, we could have put those methods into the existing interface.

Because we’re here to learn, we think we have to gird our loins and back out all of today’s work and start over. Before we do that, we’ll snapshot our code. Backing up was easy enough, I just held down Ctrl-Z in all the windows until they reverted to where we started. Almost as good as having a decent code manager – and maybe better. OK, clean slate.

Paul Harvey, Page Two

We’ll begin by making a new abstract Kicker superclass, implementing a couple of its methods, and making the rest abstract:

  public abstract class Kicker: IKicker {

    public bool RollConsumed(Frame frame, int pins) {
      AddRoll(frame, pins);
      SetKicker(frame, NextKicker());
      return BallConsumed();
    }

    private void SetKicker(Frame frame, Kicker kicker) {
      frame.SetKicker(kicker);
    }

    protected abstract void AddRoll(Frame frame, int pins);
    protected abstract Kicker NextKicker();
    protected abstract bool BallConsumed();
  }

We think that a couple of those methods will wind up needing parameters, but we’re going to discover that rather than guess at it. Now I believe that if, instead of making everyone inherit at once, we do it one Kicker at a time, we can keep all the tests running most of the time. Let’s try that. First we’ll do IgnoreKicker because it’s simpler than most.

As soon as we make IgnoreKicker inherit from Kicker, Resharper offers to build the stubs. We fill them in, remove the previous RollConsumed method, and get this:

  public class IgnoreKicker: Kicker {

    protected override void AddRoll(Frame frame, int pins) {
      return;
    }

    protected override Kicker NextKicker() {
      return this;
    }

    protected override bool BallConsumed() {
      return false;
    }
  }
}

And the tests run! This is much better! Now OneBallBonus … we’ll start from the end and work forward:

  public class OneBonusKicker: Kicker {

    protected override void AddRoll(Frame frame, int pins) {
      frame.AddRoll(pins);
    }

    protected override Kicker NextKicker() {
      return new IgnoreKicker();
    }

    protected override bool BallConsumed() {
      return false;
    }
  }

This is way better. The tests run again. We decided to implement AddRoll in this, and every Kicker class, because if we push it up to Kicker now, we’ll lost the compiler’s help for the rest. Now on to TwoBallBonus …

  public class TwoBonusKicker: Kicker {

    protected override void AddRoll(Frame frame, int pins) {
      frame.AddRoll(pins);
    }

    protected override Kicker NextKicker() {
      return new OneBonusKicker();
    }

    protected override bool BallConsumed() {
      return false;
    }
  }

This is only a thousand times better. No guessing. We just copy from the old RollConsumed method, and then delete it. Whee. Now what … SecondRollKicker, just to stick with the pattern.

Here is where we discover that we need the pins passed down to NextKicker:

    protected override Kicker NextKicker() {
      if (IsSpare(secondRoll))
        return new OneBonusKicker();
      else
        return new IgnoreKicker();
    }

… because we need to know if we have a spare. So we’ll change the abstract one and fix the compiler errors. Everyone but this guy will ignore the pins.

  public class SecondRollKicker : Kicker {
    int firstRoll;

    public SecondRollKicker(int pins) {
      firstRoll = pins;
    }

    protected override void AddRoll(Frame frame, int secondRoll) {
      frame.AddRoll(secondRoll);
    }

    protected override Kicker NextKicker(int secondRoll) {
      if (IsSpare(secondRoll))
        return new OneBonusKicker();
      else
        return new IgnoreKicker();
    }

    protected override bool BallConsumed() {
      return true;
    }

    private bool IsSpare(int secondRoll) {
      return firstRoll + secondRoll == 10;
    }
  }

Still easy. The compiler is telling us everything we need to know. Now, we just have FirstRollKicker to do:

  public class FirstRollKicker : Kicker {

    protected override void AddRoll(Frame frame, int pins) {
      frame.AddRoll(pins);
    }

    protected override Kicker NextKicker(int pins) {
      if ( IsStrike(pins) )
        return new TwoBonusKicker();
      else
        return new SecondRollKicker(pins);
    }

    protected override bool BallConsumed() {
      return true;
    }

    private bool IsStrike(int pins) {
      return pins == 10;
    }
  }

We’re all green. This time was no hassle at all. Putting the default methods in our superclass was a bad idea. Doing it this way let us proceed one class at a time. Worth doing over: it took less than 45 minutes counting the writing and discussion, and now we feel confident in the code.

But we still have some duplication we could get rid of. For example, everyone does AddRoll by actually adding the roll, except for IgnoreKicker. We could certainly move that implementation up. We could put it in Kicker, or we could build a new superclass, RollAddingKicker. Or we could rename Kicker to RollAddingKicker, and let IgnoreKicker inherit the interface and do its own thing. We’ll rename the class and unhook IgnoreKicker:

  public abstract class RollAddingKicker: IKicker {

    public bool RollConsumed(Frame frame, int pins) {
      AddRoll(frame, pins);
      SetKicker(frame, NextKicker(pins));
      return BallConsumed();
    }

    private void SetKicker(Frame frame, RollAddingKicker kicker) {
      frame.SetKicker(kicker);
    }

    protected abstract void AddRoll(Frame frame, int pins);
    protected abstract RollAddingKicker NextKicker(int pins);
    protected abstract bool BallConsumed();
  }

Then we unhook IgnoreKicker. But oops, Resharper renamed a few too many things. We need to return IKickers, not concrete RollAddingKickers:

  public abstract class RollAddingKicker: IKicker {

    public bool RollConsumed(Frame frame, int pins) {
      AddRoll(frame, pins);
      SetKicker(frame, NextKicker(pins));
      return BallConsumed();
    }

    private void SetKicker(Frame frame, IKicker kicker) {
      frame.SetKicker(kicker);
    }

    protected abstract void AddRoll(Frame frame, int pins);
    protected abstract IKicker NextKicker(int pins);
    protected abstract bool BallConsumed();
  }

Now then, back to IgnoreKicker:

  public class IgnoreKicker: IKicker {

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

Tests are green. This is much more comfortable. Now we can move AddRoll up to RollAddingKicker, and remove it from the other Kickers. That’s done and the tests are green. I’ll spare you the code, except for RollAddingKicker and one of the others.

  public abstract class RollAddingKicker: IKicker {

    public bool RollConsumed(Frame frame, int pins) {
      AddRoll(frame, pins);
      SetKicker(frame, NextKicker(pins));
      return BallConsumed();
    }

    private void SetKicker(Frame frame, IKicker kicker) {
      frame.SetKicker(kicker);
    }

    protected void AddRoll(Frame frame, int pins) {
      frame.AddRoll(pins);
    }

    protected abstract IKicker NextKicker(int pins);
    protected abstract bool BallConsumed();
  }

  public class OneBonusKicker: RollAddingKicker {

    protected override IKicker NextKicker(int ignoredPins) {
      return new IgnoreKicker();
    }

    protected override bool BallConsumed() {
      return false;
    }
  }

Now then. There are two of the RollAddingKickers who return true to BallConsumed, and two that return false. Duplication! Must kill duplication! We can do this by adding two new classes, ConsumingRollAddingKicker and NonconsumingRollAddingKicker. The good news is that that gives each Kicker a reasonably named superclass that tells what it does. The bad news is that it creates two more classes, while replacing four methods with two.

We’re tempted to do this because it continues our trend to the ultimate. But we’re going to stop for lunch and discuss it first.

After Lunch ...

Chet has decided to move on, but I can’t go home yet, because Urban Foresters are cutting down a tree in our yard, and they have the driveway blocked. The 3M3RTX3 doesn’t park in the street, so I’m waiting for a call from Ricia telling me the coast is clear. We decided to push this implementation with the superclasses as far as it will go. The approach will be as described above, adding two new superclasses, Consuming and Nonconsuming. I’ll do them one at a time. I’m expecting no trouble, since we just did the same thing this morning. I guess I’ll do consuming first:

  public abstract class ConsumingRollAddingKicker: RollAddingKicker {

    protected override bool BallConsumed() {
      return true;
    }
  }

Now I should just be able to remove the BallConsumed method from the FirstRoll and SecondRoll kickers. Done one class at a time with tests in between, but both go green. Here’s that code:

  public class FirstRollKicker : ConsumingRollAddingKicker {

    protected override IKicker NextKicker(int pins) {
      if ( IsStrike(pins) )
        return new TwoBonusKicker();
      else
        return new SecondRollKicker(pins);
    }

    private bool IsStrike(int pins) {
      return pins == 10;
    }
  }

  public class SecondRollKicker : ConsumingRollAddingKicker {
    int firstRoll;

    public SecondRollKicker(int pins) {
      firstRoll = pins;
    }

    protected override IKicker NextKicker(int secondRoll) {
      if (IsSpare(secondRoll))
        return new OneBonusKicker();
      else
        return new IgnoreKicker();
    }

    private bool IsSpare(int secondRoll) {
      return firstRoll + secondRoll == 10;
    }
  }

Tests are green. Now Nonconsuming:

  public abstract class NonconsumingRollAddingKicker: RollAddingKicker {

    protected override bool BallConsumed() {
      return false;
    }
  }

And the using classes:

  public class OneBonusKicker: NonconsumingRollAddingKicker {

    protected override IKicker NextKicker(int ignoredPins) {
      return new IgnoreKicker();
    }
  }

… and …

  public class TwoBonusKicker: NonconsumingRollAddingKicker {

    protected override IKicker NextKicker(int ignoredPins) {
      return new OneBonusKicker();
    }
  }

Done. I’ll show the code below, but first let me sum up my impressions.

The Hierarchic Kicker Implementation

Our original kicker implementation had five concrete Kicker classes, FirstRoll, SecondRoll, TwoBonus, OneBonus, and Ignore. These classes had duplication regarding whether they added in the result, and whether they consumed the ball or just looked at it. Our task today was to remove that duplication by adding suitable superclasses.

The first time we did this, we did it poorly, primarily just because we took bites that were too large instead of proceeding one Kicker at a time. This was caused, partly, because we didn’t make our top Kicker class abstract, which meant that the compiler couldn’t help us. Then, to make matters worse, we just bulled ahead. It was hard slogging, though not challenging, and we finally got it done.

But we felt we had learned the wrong lesson: Don’t do that. We backed out the morning’s work and did it again, this time with abstract classes (containing some concrete methods) and one Kicker subclass at a time.

Then we removed duplication by putting all the Kickers that add in the score (all but one) under a common superclass. Then we removed duplication from the ones that do consume the roll, again by adding a superclass. Then, finally, we removed the duplication from the ones that do not consume, adding another superclass.

All that went very smoothly, and the code is now almost entirely without duplication. That’s all good. The bad news is that we are up around 250 lines now, for a problem that we know can be solved in about 50 lines, also with no duplication, in an admittedly rather procedural way. We have one interface, three abstract classes, and nine concrete classes. We could hide some of the classes by making them internal to our namespace or private to Frame, but basically they’re all there and have to be understood to understand this code.

I think this design needs at least two pieces of external documentation to be readily understood. The state machine drawn in a previous article is one picture that we need – that’s common when we build a state machine style solution, and I think that picture really helps.

In addition, I think that a simple hierarchy diagram would help as well, something like this:

image

Now, if a program needs a picture or other description to help people understand it, then by all means we should provide those pictures. XP isn’t about no documentation, it’s about writing code that needs as little documentation as possible, and then providing whatever is needed. However, since we know there is a 50 or 60 line program that does this same job without all this structure, we really need to ask ourselves whether we should have done this. It was great fun and we learned a bit about how to do such things – but does it pay off in some way? I’m really not sure.

Chet and I intend to explore whether it pays off. Presumably the reason to go to an OO-style solution here would be so that the code would be more extendable when new features inevitably come along. Setting aside the issue of what if they never come, we can explore whether, when they do come, this version is easier to change. To that end, after our next trick, Chet and I are going to undertake two changes to each version of interest:

  1. Provide for a GUI-style display of each frame, indicating the classical X for strike, / for spare, and so on.
  2. Implement support for the game of Candlepins, a bowling game with three balls per frame.

From these two changes, both uncontemplated in our original plans, we should see what advantages there, are, if any, to this much larger more complex version. Meanwhile, your opinions are welcome via email or on the lists.

Before we do that, however, we have one more implementation that we’d like to try. We would like to try using C# delegates to implement the kicker’s behavior, instead of separate classes. We’ll start that next week.

Thanks for tuning in, and remember to buy from our kind sponsors, especially XProgramming.com, and HendricksonXP.com, without whom none of these programs would be possible.

Today's Code

Here’s all the code, with the Kickers first and then the using code, then the tests, since mostly you are probably just wondering how the Kickers turned out.

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

  public abstract class RollAddingKicker: IKicker {

    public bool RollConsumed(Frame frame, int pins) {
      AddRoll(frame, pins);
      SetKicker(frame, NextKicker(pins));
      return BallConsumed();
    }

    private void SetKicker(Frame frame, IKicker kicker) {
      frame.SetKicker(kicker);
    }

    protected void AddRoll(Frame frame, int pins) {
      frame.AddRoll(pins);
    }

    protected abstract IKicker NextKicker(int pins);
    protected abstract bool BallConsumed();
  }

  public abstract class ConsumingRollAddingKicker: RollAddingKicker {

    protected override bool BallConsumed() {
      return true;
    }
  }

  public abstract class NonconsumingRollAddingKicker: RollAddingKicker {

    protected override bool BallConsumed() {
      return false;
    }
  }

  public class FirstRollKicker : ConsumingRollAddingKicker {

    protected override IKicker NextKicker(int pins) {
      if ( IsStrike(pins) )
        return new TwoBonusKicker();
      else
        return new SecondRollKicker(pins);
    }

    private bool IsStrike(int pins) {
      return pins == 10;
    }
  }

  public class SecondRollKicker : ConsumingRollAddingKicker {
    int firstRoll;

    public SecondRollKicker(int pins) {
      firstRoll = pins;
    }

    protected override IKicker NextKicker(int secondRoll) {
      if (IsSpare(secondRoll))
        return new OneBonusKicker();
      else
        return new IgnoreKicker();
    }

    private bool IsSpare(int secondRoll) {
      return firstRoll + secondRoll == 10;
    }
  }

  public class TwoBonusKicker: NonconsumingRollAddingKicker {

    protected override IKicker NextKicker(int ignoredPins) {
      return new OneBonusKicker();
    }
  }

  public class OneBonusKicker: NonconsumingRollAddingKicker {

    protected override IKicker NextKicker(int ignoredPins) {
      return new IgnoreKicker();
    }
  }

  public class IgnoreKicker: IKicker {

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

  public class Game {
    Frame[] frames;

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

    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;
    }
  }

  public class Frame {
    private int score;
    protected IKicker kicker;

    public Frame() {
      score = 0;
      kicker = new FirstRollKicker();
    }

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

    public void AddRoll(int pins) {
      score += pins;
    }

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

    public int Score() {
      return score;
    }
  }

  public class TenthFrame : Frame {

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

  [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());
    }

    [Test] public void Alternating() {
      RollAll( new int[] { 10, 6,4, 10, 6,4, 10, 6,4, 10, 6,4, 10, 6,4, 10});
      Assert.AreEqual(200, game.Score());
    }

    [Test] public void AlternatingSpareFirst() {
      RollAll( new int[] { 6,4, 10, 6,4, 10, 6,4, 10, 6,4, 10, 6,4, 10, 6, 4});
      Assert.AreEqual(200, game.Score());
    }
  }

  [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());
    }

    [Test] public void Strike() {
      Assert.IsTrue(frame.RollConsumed(10), "Strike");
      Assert.IsFalse(frame.RollConsumed(4), "First Bonus");
      Assert.IsFalse(frame.RollConsumed(5), "Second Bonus");
      Assert.IsFalse(frame.RollConsumed(2), "Ignored next roll");
      Assert.AreEqual(19, frame.Score());
    }

    [Test] public void TenthFrameConsumesEverything() {
      frame = new TenthFrame();
      Assert.IsTrue(frame.RollConsumed(6), "first");
      Assert.IsTrue(frame.RollConsumed(4), "second");
      Assert.IsTrue(frame.RollConsumed(10), "third");
    }
  }