Agile Principles, Patterns, and Practices in C#

Will you help me write a little application that calculates bowling scores?

(Reflects to himself: The XP practice of pair programming says that I can't say no when asked to help. I suppose that's especially true when it is your boss who is asking.) Sure, Bob, I'd be glad to help.

OK, great. What I'd like to do is write an application that keeps track of a bowling league. It needs to record all the games, determine the ranks of the teams, determine the winners and losers of each weekly match, and accurately score each game.

Cool. I used to be a pretty good bowler. This will be fun. You rattled off several user stories; which one would you like to start with?

Let's begin with scoring a single game.

OK. What does that mean? What are the inputs and outputs for this story?

It seems to me that the inputs are simply a sequence of throws. A throw is an integer that tells how many pins were knocked down by the ball. The output is the score for each frame.

I'm assuming you are acting as the customer in this exercise; so what form do you want the inputs and outputs to be in?

Yes, I'm the customer. We'll need a function to call to add throws and another function that gets the score. Sort of like:

ThrowBall(6); ThrowBall(3); Assert.AreEqual(9, GetScore());

OK, we're going to need some test data. Let me sketch out a little picture of a scorecard. [See Figure 6-1.]

That guy is pretty erratic.

Or drunk, but it will serve as a decent acceptance test.

We'll need others, but let's deal with that later. How should we start? Shall we come up with a design for the system?

I wouldn't mind a UML diagram showing the problem domain concepts that we might see from the scorecard. That will give us some candidate objects that we can explore further in code.

(putting on his powerful object designer hat) OK, clearly a game object consists of a sequence of ten frames. Each frame object contains one, two, or three throws.

Great minds. That was exactly what I was thinking. Let me quickly draw that. [See Figure 6-2.]

Well, pick a class, any class. Shall we start at the end of the dependency chain and work backward? That will make testing easier.

Sure, why not. Let's create a test case for the Throw class.

(starts typing)

//ThrowTest.cs-------------------------------- using NUnit.Framework; [TestFixture] public class ThrowTest { [Test] public void Test??? }

Do you have a clue what the behavior of a Throw object should be?

It holds the number of pins knocked down by the player.

OK, you just said, in not so many words, that it doesn't really do anything. Maybe we should come back to it and focus on an object that actually has behavior instead of one that's simply a data store.

Hmm. You mean the Throw class might not really exist?

Well, if it doesn't have any behavior, how important can it be? I don't know if it exists or not yet. I'd just feel more productive if we were working on an object that had more than setters and getters for methods. But if you want to drive . . . (slides the keyboard to RCM).

Well, let's move up the dependency chain to Frame and see whether there are any test cases we can write that will force us to finish Throw (pushes the keyboard back to RSK).

(wondering whether RCM is leading me down a blind alley to educate me or whether he is really agreeing with me) OK, new file, new test case.

//FrameTest.cs------------------------------------ using NUnit.Framework; [TestFixture] public class FrameTest { [Test] public void Test??? }

OK, that's the second time we've typed that. Now, can you think of any interesting test cases for Frame?

A Frame might provide its score, the number of pins on each throw, whether there was a strike or a spare . . .

OK, show me the code.

(types)

//FrameTest.cs----------------------------- using NUnit.Framework; [TestFixture] public class FrameTest { [Test] public void TestScoreNoThrows() { Frame f = new Frame(); Assert.AreEqual(0, f.Score); } } //Frame.cs--------------------------------------- public class Frame { public int Score { get { return 0; } } }

OK, the test case passes. But Score is a really stupid property. It will fail if we add a throw to the Frame. So let's write the test case that adds some throws and then checks the score.

//FrameTest.cs--------------------------------- [Test] public void TestAddOneThrow() { Frame f = new Frame(); f.Add(5); Assert.AreEqual(5, f.Score); }

That doesn't compile. There's no Add method in Frame.

I'll bet if you define the method, it will compile ;-)

(types)

//Frame.cs--------------------------------------- public class Frame { public int Score { get { return 0 }; } public void Add(Throw t) { } }

(thinking out loud) This doesn't compile, because we haven't written the Throw class.

Talk to me, Bob. The test is passing an integer, and the method expects a THRow object. You can't have it both ways. Before we go down the THRow path again, can you describe its behavior?

Wow! I didn't even notice that I had written f.Add(5). I should have written f.Add(new Throw(5)), but that's ugly as hell. What I really want to write is f.Add(5).

Ugly or not, let's leave aesthetics out of it for the time being. Can you describe any behavior of a Throw objectbinary response, Bob.

101101011010100101. I don't know whether there is any behavior in Throw; I'm beginning to think that a Throw is just an int. However, we don't need to consider that yet, since we can write Frame.Add to take an int.

Then I think we should do that for no other reason than it's simple. When we feel pain, we can do something more sophisticated.

Agreed.

//Frame.cs--------------------------------------- public class Frame { public int Score { get { return 0}; } public void Add(int pins) { } }

OK, this compiles and fails the test. Now, let's make the test pass.

//Frame.cs--------------------------------------- public class Frame { private int score; public int Score { get { return score; } } public void Add(int pins) { score += pins; } }

This compiles and passes the tests. But it's clearly simplistic. What's the next test case?

Can we take a break first?

That's better. Frame.Add is a fragile function. What if you call it with an 11?

It can throw an exception if that happens. But who is calling it? Is this going to be an application framework that thousands of people will use and we have to protect against such things, or is this going to be used by you and only you? If the latter, just don't call it with an 11 (chuckle).

Good point; the tests in the rest of the system will catch an invalid argument. If we run into trouble, we can put the check in later.

So, the Add function doesn't currently handle strikes or spares. Let's write a test case that expresses that.

Hmmmm. If we call Add(10) to represent a strike, what should GetScore() return? I don't know how to write the assertion, so maybe we're asking the wrong question. Or we're asking the right question of the wrong object.

When you call Add(10), or Add(3) followed by Add(7), calling Score on the Frame is meaningless. The Frame would have to look at later Frame instances to calculate its score. If those later Frame instances don't exist, it would have to return something ugly, such as -1. I don't want to return -1.

Yeah, I hate the -1 idea too. You've introduced the idea of Frames knowing about other Frames. Who is holding these different Frame objects?

The Game object.

So Game depends on Frame, and Frame in turn depends back on Game. I hate that.

Frames don't have to depend on Game; they could be arranged in a linked list. Each Frame could hold pointers to its next and previous Frames. To get the score from a Frame, the Frame would look backward to get the score of the previous Frame and look forward for any spare or strike balls it needs.

OK, I'm feeling kind of dumb because I can't visualize this. Show me some code.

Right. So we need a test case first.

For Game or another test for Frame?

I think we need one for Game, since it's Game that will build the Frames and hook them up to each other.

Do you want to stop what we're doing on Frame and do a mental longjump to Game, or do you just want to have a MockGame object that does just what we need to get Frame working?

No, let's stop working on Frame and start working on Game. The test cases in Game should prove that we need the linked list of Frames.

I'm not sure how they'll show the need for the list. I need code.

(types)

//GameTest.cs------------------------------------------ using NUnit.Framework; [TestFixture] public class GameTest { [Test] public void TestOneThrow() { Game game = new Game(); game.Add(5); Assert.AreEqual(5, game.Score); } }

Does that look reasonable?

Sure, but I'm still looking for proof for this list of Frames.

Me too. Let's keep following these test cases and see where they lead.

//Game.cs---------------------------------- public class Game { public int Score { get { return 0; } } public void Add(int pins) { } }

OK; this compiles and fails the test. Now let's make it pass.

//Game.cs---------------------------------- public class Game { private int score; public int Score { get { return score; } } public void Add(int pins) { score += pins; } }

This passes. Good.

I can't disagree with it. But I'm still looking for this great proof of the need for a linked list of Frame objects. That's what led us to Game in the first place.

Yeah, that's what I'm looking for, too. I fully expect that once we start injecting spare and strike test cases, we'll have to build Frames and tie them together in a linked list. But I don't want to build that until the code forces us to.

Good point. Let's keep going in small steps on Game. What about another test that tests two throws but with no spare?

OK; that should pass right now. Let's try it.

//GameTest.cs-------------------------------- [Test] public void TestTwoThrowsNoMark() { Game game = new Game(); game.Add(5); game.Add(4); Assert.AreEqual(9, game.Score); }

Yep, that one passes. Now let's try four balls, with no marks.

Well, that will pass too. I didn't expect this. We can keep adding throws, and we don't ever even need a Frame. But we haven't done a spare or a strike yet. Maybe that's when we'll have to make one.

That's what I'm counting on. However, consider this test case:

//TestGame.cs------------------------------------------ [Test] public void TestFourThrowsNoMark() { Game game = new Game(); game.Add(5); game.Add(4); game.Add(7); game.Add(2); Assert.AreEqual(18, game.Score); Assert.AreEqual(9, game.ScoreForFrame(1)); Assert.AreEqual(18, game.ScoreForFrame(2)); }

Does this look reasonable?

It sure does. I forgot that we have to be able to show the score in each frame. Ah, our sketch of the scorecard was serving as a coaster for my Diet Coke. Yeah, that's why I forgot.

(sigh) OK; first, let's make this test case fail by adding the ScoreForFrame method to Game.

//Game.cs---------------------------------- public int ScoreForFrame(int frame) { return 0; }

Great; this compiles and fails. Now, how do we make it pass?

We can start making Frame objects. But is that the simplest thing that will get the test to pass?

No, actually, we could simply create an array of integers in the Game. Each call to Add would append a new integer onto the array. Each call to ScoreForFrame will simply work forward through the array and calculate the score.

//Game.cs---------------------------------- public class Game { private int score; private int[] throws = new int[21]; private int currentThrow; public int Score { get { return score; } } public void Add(int pins) { throws[currentThrow++] = pins; score += pins; } public int ScoreForFrame(int frame) { int score = 0; for(int ball = 0; frame < 0 && ball < currentThrow; ball+=2, frame--) { score += throws[ball] + throws[ball + 1]; } return score; } }

(very satisfied with himself) There, that works.

Why the magic number 21?

That's the maximum possible number of throws in a game.

Yuck. Let me guess; in your youth, you were a UNIX hacker and prided yourself on writing an entire application in one statement that nobody else could decipher.

ScoreForFrame() needs to be refactored to be more communicative. But before we consider refactoring, let me ask another question. Is Game the best place for this method? In my mind, Game is violating the Single-Responsibility Principle. [See Chapter 8.] It is accepting throws, and it knows how to score for each frame. What would you think about a Scorer object?

(makes a rude oscillating gesture with his hand) I don't know where the functions live now; right now, I'm interested in getting the scoring stuff to work. Once we've got that all in place, then we can debate the values of the SRP. However, I see your point about the UNIX hacker stuff; let's try to simplify that loop.

public int ScoreForFrame(int theFrame) { int ball = 0; int score=0; for (int currentFrame = 0; currentFrame < theFrame; currentFrame++) { score += throws[ball++] + throws[ball++]; } return score; }

That's a little better, but there are side effects in the score+= expression. They don't matter here, because it doesn't matter which order the two addend expressions are evaluated in. (Or does it? Is it possible that the two increments could be done before either array operations?)

I suppose we could do an experiment to verify that there aren't any side effects, but that function isn't going to work with spares and strikes. Should we keep trying to make it more readable, or should we push further on its functionality?

The experiment might have meaning only on certain compilers. Other compilers might use different evaluation orders. I don't know whether this is an issue, but let's get rid of the potential order dependency and then push on with more test cases.

public int ScoreForFrame(int theFrame) { int ball = 0; int score=0; for (int currentFrame = 0; currentFrame < theFrame; currentFrame++) { int firstThrow = throws[ball++]; int secondThrow = throws[ball++]; score += firstThrow + secondThrow; } return score; }

OK, next test case. Let's try a spare.

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

I'm tired of writing this. Let's refactor the test and put the creation of the game in a SetUp function.

//GameTest.cs-------------------------------- using NUnit.Framework; [TestFixture] public class GameTest { private Game game; [SetUp] public void SetUp() { game = new Game(); } [Test] public void TestOneThrow() { game.Add(5); Assert.AreEqual(5, game.Score); } [Test] public void TestTwoThrowsNoMark() { game.Add(5); game.Add(4); Assert.AreEqual(9, game.Score); } [Test] public void TestFourThrowsNoMark() { game.Add(5); game.Add(4); game.Add(7); game.Add(2); Assert.AreEqual(18, game.Score); Assert.AreEqual(9, game.ScoreForFrame(1)); Assert.AreEqual(18, game.ScoreForFrame(2)); } [Test] public void TestSimpleSpare() { } }

That's better; now let's write the spare test case.

[Test] public void TestSimpleSpare() { game.Add(3); game.Add(7); game.Add(3); Assert.AreEqual(13, game.ScoreForFrame(1)); }

OK, that test case fails. Now we need to make it pass.

I'll drive.

public int ScoreForFrame(int theFrame) { int ball = 0; int score=0; for (int currentFrame = 0; currentFrame < theFrame; currentFrame++) { int firstThrow = throws[ball++]; int secondThrow = throws[ball++]; int frameScore = firstThrow + secondThrow; // spare needs next frames first throw if ( frameScore == 10 ) score += frameScore + throws[ball++]; else score += frameScore; } return score; }

Yee-HA! That works!

(grabbing the keyboard) OK, but I think the increment of ball in the frameScore==10 case shouldn't be there. Here's a test case that proves my point.

[Test] public void TestSimpleFrameAfterSpare() { game.Add(3); game.Add(7); game.Add(3); game.Add(2); Assert.AreEqual(13, game.ScoreForFrame(1)); Assert.AreEqual(18, game.Score); }

Ha! See, that fails. Now if we just take out that pesky extra increment . . .

if ( frameScore == 10 ) score += frameScore + throws[ball];

Uh, it still fails. Could it be that the Score method is wrong? I'll test that by changing the test case to use ScoreForFrame(2).

[Test] public void TestSimpleFrameAfterSpare() { game.Add(3); game.Add(7); game.Add(3); game.Add(2); Assert.AreEqual(13, game.ScoreForFrame(1)); Assert.AreEqual(18, game.ScoreForFrame(2)); }

Hmmmm. That passes. The Score property must be messed up. Let's look at it.

public int Score { get { return score; } } public void Add(int pins) { throws[currentThrow++] = pins; score += pins; }

Yeah, that's wrong. The Score property is simply returning the sum of the pins, not the proper score. What we need Score to do is call ScoreForFrame() with the current frame.

We don't know what the current frame is. Let's add that message to each of our current tests, one at a time, of course.

Right.

//GameTest.cs-------------------------------- [Test] public void TestOneThrow() { game.Add(5); Assert.AreEqual(5, game.Score); Assert.AreEqual(1, game.CurrentFrame); } //Game.cs---------------------------------- public int CurrentFrame { get { return 1; } }

OK, that works. But it's stupid. Let's do the next test case.

[Test] public void TestTwoThrowsNoMark() { game.Add(5); game.Add(4); Assert.AreEqual(9, game.Score); Assert.AreEqual(1, game.CurrentFrame); }

That one's uninteresting; let's try the next.

[Test] public void TestFourThrowsNoMark() { game.Add(5); game.Add(4); game.Add(7); game.Add(2); Assert.AreEqual(18, game.Score); Assert.AreEqual(9, game.ScoreForFrame(1)); Assert.AreEqual(18, game.ScoreForFrame(2)); Assert.AreEqual(2, game.CurrentFrame); }

This one fails. Now let's make it pass.

I think the algorithm is trivial. Just divide the number of throws by 2, since there are two throws per frame. Unless we have a strike. But we don't have strikes yet, so let's ignore them here too.

(flails around, adding and subtracting 1 until it works)^[1]

^[1] Dave Thomas and Andy Hunt call this programming by coincidence.

public int CurrentFrame { get { return 1 + (currentThrow - 1) / 2; } }

That's isn't very satisfying.

What if we don't calculate it each time? What if we adjust a currentFrame member variable after each throw?

OK, let's try that.

//Game.cs---------------------------------- private int currentFrame; private bool isFirstThrow = true; public int CurrentFrame { get { return currentFrame; } } public void Add(int pins) { throws[currentThrow++] = pins; score += pins; if (isFirstThrow) { isFirstThrow = false; currentFrame++; } else { isFirstThrow=true;; } }

OK, this works. But it also implies that the current frame is the frame of the last ball thrown, not the frame that the next ball will be thrown into. As long as we remember that, we'll be fine.

I don't have that good of a memory, so let's make it more readable. But before we go screwing around with it some more, let's pull that code out of Add() and put it in a private member function called AdjustCurrentFrame() or something.

OK, that sounds good.

public void Add(int pins) { throws[currentThrow++] = pins; score += pins; AdjustCurrentFrame(); } private void AdjustCurrentFrame() { if (isFirstThrow) { isFirstThrow = false; currentFrame++; } else { isFirstThrow=true;; } }

Now let's change the variable and function names to be more clear. What should we call currentFrame?

I kind of like that name. I don't think we're incrementing it in the right place, though. The current frame, to me, is the frame number that I'm throwing in. So it should get incremented right after the last throw in a frame.

I agree. Let's change the test cases to reflect that; then we'll fix AdjustCurrentFrame.

//GameTest.cs-------------------------------- [Test] public void TestTwoThrowsNoMark() { game.Add(5); game.Add(4); Assert.AreEqual(9, game.Score); Assert.AreEqual(2, game.CurrentFrame); } [Test] public void TestFourThrowsNoMark() { game.Add(5); game.Add(4); game.Add(7); game.Add(2); Assert.AreEqual(18, game.Score); Assert.AreEqual(9, game.ScoreForFrame(1)); Assert.AreEqual(18, game.ScoreForFrame(2)); Assert.AreEqual(3, game.CurrentFrame); } //Game.cs---------------------------------- private int currentFrame = 1; private void AdjustCurrentFrame() { if (isFirstThrow) { isFirstThrow = false; } else { isFirstThrow=true; currentFrame++; } }

OK, that's working. Now let's test CurrentFrame in the two spare cases.

[Test] public void TestSimpleSpare() { game.Add(3); game.Add(7); game.Add(3); Assert.AreEqual(13, game.ScoreForFrame(1)); Assert.AreEqual(2, game.CurrentFrame); } [Test] public void TestSimpleFrameAfterSpare() { game.Add(3); game.Add(7); game.Add(3); game.Add(2); Assert.AreEqual(13, game.ScoreForFrame(1)); Assert.AreEqual(18, game.ScoreForFrame(2)); Assert.AreEqual(3, game.CurrentFrame); }

This works. Now, back to the original problem. We need Score to work. We can now write Score to call ScoreForFrame(CurrentFrame-1).

[Test] public void TestSimpleFrameAfterSpare() { game.Add(3); game.Add(7); game.Add(3); game.Add(2); Assert.AreEqual(13, game.ScoreForFrame(1)); Assert.AreEqual(18, game.ScoreForFrame(2)); Assert.AreEqual(18, game.Score); Assert.AreEqual(3, game.CurrentFrame); } //Game.cs---------------------------------- public int Score() { return ScoreForFrame(CurrentFrame - 1); }

This fails the TestOneThrow test case. Let's look at it.

[Test] public void TestOneThrow() { game.Add(5); Assert.AreEqual(5, game.Score); Assert.AreEqual(1, game.CurrentFrame); }

With only one throw, the first frame is incomplete. The score method is calling ScoreForFrame(0). This is yucky.

Maybe, maybe not. Who are we writing this program for, and who is going to be calling Score? Is it reasonable to assume that it won't get called on an incomplete frame?

Yeah. But it bothers me. To get around this, we have to take the score out of the TestOneThrow test case. Is that what we want to do?

We could. We could even eliminate the entire TestOneThrow test case. It was used to ramp us up to the test cases of interest. Does it really serve a useful purpose now? We still have coverage in all of the other test cases.

Yeah, I see your point. OK, out it goes. (edits code, runs test, gets green bar) Ahhh, that's better.

Now, we'd better work on the strike test case. After all, we want to see all those Frame objects built into a linked list, don't we? (snicker).

[Test] public void TestSimpleStrike() { game.Add(10); game.Add(3); game.Add(6); Assert.AreEqual(19, game.ScoreForFrame(1)); Assert.AreEqual(28, game.Score); Assert.AreEqual(3, game.CurrentFrame); }

OK, this compiles and fails as predicted. Now we need to make it pass.

//Game.cs---------------------------------- public class Game { private int score; private int[] throws = new int[21]; private int currentThrow; private int currentFrame = 1; private bool isFirstThrow = true; public int Score { get { return ScoreForFrame(GetCurrentFrame() - 1); } } public int CurrentFrame { get { return currentFrame; } } public void Add(int pins) { throws[currentThrow++] = pins; score += pins; AdjustCurrentFrame(pins); } private void AdjustCurrentFrame(int pins) { if (isFirstThrow) { if(pins == 10) //Strike currentFrame++; else isFirstThrow = false; } else { isFirstThrow=true; currentFrame++; } } public int ScoreForFrame(int theFrame) { int ball = 0; int score=0; for (int currentFrame = 0; currentFrame < theFrame; currentFrame++) { int firstThrow = throws[ball++]; if(firstThrow == 10) //Strike { score += 10 + throws[ball] + throws[ball+1]; } else { int secondThrow = throws[ball++]; int frameScore = firstThrow + secondThrow; // spare needs next frames first throw if ( frameScore == 10 ) score += frameScore + throws[ball]; else score += frameScore; } } return score; } }

OK, that wasn't too hard. Let's see if it can score a perfect game.

[Test] public void TestPerfectGame() { for (int i=0; i<12; i++) { game.Add(10); } Assert.AreEqual(300, game.Score); Assert.AreEqual(10, game.CurrentFrame); }

Urg, it's saying that the score is 330. Why would that be?

Because the current frame is getting incremented all the way to 12.

Oh! We need to limit it to 10.

private void AdjustCurrentFrame(int pins) { if (isFirstThrow) { if(pins == 10) //Strike currentFrame++; else isFirstThrow = false; } else { isFirstThrow=true; currentFrame++; } if(currentFrame > 10) currentFrame = 10; }

Damn, now it's saying that the score is 270. What's going on?

Bob, the Score property is subtracting 1 from SetCurrentFrame, so it's giving you the score for frame 9, not 10.

What? You mean I should limit the current frame to 11, not 10? I'll try it.

if(currentFrame > 11) currentFrame = 11;

OK, so now it gets the score correct but fails because the current frame is 11 and not 10. Ick! this current frame thing is a pain in the butt. We want the current frame to be the frame the player is throwing into, but what does that mean at the end of the game?

Maybe we should go back to the idea that the current frame is the frame of the last ball thrown.

Or maybe we need to come up with the concept of the last completed frame? After all, the score of the game at any time is the score in the last completed frame.

A completed frame is a frame that you can write the score into, right?

Yes, a frame with a spare in it completes after the next ball. A frame with a strike in it completes after the next two balls. A frame with no mark completes after the second ball in the frame.

Wait a minute. We are trying to get the Score property to work, right? All we need to do is force Score to call ScoreForFrame(10) if the game is complete.

How do we know whether the game is complete?

If AdjustCurrentFrame ever tries to increment currentFrame past the tenth frame, the game is complete.

Wait. All you are saying is that if CurrentFrame returns 11, the game is complete; that's the way the code works now!

Hmm. You mean we should change the test case to match the code?

[Test] public void TestPerfectGame() { for (int i=0; i<12; i++) { game.Add(10); } Assert.AreEqual(300, game.Score); Assert.AreEqual(11, game.CurrentFrame); }

Well, that works. But I still feel uneasy about it.

Maybe something will occur to us later. Right now, I think I see a bug. May I? (grabs keyboard)

[Test] public void TestEndOfArray() { for (int i=0; i<9; i++) { game.Add(0); game.Add(0); } game.Add(2); game.Add(8); // 10th frame spare game.Add(10); // Strike in last position of array. Assert.AreEqual(20, game.Score); }

Hmm. That doesn't fail. I thought since the twenty-first position of the array was a strike, the scorer would try to add the twenty-second and twenty-third positions to the score. But I guess not.

Hmm, you are still thinking about that scorer object, aren't you. Anyway, I see what you were getting at, but since score never calls ScoreForFrame with a number larger than 10, the last strike is not actually counted as a strike. It's just counted at a 10 to complete the last spare. We never walk beyond the end of the array.

OK, lets pump our original score card into the program.

[Test] public void TestSampleGame() { game.Add(1); game.Add(4); game.Add(4); game.Add(5); game.Add(6); game.Add(4); game.Add(5); game.Add(5); game.Add(10); game.Add(0); game.Add(1); game.Add(7); game.Add(3); game.Add(6); game.Add(4); game.Add(10); game.Add(2); game.Add(8); game.Add(6); Assert.AreEqual(133, game.Score); }

Well, that works. Are there any other test cases that you can think of?

Yeah, let's test a few more boundary conditions; how about the poor schmuck who throws 11 strikes and then a final 9?

[Test] public void TestHeartBreak() { for (int i=0; i<11; i++) game.Add(10); game.Add(9); Assert.AreEqual(299, game.Score); }

That works. OK, how about a tenth frame spare?

[Test] public void TestTenthFrameSpare() { for (int i=0; i<9; i++) game.Add(10); game.Add(9); game.Add(1); game.Add(1); Assert.AreEqual(270, game.Score); }

(staring happily at the green bar) "That works, too. I can't think of any more, can you.

No, I think we've covered them all. Besides, I really want to refactor this mess. I still see the scorer object in there somewhere.

OK, well, the ScoreForFrame function is pretty messy. Let's consider it.

public int ScoreForFrame(int theFrame) { int ball = 0; int score=0; for (int currentFrame = 0; currentFrame < theFrame; currentFrame++) { int firstThrow = throws[ball++]; if(firstThrow == 10) //Strike { score += 10 + throws[ball] + throws[ball+1]; } else { int secondThrow = throws[ball++]; int frameScore = firstThrow + secondThrow; // spare needs next frames first throw if ( frameScore == 10 ) score += frameScore + throws[ball]; else score += frameScore; } } return score; }

I'd really like to extract the body of that else clause into a separate method named HandleSecondThrow, but I can't, because it uses ball, firstThrow, and secondThrow local variables.

We could turn those locals into member variables.

Yeah, that kind of reinforces your notion that we'll be able to pull the scoring out into its own scorer object. OK, let's give that a try.

(grabs keyboard)

private int ball; private int firstThrow; private int secondThrow; public int ScoreForFrame(int theFrame) { ball = 0; int score=0; for (int currentFrame = 0; currentFrame < theFrame; currentFrame++) { firstThrow = throws[ball++]; if(firstThrow == 10) //Strike { score += 10 + throws[ball] + throws[ball+1]; } else { secondThrow = throws[ball++]; int frameScore = firstThrow + secondThrow; // spare needs next frames first throw if ( frameScore == 10 ) score += frameScore + throws[ball]; else score += frameScore; } } return score; }

This works, so now we can pull the else clause out into its own function.

public int ScoreForFrame(int theFrame) { ball = 0; int score=0; for (int currentFrame = 0; currentFrame < theFrame; currentFrame++) { firstThrow = throws[ball++]; if(firstThrow == 10) //Strike { score += 10 + throws[ball] + throws[ball+1]; } else { score += HandleSecondThrow(); } } return score; } private int HandleSecondThrow() { int score = 0; secondThrow = throws[ball++]; int frameScore = firstThrow + secondThrow; // spare needs next frames first throw if ( frameScore == 10 ) score += frameScore + throws[ball]; else score += frameScore; return score; }

Look at the structure of ScoreForFrame! In pseudocode, it looks something like

if strike score += 10 + NextTwoBalls; else HandleSecondThrow.

What if we changed it to

if strike score += 10 + NextTwoBalls; else if spare score += 10 + NextBall; else score += TwoBallsInFrame

Geez! That's pretty much the rules for scoring bowling, isn't it? OK, let's see whether we can get that structure in the real function. First, let's change the way the ball variable is being incremented, so that the three cases manipulate it independently.

public int ScoreForFrame(int theFrame) { ball = 0; int score=0; for (int currentFrame = 0; currentFrame < theFrame; currentFrame++) { firstThrow = throws[ball]; if(firstThrow == 10) //Strike { ball++; score += 10 + throws[ball] + throws[ball+1]; } else { score += HandleSecondThrow(); } } return score; } private int HandleSecondThrow() { int score = 0; secondThrow = throws[ball + 1]; int frameScore = firstThrow + secondThrow; // spare needs next frames first throw if ( frameScore == 10 ) { ball += 2; score += frameScore + throws[ball]; } else { ball += 2; score += frameScore; } return score; }

(grabs keyboard) OK, now let's get rid of the firstThrow and secondThrow variables and replace them with appropriate functions.

public int ScoreForFrame(int theFrame) { ball = 0; int score=0; for (int currentFrame = 0; currentFrame < theFrame; currentFrame++) { firstThrow = throws[ball]; if(Strike()) { ball++; score += 10 + NextTwoBalls; } else { score += HandleSecondThrow(); } } return score; } private bool Strike() { return throws[ball] == 10; } private int NextTwoBalls { get { return (throws[ball] + throws[ball+1]); } }

That step works; let's keep going.

private int HandleSecondThrow() { int score = 0; secondThrow = throws[ball + 1]; int frameScore = firstThrow + secondThrow; // spare needs next frames first throw if (Spare()) { ball += 2; score += 10 + NextBall; } else { ball += 2; score += frameScore; } return score; } private bool Spare() { return throws[ball] + throws[ball+1] == 10; } private int NextBall { get { return throws[ball]; } }

OK, that works too. Now let's deal with frameScore.

private int HandleSecondThrow() { int score = 0; secondThrow = throws[ball + 1]; int frameScore = firstThrow + secondThrow; // spare needs next frames first throw if ( IsSpare() ) { ball += 2; score += 10 + NextBall; } else { score += TwoBallsInFrame; ball += 2; } return score; } private int TwoBallsInFrame { get { return throws[ball] + throws[ball+1]; } }

Bob, you aren't incrementing ball in a consistent manner. In the spare and strike case, you increment before you calculate the score. In the TwoBallsInFrame case, you increment after you calculate the score. And the code depends on this order! What's up?

Sorry, I should have explained. I'm planning on moving the increments into Strike, Spare, and TwoBallsInFrame. That way, they'll disappear from the ScoreForFrame method, and the method will look just like our pseudocode.

OK, I'll trust you for a few more steps, but remember, I'm watching.

OK, now since nobody uses firstThrow, secondThrow, and frameScore anymore, we can get rid of them.

public int ScoreForFrame(int theFrame) { ball = 0; int score=0; for (int currentFrame = 0; currentFrame < theFrame; currentFrame++) { if(Strike()) { ball++; score += 10 + NextTwoBalls; } else { score += HandleSecondThrow(); } } return score; } private int HandleSecondThrow() { int score = 0; // spare needs next frames first throw if ( Spare() ) { ball += 2; score += 10 + NextBall; } else { score += TwoBallsInFrame; ball += 2; } return score; }

(The sparkle in his eyes is a reflection of the green bar.) Now, since the only variable that couples the three cases is ball, and since ball is dealt with independently in each case, we can merge the three cases.

public int ScoreForFrame(int theFrame) { ball = 0; int score=0; for (int currentFrame = 0; currentFrame < theFrame; currentFrame++) { if(Strike()) { ball++; score += 10 + NextTwoBalls; } else if ( Spare() ) { ball += 2; score += 10 + NextBall; } else { score += TwoBallsInFrame; ball += 2; } } return score; }

OK, now we can make the increments consistent and rename the functions to be more explict. (grabs keyboard)

public int ScoreForFrame(int theFrame) { ball = 0; int score=0; for (int currentFrame = 0; currentFrame < theFrame; currentFrame++) { if(Strike()) { score += 10 + NextTwoBallsForStrike; ball++; } else if ( Spare() ) { score += 10 + NextBallForSpare; ball += 2; } else { score += TwoBallsInFrame; ball += 2; } } return score; } private int NextTwoBallsForStrike { get { return (throws[ball+1] + throws[ball+2]); } } private int NextBallForSpare { get { return throws[ball+2]; } }

Look at that ScoreForFrame method! That's the rules of bowling stated about as succinctly as possible.

But, Bob, what happened to the linked list of Frame objects? (snicker, snicker)

(sigh) We were bedeviled by the daemons of diagrammatic overdesign. My God, three little boxes drawn on the back of a napkinGame, Frame, and Throwand it was still too complicated and just plain wrong.

We made a mistake starting with the Throw class. We should have started with the Game class first!

Indeed! So, next time, let's try starting at the highest level and work down.

(gasp) Top-down design!??!?!?

Correction: Top-down test-first design. Frankly, I don't know whether this is a good rule. It's just what would have helped us in this case. So next time, I'm going to try it and see what happens.

Yeah, OK. Anyway, we still have some refactoring to do. The ball variable is simply a private iterator for ScoreForFrame and its minions. They should all be moved into a different object.

Oh, yes, your Scorer object. You were right, after all. Let's do it.

(grabs keyboard and takes several small steps punctuated by tests to create)

//Game.cs---------------------------------- public class Game { private int score; private int currentFrame = 1; private bool isFirstThrow = true; private Scorer scorer = new Scorer(); public int Score { get { return ScoreForFrame(GetCurrentFrame() - 1); } } public int CurrentFrame { get { return currentFrame; } } public void Add(int pins) { scorer.AddThrow(pins); score += pins; AdjustCurrentFrame(pins); } private void AdjustCurrentFrame(int pins) { if (isFirstThrow) { if(pins == 10) //Strike currentFrame++; else isFirstThrow = false; } else { isFirstThrow = true; currentFrame++; } if(currentFrame > 11) currentFrame = 11; } public int ScoreForFrame(int theFrame) { return scorer.ScoreForFrame(theFrame); } } //Scorer.cs---------------------------------- public class Scorer { private int ball; private int[] throws = new int[21]; private int currentThrow; public void AddThrow(int pins) { throws[currentThrow++] = pins; } public int ScoreForFrame(int theFrame) { ball = 0; int score=0; for (int currentFrame = 0; currentFrame < theFrame; currentFrame++) { if(Strike()) { score += 10 + NextTwoBallsForStrike; ball++; } else if ( Spare() ) { score += 10 + NextBallForSpare; ball += 2; } else { score += TwoBallsInFrame; ball += 2; } } return score; } private int NextTwoBallsForStrike { get { return (throws[ball+1] + throws[ball+2]); } } private int NextBallForSpare { get { return throws[ball+2]; } } private bool Strike() { return throws[ball] == 10; } private int TwoBallsInFrame { get { return throws[ball] + throws[ball+1]; } } private bool Spare() { return throws[ball] + throws[ball+1] == 10; } }

That's much better. Now the Game simply keeps track of frames, and the Scorer simply calculates the score. The Single-Responsibility Principle rocks!

Whatever. But it is better. Did you notice that the score variable is not being used anymore?

Ha! You're right. Let's kill it. (gleefully starts erasing things)

public void Add(int pins) { scorer.AddThrow(pins); AdjustCurrentFrame(pins); }

Not bad. Now, should we clean up the AdjustCurrentFrame stuff?

OK, let's look at it.

private void AdjustCurrentFrame(int pins) { if (isFirstThrow) { if(pins == 10) //Strike currentFrame++; else isFirstThrow = false; } else { isFirstThrow = true; currentFrame++; } if(currentFrame > 11) currentFrame = 11; }

OK, first, let's extract the increments into a single function that also restricts the frame to 11. (Brrrr. I still don't like that 11.)

Bob, 11 means end of game.

Yeah. Brrrr. (grabs keyboard, makes a couple of changes punctuated by tests)

private void AdjustCurrentFrame(int pins) { if (isFirstThrow) { if(pins == 10) //Strike AdvanceFrame(); else isFirstThrow = false; } else { isFirstThrow = true; AdvanceFrame(); } } private void AdvanceFrame() { currentFrame++; if(currentFrame > 11) currentFrame = 11; }

OK, that's a little better. Now let's break out the strike case into its own function. (takes a few small steps and runs tests between each)

private void AdjustCurrentFrame(int pins) { if (isFirstThrow) { if(AdjustFrameForStrike(pins) == false) isFirstThrow = false; } else { isFirstThrow = true; AdvanceFrame(); } } private bool AdjustFrameForStrike(int pins) { if(pins == 10) { AdvanceFrame(); return true; } return false; }

That's pretty good. Now, about that 11.

You really hate that, don't you?

Yeah, look at the Score property

public int Score { get { return ScoreForFrame(GetCurrentFrame() - 1); } }

That -1 is odd. It's the only place we truly use CurrentFrame, and yet we need to adjust what it returns.

Damn, you're right. How many times have we reversed ourselves on this?

Too many. But there is it. The code wants currentFrame to represent the frame of the last thrown ball, not the frame we are about to throw into.

Sheesh, that's going to break lots of tests cases.

Actually, I think we should remove CurrentFrame from all the test cases and remove the CurrentFrame function itself. Nobody really uses it.

OK, I get your point. I'll do it. It'll be like putting a lame horse out of its misery. (grabs keyboard)

//Game.cs---------------------------------- public int Score { get { return ScoreForFrame(currentFrame); } } private void AdvanceFrame() { currentFrame++; if(currentFrame > 10) currentFrame = 10; }

Oh, for crying out loud. You mean to tell me that we were fretting over that. All we did was change the limit from 11 to 10 and remove the -1. Cripe.

Yeah, Uncle Bob, it really wasn't worth all the angst we gave it.

I hate the side effect in AdjustFrameForStrike(). I want to get rid of it. What do you think of this?

private void AdjustCurrentFrame(int pins) { if ((isFirstThrow && pins == 10) || (!isFirstThrow)) AdvanceFrame(); else isFirstThrow = false; }

I like the idea, and it passes the tests, but I hate the long if statement. How about this?

private void AdjustCurrentFrame(int pins) { if (Strike(pins) || (!isFirstThrow)) AdvanceFrame(); else isFirstThrow = false; } private bool Strike(int pins) { return (isFirstThrow && pins == 10); }

Yeah, that's pretty. We could even go one step further:

private void AdjustCurrentFrame(int pins) { if (LastBallInFrame(pins)) AdvanceFrame(); else isFirstThrow = false; } private bool LastBallInFrame(int pins) { return Strike(pins) || (!isFirstThrow); }

Nice!

OK, looks like we are done. Let's just read through the whole program and see whether it's as simple and communicative as it can be.

//Game.cs---------------------------------- public class Game { private int currentFrame = 0; private bool isFirstThrow = true; private Scorer scorer = new Scorer(); public int Score { get { return ScoreForFrame(currentFrame); } } public void Add(int pins) { scorer.AddThrow(pins); AdjustCurrentFrame(pins); } private void AdjustCurrentFrame(int pins) { if (LastBallInFrame(pins)) AdvanceFrame(); else isFirstThrow = false; } private bool LastBallInFrame(int pins) { return Strike(pins) || (!isFirstThrow); } private bool Strike(int pins) { return (isFirstThrow && pins == 10); } private void AdvanceFrame() { currentFrame++; if(currentFrame > 10) currentFrame = 10; } public int ScoreForFrame(int theFrame) { return scorer.ScoreForFrame(theFrame); } } //Scorer.cs---------------------------------- public class Scorer { private int ball; private int[] throws = new int[21]; private int currentThrow; public void AddThrow(int pins) { throws[currentThrow++] = pins; } public int ScoreForFrame(int theFrame) { ball = 0; int score=0; for (int currentFrame = 0; currentFrame < theFrame; currentFrame++) { if(Strike()) { score += 10 + NextTwoBallsForStrike; ball++; } else if ( Spare() ) { score += 10 + NextBallForSpare; ball += 2; } else { score += TwoBallsInFrame; ball += 2; } } return score; } private int NextTwoBallsForStrike { get { return (throws[ball+1] + throws[ball+2]); } } private int NextBallForSpare { get { return throws[ball+2]; } } private bool Strike() { return throws[ball] == 10; } private int TwoBallsInFrame { get { return throws[ball] + throws[ball+1]; } } private bool Spare() { return throws[ball] + throws[ball+1] == 10; } }

OK, that looks pretty good. I can't think of anything else to do.

Yeah, it's pretty. Let's look over the tests for good measure.

//GameTest.cs-------------------------------- using NUnit.Framework; [TestFixture] public class GameTest { private Game game; [SetUp] public void SetUp() { game = new Game(); } [Test] public void TestTwoThrowsNoMark() { game.Add(5); game.Add(4); Assert.AreEqual(9, game.Score); } [Test] public void TestFourThrowsNoMark() { game.Add(5); game.Add(4); game.Add(7); game.Add(2); Assert.AreEqual(18, game.Score); Assert.AreEqual(9, game.ScoreForFrame(1)); Assert.AreEqual(18, game.ScoreForFrame(2)); } [Test] public void TestSimpleSpare() { game.Add(3); game.Add(7); game.Add(3); Assert.AreEqual(13, game.ScoreForFrame(1)); } [Test] public void TestSimpleFrameAfterSpare() { game.Add(3); game.Add(7); game.Add(3); game.Add(2); Assert.AreEqual(13, game.ScoreForFrame(1)); Assert.AreEqual(18, game.ScoreForFrame(2)); Assert.AreEqual(18, game.Score); } [Test] public void TestSimpleStrike() { game.Add(10); game.Add(3); game.Add(6); Assert.AreEqual(19, game.ScoreForFrame(1)); Assert.AreEqual(28, game.Score); } [Test] public void TestPerfectGame() { for (int i=0; i<12; i++) { game.Add(10); } Assert.AreEqual(300, game.Score); } [Test] public void TestEndOfArray() { for (int i=0; i<9; i++) { game.Add(0); game.Add(0); } game.Add(2); game.Add(8); // 10th frame spare game.Add(10); // Strike in last position of array. Assert.AreEqual(20, game.Score); } [Test] public void TestSampleGame() { game.Add(1); game.Add(4); game.Add(4); game.Add(5); game.Add(6); game.Add(4); game.Add(5); game.Add(5); game.Add(10); game.Add(0); game.Add(1); game.Add(7); game.Add(3); game.Add(6); game.Add(4); game.Add(10); game.Add(2); game.Add(8); game.Add(6); Assert.AreEqual(133, game.Score); } [Test] public void TestHeartBreak() { for (int i=0; i<11; i++) game.Add(10); game.Add(9); Assert.AreEqual(299, game.Score); } [Test] public void TestTenthFrameSpare() { for (int i=0; i<9; i++) game.Add(10); game.Add(9); game.Add(1); game.Add(1); Assert.AreEqual(270, game.Score); } }

That pretty much covers it. Can you think of any more meaningful test cases?

No, I think that's the set. There aren't any there that I'd be comfortable removing at this point.

Then we're done.

I'd say so. Thanks a lot for your help.

No problem; it was fun.