TDD For Breaking Problems Apart 2: Magic Boxes
In the last post, I started a TDD exploration of the “bowling score” code kata. Today, I’m going to build a “magic box” in the course of TDD to solve the problem that arose at the end of the post with the scoring class doing too much.
Last time, we left off with production code that looked like this:
public class BowlingScoreCalculator
{
public const int Mark = 10;
public class FrameUnderflowException : Exception { }
public class FrameOverflowException : Exception { }
public int Score { get; private set; }
public void BowlFrame(int firstThrow, int secondThrow)
{
if (firstThrow < 0 || secondThrow < 0)
throw new FrameUnderflowException();
else if (firstThrow + secondThrow > Mark)
throw new FrameOverflowException();
Score += firstThrow + secondThrow;
}
}
And the last test we had written looked like this:
[TestMethod, Owner("ebd"), TestCategory("Proven"), TestCategory("Unit")]
public void Sets_Score_To_2_After_2_Frames_With_Score_Of_1_Each()
{
Target.BowlFrame(1, 0);
Target.BowlFrame(1, 0);
Assert.AreEqual(2, Target.Score);
}
I had noticed that the amount of integer literals here seemed to be a bit of a smell and was starting to wrap my head around the idea of what to do about it from a design perspective. Specifically, the smell in question is “primitive obsession,” a tendency to overuse primitive types, often redundantly, due to reluctance to make an object. So what if we made an object — a frame object — what would that look like?
Looking at the BowlFrame() method, I think “what if I had a magic box that handled frame operations?” Well, the method would take a frame object and presumably that object would handle validating the individual throws, so the method would lose that exception handling logic (and the class would probably lose those exception definitions). It would probably also make sense for the frame to encapsulate some kind of totaling mechanism internally so that the score calculator didn’t need to inspect its properties to figure out what to add.
At this point, though, I’m going to stop and start coding up the frame class as I envision it. After the first test (including a couple of non-compile failures and passes prior to the test run failure), this is what the frame test and production code looks like:
[TestClass]
public class FrameTest
{
[TestClass]
public class Constructor
{
[TestMethod, Owner("ebd"), TestCategory("Proven"), TestCategory("Unit")]
public void Initializes_FirstThrow_To_Passed_In_Value()
{
var frame = new Frame(1, 0);
Assert.AreEqual(1, frame.FirstThrow);
}
}
}
public class Frame
{
public int FirstThrow { get; private set; }
public Frame(int firstThrow, int secondThrow)
{
FirstThrow = firstThrow;
}
}
The first thing I do at this point is refactor the test to eliminate the magic numbers and duplication. At this point, you might wonder why, having done this enough times, I don’t simply start out by leaving out the magic numbers and duplication. Am I obtuse? Well, sometimes, but the reason I often don’t bother with this is that I want the test I’m writing to be the simplest possible thing as I perceive it for problem solving purposes — not necessarily the quickest and certainly not the cleanest. I wouldn’t ever purposefully make a mess, but if calling the frame constructor with (1, 0) seems more intuitive to me, then that’s what I do. I encourage you to do the same. Simplicity as you perceive it is of the utmost importance writing the failing test. Whatever it is, make it pretty and elegant later.
The next thing I do is add a similar test and then code for the second throw, with similar green and refactor. With that in place, I move the exception handling tests from the ScoreCalculator test class to the Frame test class and change them to operate on Frame’s constructor. When I do this, I get a red test, which I make green by porting the exception handling code and constants over to the new Frame class. The production code is now:
public class BowlingScoreCalculator
{
public int Score { get; private set; }
public void BowlFrame(int firstThrow, int secondThrow)
{
Score += firstThrow + secondThrow;
}
}
public class Frame
{
public class UnderflowException : Exception { }
public class OverflowException : Exception { }
public const int Mark = 10;
public int FirstThrow { get; private set; }
public int SecondThrow { get; private set; }
public Frame(int firstThrow, int secondThrow)
{
if (firstThrow < 0 || secondThrow < 0)
throw new UnderflowException();
if (firstThrow + secondThrow > Mark)
throw new OverflowException();
FirstThrow = firstThrow;
SecondThrow = secondThrow;
}
}
I’m happy that all of that logic about determining what constitutes a valid frame is now out of the BowlingScoreCalculator class, but it’s not yet sitting inside of a magic box — it’s just gone. So, let’s give the calculator the box that it’s looking for. We’re going to need a failing test, and the easiest way to do that is to change a test where we’re bowling a frame to look like this:
[TestMethod, Owner("ebd"), TestCategory("Proven"), TestCategory("Unit")]
public void With_Throws_0_And_1_Results_In_Score_1()
{
var frame = new Frame(0, 1);
Target.BowlFrame(frame);
Assert.AreEqual(frame.FirstThrow + frame.SecondThrow, Target.Score);
}
That doesn’t compile, so we can fix the broken test by adding this method in production:
public class BowlingScoreCalculator
{
public int Score { get; private set; }
public void BowlFrame(int firstThrow, int secondThrow)
{
Score += firstThrow + secondThrow;
}
public void BowlFrame(Frame frame)
{
BowlFrame(frame.FirstThrow, frame.SecondThrow);
}
}
At this point, we can refactor the rest of the tests to use the second method and then refactor that method to inline the original method and now the score calculator only accepts frames. There’s our magic box. Suddenly, we’re just worrying about how to add the scores from the frame’s throws and not whether the frame we’re being passed is valid or not. And that makes sense — we shouldn’t care about the anatomy of a frame in a class responsible for adding up and tracking frames. We should be able to assume that if the frame exists and is being handed to us that it’s valid.
Now that this bigger refactoring is done, there’s still something a little fishy. Look at this test and production code for the calculator:
[TestMethod, Owner("ebd"), TestCategory("Proven"), TestCategory("Unit")]
public void Sets_Score_To_2_After_2_Frames_With_Score_Of_1_Each()
{
var frame = new Frame(1, 0);
Target.BowlFrame(frame);
Target.BowlFrame(frame);
Assert.AreEqual(frame.FirstThrow + frame.SecondThrow + frame.FirstThrow + frame.SecondThrow, Target.Score);
}
public class BowlingScoreCalculator
{
public int Score { get; private set; }
public void BowlFrame(Frame frame)
{
Score += frame.FirstThrow + frame.SecondThrow;
}
}
We’ve fixed our primitive obsession, but there sure seems to be a lot of redundancy. I mean I have to ask frame for two things in the production code and four things in the test: two for each frame. What if I had a magic box that turned those two things into one? What would that look like? Well, here’s a test I can write against frame that I think will tell me:
[TestMethod, Owner("ebd"), TestCategory("Proven"), TestCategory("Unit")]
public void Initializes_Total_To_FirstThrow_Plus_SecondThrow()
{
const int firstThrow = 4;
const int secondThrow = 3;
Assert.AreEqual(firstThrow + secondThrow, new Frame(firstThrow, secondThrow).Total);
}
Now I just need to define that property as the sum of my two throws to make it pass. I do that, and now I can refactor my score calculator and test. Here’s what it looks like now:
[TestMethod, Owner("ebd"), TestCategory("Proven"), TestCategory("Unit")]
public void Sets_Score_To_2_After_2_Frames_With_Score_Of_1_Each()
{
var frame = new Frame(1, 0);
Target.BowlFrame(frame);
Target.BowlFrame(frame);
Assert.AreEqual(frame.Total + frame.Total, Target.Score);
}
public class BowlingScoreCalculator
{
public int Score { get; private set; }
public void BowlFrame(Frame frame)
{
Score += frame.Total;
}
}
Now, all of the frame stuff is removed from score calculation and those concerns are separated. The “magic box,” Frame, handles evaluating frames for validity and totaling them up. This score calculator class is actually starting to smell like a lazy class that could be replaced with a list and a call to a Linq extension method, but I’m going to keep it around on a hunch.
Okay, now that we’ve done a good bit of cleanup and pulled out a magic box, time to get back to implementing features. Off the top of my head, I can think of two scenarios that we don’t currently handle: the 10th frame and marks carrying over from previous frames. I’m going to work on the latter for now and write a test that when I bowl a spare in the first frame and a 5, 0 in the second frame, my score should be 20 instead of 15.
[TestMethod, Owner("ebd"), TestCategory("Proven"), TestCategory("Unit")]
public void Sets_Score_To_Twenty_After_Spare_Then_Five_Then_Zero()
{
var firstFrame = new Frame(9, 1);
var secondFrame = new Frame(5, 0);
Target.BowlFrame(firstFrame);
Target.BowlFrame(secondFrame);
Assert.AreEqual(20, Target.Score);
}
This test fails and I need to get it to pass. There are no trivial tricks I can do, but I will do the simplest thing I can think of. I’ll store an array, write frames to it, and check last frame to know when to handle this case. Here is the simplest thing I can think of that makes this pass:
public class BowlingScoreCalculator
{
private Frame[] _frames = new Frame[10];
private int _currentFrame;
public int Score { get; private set; }
public void BowlFrame(Frame frame)
{
if (_currentFrame > 0 && _frames[_currentFrame - 1].Total == Frame.Mark)
Score += frame.FirstThrow;
Score += frame.Total;
_frames[_currentFrame++] = frame;
}
}
That’s pretty ugly though, so let’s clean it up at least a little before we move on. Boy Scout Rule and all that:
public class BowlingScoreCalculator
{
private Frame[] _frames = new Frame[10];
private int _currentFrame;
public int Score { get; private set; }
public void BowlFrame(Frame frame)
{
if (WasLastFrameAMark())
Score += frame.FirstThrow;
Score += frame.Total;
_frames[_currentFrame++] = frame;
}
private bool WasLastFrameAMark()
{
return _currentFrame > 0 && _frames[_currentFrame - 1].Total == Frame.Mark;
}
}
Now the method is semantically clearer than it was with all of that crap in the guard condition. If the last frame was a mark, do something different with this frame, and then do our normal score augmentation and frame recording. We might decide to do some reasoning at this point about whether score should be computed as frames are added or only on demand, but that’s a little nitpicky, so let’s instead focus on the fact that this doesn’t score strikes correctly. So we’ll write a test in which we bowl a strike in the first frame and then a (6, 4) in the second frame and assert that the score should be 30 (unlike the current implementation, which would make it 26):
[TestMethod, Owner("ebd"), TestCategory("Proven"), TestCategory("Unit")]
public void Sets_Score_To_25_After_Strike_Then_Five_Five()
{
var firstFrame = new Frame(10, 0);
var secondFrame = new Frame(6, 4);
Target.BowlFrame(firstFrame);
Target.BowlFrame(secondFrame);
Assert.AreEqual(30, Target.Score);
}
And, let’s fix the code:
public void BowlFrame(Frame frame)
{
if (_currentFrame > 0 && _frames[_currentFrame - 1].FirstThrow == Frame.Mark)
Score += frame.Total;
else if (WasLastFrameAMark())
Score += frame.FirstThrow;
Score += frame.Total;
_frames[_currentFrame++] = frame;
}
That makes everyone green, but it’s ugly, so let’s refactor it to this:
public class BowlingScoreCalculator
{
private readonly Frame[] _frames = new Frame[10];
private int _currentFrame;
public int Score { get; private set; }
public void BowlFrame(Frame frame)
{
AddMarkBonuses(frame);
Score += frame.Total;
_frames[_currentFrame++] = frame;
}
private void AddMarkBonuses(Frame frame)
{
if (WasLastFrameAStrike())
Score += frame.Total;
else if (WasLastFrameASpare())
Score += frame.FirstThrow;
}
private bool WasLastFrameAStrike()
{
return _currentFrame > 0 && _frames[_currentFrame - 1].FirstThrow == Frame.Mark;
}
private bool WasLastFrameASpare()
{
return _currentFrame > 0 && _frames[_currentFrame - 1].Total == Frame.Mark;
}
}
Now BowlFrame() is starting to read like the actual scoring rules of bowling. “If the last frame was a strike, add current frame’s total points to the score as a bonus. Otherwise, if it was a spare, add the first throw’s points to the score as a bonus. No matter what, add this frame’s pins.” Not too shabby. At this point, before proceeding any further with the score calculator (such as handling double strikes and the 10th frame), I’d like to clean up something I don’t like: the fact that the score class is responsible for figuring out whether a frame is a strike or spare. I’m eliding the details of the tests I wrote for the Frame class to make this happen, but they’re pretty straightforward and I’ll post everything at the end of the series. Here is the new look calculator, after some refactoring:
public class BowlingScoreCalculator
{
private readonly Frame[] _frames = new Frame[10];
private int _currentFrame;
private Frame LastFrame { get { return _frames[_currentFrame - 1]; } }
public int Score { get; private set; }
public void BowlFrame(Frame frame)
{
AddMarkBonuses(frame);
Score += frame.Total;
_frames[_currentFrame++] = frame;
}
private void AddMarkBonuses(Frame frame)
{
if (WasLastFrameAStrike())
Score += frame.Total;
else if (WasLastFrameASpare())
Score += frame.FirstThrow;
}
private bool WasLastFrameAStrike()
{
return _currentFrame > 0 && LastFrame.IsStrike;
}
private bool WasLastFrameASpare()
{
return _currentFrame > 0 && LastFrame.IsSpare;
}
}
Next time, I’ll cover the addition of double strike logic and the tenth frame, and I’ll explore some different alternatives that are easily achievable with a nice suite of unit tests backing things up.
There are few things in life more frustrating than pouring a good amount of effort into something only to have it shot down without any kind of satisfying explanation of the rationale. For children this tends to be unfortunate for self interested reasons: “I want to do X in the immediate future and I can’t.” But as you get older and are motivated by more complex and nuanced concerns, these rejections get more befuddling and harder to understand. A child making a case for why he should own a Red Rider BB Gun will understand on some level the parental objection “it’s dangerous” and that the parental objection arises out of concern for his welfare. So when the “enough — I’m your parent and I say so” comes up it has the backing of some kind of implied reasoning and concern. An adult making a case that adopting accounting software instead of doing things by hand would be a money saving investment will have a harder time understanding an answer of “no, we aren’t going to do that because I’m your boss and I say so.”
