C# Bowling Kata as a TDD Exercise (Revisited)

After discussing my first implementation of the Bowling Kata as a TDD exercise with a few different people, and using the Bowling Kata as an example while trying to teach some of my coworkers how TDD works, I found that there were several points I could improve on. Also, I realized that my previous post wasn't as clear as it could have been about which steps I took and when.

So, here's a new variant of the Bowling Kata in C# as a TDD Exercise, which ends up with more or less the same end result, but is more explicit in how it gets there and uncovers some jumps I made in the previous attempt. The changes mainly lie in the process and description.

To start, I realized that I put too much into getting my first test up, so to stay truer to the TDD principles, I've simplified. Here comes take two of my TDD excercise.

Bowling Kata in C# as a TDD Exercise
My first test is as follows. The Scorer class isn't implemented yet.

[Test]
public void CalculateScore_AllBallsAreGutterBalls_0()
{
    var scorer = new Scorer();
    var score = scorer.CalculateScore();
    Assert.AreEqual(0, score);
}

Both the class creation and the CalculateScore methods show up red, as expected, and my first test doesn't compile. I solve that by creating the class and adding the following method.

public int CalculateScore()
{
    return 0;
}

It compiles, and causes my test to pass. This test basically tells me that I have a class that is live and initialized, and a CalculateScore method that works for all gutterballs.

For the next test I need to show that my CalculateScore method can actually calculate the score for more than one defined game, so I set up a second test for rolling all ones, instead of zeros.

Since I'm declaring the scorer object in two places, I refactor it out and initialize it in the setup.

private Scorer scorer;

[SetUp]
public void SetUp()
{
    scorer = new Scorer();    
}

The test looks like the following:

[Test]
public void CalculateScore_AllBallsKnockDownOnePin_20()
{
    for (var i=0; i < 20; i++)
        scorer.Roll(1);

    var score = scorer.CalculateScore();
    Assert.AreEqual(20, score);
}

This forces me to add some logic and a Roll method to allow me to update the number of pins knocked down per roll. I don't need to add any tests that test the Roll method specifically, since that will be proven through getting the correct result from the CalculateScore method. Conversely, I can't get the correct score from the CalculateScore method without the Roll method, which will show that they're both necessary and functioning. The code that makes the new test function without breaking the old test is this:

private int score;

public int CalculateScore()
{
    return score;
}

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

Next I need to deal with special rolls, the strikes and spares. Rather than trying to solve both the case of extra frames and special score handling at once, I reduce the next test to represent a strike in the first frame, and the rest of the balls as ones. Again I have some code duplication, since I need another loop to roll the ones, so I separate the ball rolling into its own helper method. Its name is a bit longer than I like, but I don't have a more concise name while leaving its function clear.

private void RollManyBallsWithSameNumberOfPins(int numberOfBalls, int pins)
{
    for(var i=0; i < numberOfBalls; i++)
        scorer.Roll(pins);
}

And the test itself looks like:

[Test]
public void CalculateScore_FirstFrameIsStrikeRestOnes_30()
{
    scorer.Roll(10);
    RollManyBallsWithSameNumberOfPins(18, 1);
    var score = scorer.CalculateScore();
    Assert.AreEqual(30, score);
}

To handle a strike, it becomes necessary to remember each roll, so I add a list of integers to do that, and modify the Roll method to add to it. The score variable can be made local. The score calculation is then as follows (remembering that a bowling strike is 10 + the next two rolls).

private readonly IList<int> rolls = new List<int>();

public int CalculateScore()
{
    var score = 0;

    for (var i = 0; i < rolls.Count; i++)
    {
        if (rolls[i] == 10)
            score += rolls[i] + rolls[i + 1] + rolls[i + 2];
        else
            score += rolls[i];
    }

    return score;
}

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

The test passes, but there is trouble lurking at the edge cases. So the next test is with all strikes to see how the code handles that.

[Test]
public void CalculateScore_AllFramesAreStrikes_300()
{
    RollManyBallsWithSameNumberOfPins(12, 10);
    var score = scorer.CalculateScore();
    Assert.AreEqual(300, score);
}

As expected, bowling a perfect game blows up at the end of the scoring list. The current algorithm tries to add two balls to the last two bonus balls, which are really just there to help score the last 'true' strike. So we fix the scorer to make the test pass. The easiest way to do this is to start scoring frames instead of individual rolls.

private readonly IList<int> rolls = new List<int>();
private int currentRoll;

public int CalculateScore()
{
    var score = 0;
    currentRoll = 0;

    for (var i = 0; i < 10; i++)
    {
        if (rolls[currentRoll] == 10)
        {
            score += rolls[currentRoll] + rolls[currentRoll + 1] + rolls[currentRoll + 2];
            currentRoll++;
        }
        else
        {
            score += rolls[currentRoll] + rolls[currentRoll + 1];
            currentRoll += 2;
        }
    }
    
    return score;                       
}

This makes my test pass, but wait! My first test suddenly implodes, failing with an out of range exception. While this test has been passing since the very beginning, I actually made a mistake there. It doesn't test that the scorer handles gutterballs, but just if the CalculateScore method exists.

And with the new implementation, it becomes obvious that the CalculateScore method has a temporal (time-based) dependency on the Roll method. In other words, it relies on the Roll method having been called first to initialize the list of rolls. In real life, this is fairly obvious, since it doesn't make sense to score a bowling game that hasn't been played yet, but this could create trouble down the road in code.

There are many ways to handle this, and that goes a bit outside the scope of this excercise. However, the point to notice here is that the unstable code was exposed by the tests. If I were just coding this directly, the instability might have gone into production and been triggered by other code. However, I will fix this the easy way, and correct the gutterballs test to actually roll an appropriate number of zeros.

[Test]
public void CalculateScore_AllBallsAreGutterBalls_0()
{
    RollManyBallsWithSameNumberOfPins(20, 0);
    var score = scorer.CalculateScore();
    Assert.AreEqual(0, score);
}

Now all tests pass again, and we can move on to scoring spares. Again I follow the pattern of just testing the first frame, so I'm not testing for more than one thing at a time. First I roll two fives for a spare and then fill in the rest of the rolls with ones.

[Test]
public void CalculateScore_FirstFrameIsSpareRestOnes_29()
{
    RollManyBallsWithSameNumberOfPins(2, 5);
    RollManyBallsWithSameNumberOfPins(18, 1);
    var score = scorer.CalculateScore();
    Assert.AreEqual(29, score);
}

The following code change makes the test pass, inserting a new conditional between the strike and the regular roll.

if (rolls[currentRoll] == 10)
{
    score += rolls[currentRoll] + rolls[currentRoll + 1] + rolls[currentRoll + 2];
    currentRoll++;
}
else if (rolls[currentRoll] + rolls[currentRoll + 1] == 10)
{
    score += rolls[currentRoll] + rolls[currentRoll + 1] + rolls[currentRoll + 2];
    currentRoll += 2;
}
else
{
    score += rolls[currentRoll] + rolls[currentRoll + 1];
    currentRoll += 2;
}

Next I write a test where all rolls are spares, which also passes, since the logic is already such that the spare frames are taken care of.

[Test]
public void CalculateScore_AllFramesAreSpares_150()
{
    RollManyBallsWithSameNumberOfPins(21, 5);
    var score = scorer.CalculateScore();
    Assert.AreEqual(150, score);
}

Conclusion
And with that, the bowling scorer is functionally complete. Some refactoring remains, which you can see the result of below. Other than that, I have a couple of finishing thoughts which I'm still trying to work out good answers for.

What to do with tests which are covered by later tests? For example, the tests that test the first strike or spare, are covered by the later tests that test all strikes or spares. Should I take those early tests out once the broader tests are written? While I want total test coverage, I at the same time want as few tests as possible to achieve that, so less changes are necessary if the specification should change later. Those tests seem superfluous now.

On the other hand, maybe my tests are bad. Perhaps there is a different test I could write that only tests for the edge case of strike on the last frame and as such keep the responsibilities separate. But even so, I wouldn't feel safe without a test that tested a perfect game, for example, to see that all the parts hang together as they should.

Edit: The original version of this post had the final code unrefactored, which was the result an incorrect copy and paste. I did the quick fix of calling it an exercise for the reader, but since I was called on it, I've gone back and pasted in the refactored code.

Final Code
Tests:

using NUnit.Framework;

namespace BowlingKata02
{
    [TestFixture]
    public class ScorerTest
    {
        private Scorer scorer;

        [SetUp]
        public void SetUp()
        {
            scorer = new Scorer();
        }

        private void RollManyBallsWithSameNumberOfPins(int numberOfBalls, int pins)
        {
            for (var i = 0; i < numberOfBalls; i++)
                scorer.Roll(pins);
        }

        [Test]
        public void CalculateScore_AllBallsAreGutterBalls_0()
        {
            RollManyBallsWithSameNumberOfPins(20, 0);
            var score = scorer.CalculateScore();
            Assert.AreEqual(0, score);
        }

        [Test]
        public void CalculateScore_AllBallsKnockDownOnePin_20()
        {
            RollManyBallsWithSameNumberOfPins(20, 1);
            var score = scorer.CalculateScore();
            Assert.AreEqual(20, score);
        }

        [Test]
        public void CalculateScore_FirstFrameIsStrikeRestOnes_30()
        {
            scorer.Roll(10);
            RollManyBallsWithSameNumberOfPins(18, 1);
            var score = scorer.CalculateScore();
            Assert.AreEqual(30, score);
        }

        [Test]
        public void CalculateScore_AllFramesAreStrikes_300()
        {
            RollManyBallsWithSameNumberOfPins(12, 10);
            var score = scorer.CalculateScore();
            Assert.AreEqual(300, score);
        }

        [Test]
        public void CalculateScore_FirstFrameIsSpareRestOnes_29()
        {
            RollManyBallsWithSameNumberOfPins(2, 5);
            RollManyBallsWithSameNumberOfPins(18, 1);
            var score = scorer.CalculateScore();
            Assert.AreEqual(29, score);
        }

        [Test]
        public void CalculateScore_AllFramesAreSpares_150()
        {
            RollManyBallsWithSameNumberOfPins(21, 5);
            var score = scorer.CalculateScore();
            Assert.AreEqual(150, score);
        }
    }
}

Code:

using System.Collections.Generic;

namespace BowlingKata02
{
    public class Scorer
    {
        private readonly IList<int> rolls = new List<int>();
        private int currentRoll;
        private int score;

        public int CalculateScore()
        {
            score = 0;
            currentRoll = 0;

            ScoreAllRolls();

            return score;
        }

        private void ScoreAllRolls()
        {
            for (var frame = 0; frame < 10; frame++)
            {
                if (rolls[currentRoll] == 10)
                    ScoreStrike();
                else if (SumUpRollsFromCurrent(2) == 10)
                    ScoreSpare();
                else
                    ScoreNormal();
            }
        }

        private void ScoreNormal()
        {
            score += SumUpRollsFromCurrent(2);
            currentRoll += 2;
        }

        private void ScoreSpare()
        {
            score += SumUpRollsFromCurrent(3);
            currentRoll += 2;
        }

        private void ScoreStrike()
        {
            score += SumUpRollsFromCurrent(3);
            currentRoll++;
        }

        private int SumUpRollsFromCurrent(int numberOfRolls)
        {
            var sum = 0;
            for (var i = 0; i < numberOfRolls; i++)
                sum += rolls[currentRoll + i];
            return sum;
        }

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