Stories about Software


TDD For Breaking Problems Apart 3: Finishing Up

Last time, we left off with a bowling score calculator that handled basic score calculation with the exception of double strikes and the tenth frame. Here is the code as of right now for both classes:

Without further ado, let’s get back to work. The first thing I’d like to do is actually a refactor. I think it would be more expressive when creating strikes to use a static property, Frame.Strike, rather than new Frame(10, 0). Since the strike is completely specific in nature and a named case, I think this approach makes sense. So the first thing that I’m going to do is test that it returns a frame where IsStrike is true:

(This is actually what the test looked like after two red-green-refactors, since the first one was just to define Frame.Strike). At this point, I now have a static property that I can use and I’m going to find everywhere in my score calculator and frame test classes that I queued up a strike and use that instead as part of this refactor cycle. While I’m at it, I also demote the visibility of Frame.Mark, since I realize I should have done that a while ago. The Mark constant isn’t needed outside of Frame since Frame is now expressive with IsStrike, IsSpare and Total. Strictly speaking, I should conceive of some test to write that will fail if Mark is visible outside of the class, but I try to be pragmatic, and that’s a screwy test to have and persist.

Now, let’s get down to brass tacks and fix the double strike issue. If I bowl a strike in the first frame, another in the second frame, and then a 9 in the third frame, my total score should be 57 in the third (29+19+9). Let’s write such a test:

How to get this to pass… well, I’ll just tack on an extra frame.FirstThrow if the last two were strikes:

… and NCrunch gives me green. Now, let’s make the class a little nicer to look at:

And, that’s that. Now we have to think about the 10th frame. This is going to be interesting because the 10th frame is completely different in concept than the other frames. The 10th frame’s total can range up to 30 instead of being capped at 10, and if you get a strike in the first frame or spare in the second frame, you get three throws instead of two. How to model this with what we have… add a new property to the frame class called “ThirdThrow”? That seems reasonable, but what if we populate the third throw when we’re not in the 10th frame? That’s no good — how can we know that a frame is a 10th frame? We’ll probably need a boolean property called IsTenthFrame… right?

Wrong! (At least in my opinion). That amounts to adding a flag that clients look at to know how to treat the object. If the flag is set to true, we treat it like one kind of object and if it’s set to false, we treat it like another kind. This is a code smell in my opinion — one that I think of as “polymorphism envy” or “poor man’s polymorphism”. This is a milder version of the kind you usually see which is some ObjectType enum that clients switch over. We don’t have that (yet) because we only have two values.

So if we’re contemplating a polymorphism envy approach, it stands to reason that maybe what we’re nibbling at is, well, actual polymorphism. Maybe we should have a TenthFrame class that derives from Frame and overrides important functionality. I don’t know that this is the solution, but TDD is about solving small problems incrementally, so let’s start down this path and see where it leads. We don’t need all of the answers this minute. The first thing to test is probably going to be that total is the sum of the three constructor arguments:

As I wrote this test, two things didn’t compile. The first was the instantiation of the TenthFrame, which I solved by declaring it. The second was the Total property, which I solved by inheriting from Frame. That actually turned out to be an easier way to get a red test than declaring the property (and more productive toward our design). Then to get the test passing, the easiest thing to do was make Frame’s total virtual and override it in TenthFrame. So, pretty quickly we seem to be getting Total right:

Now we need to start tweaking the business rules. Parent is going to throw an exception if we initialize frame 1 and 2 each to a strike, but that’s fine in the 10th frame. Here’s a failing test:

To get this passing, I declare a default constructor in the base (to make the compiler happy) and have the new class’s constructor implement its own assignment logic to avoid the checks in parent that cause this failure. But now that simple assignment is restored, we need to implement our own rules, which will include throwing exceptions for throws greater than ten or less than zero, but it will also include oddballs like this that I don’t yet know how to describe:

This is another one of the real draws of TDD. I don’t know what to call this or how to categorize it, but I have an example, and that’s all I need to get started. I just have to make this pass:

I could have just tested for the specific literals in the test, but I didn’t feel the need to be that obtuse. You can really control your own destiny somewhat with “simplest thing to make the test pass”. IF you have no idea what direction the design should take, maybe you go that obtuse route. If you have some half-formed idea, as I do here, it’s fine to get a little more business-logic-y. I’m going to dial up another case that should fail because of the relationship between second and third frame and take if from there:

Now I have the following production code to get it to pass:

But, that’s getting a little fugly, so let’s refactor:

Ah, a business rule is starting to emerge. In general, if a throw is not a mark, then it and the subsequent throw can’t be greater than 10. Hey, come to think of it, that sounds right from my bowling experience. They only reset the pins if you knock ’em all down. Of course, we have one final rule to implement, which is that if the first and second throws don’t knock all the pins down, there is no third throw. I’ll leave that out, since it’s pretty easy.

Now the time has arrived for some integration testing. I found a site that has some example bowling scores, and I’m going to code one up:

If you’re following along, you’ll see green in NCrunch. Let’s try one more:

Oops. Red. So, what happened? Well, I went back through game, setting temporary asserts until I found that things went off the rails following the third strike in a row. I then looked in my class at the logic following the two strikes and realized it wasn’t quite right:

I commented out the mistake and put in the correct code, and the entire test suite went green, including the integration test for Kim. I think this is a good note to close on because the tests are all passing and I believe the calculator is functioning (here it is on gist if you want to check out the final product) but also because I think there’s a valuable point here.

TDD is a design methodology — not a guarantee of bug free code/comprehensive testing strategy. I experienced and even blogged about writing code for days using TDD and assembling all the parts and having it work flawlessly the first time, and that did happen. But I realized that even with that, the happy and happy-ish paths were the ones that worked. Here, I had a bowling calculator that made it through all individual scoring tests and even an entire non-trivial game going green before we teased out a case in smoke testing where it went red.

TDD will help you break problems into small pieces to solve (the whole point of this series of posts), ensure that your code is testable and thus loosely coupled and modular, ensure that you don’t push dirty mop water around the floor by breaking functionality that you had working before, and generally promote good code. But think about this — those are all productive programming concerns rather than testing concerns. You still need testers, you still need edge case unit tests once you’re done with TDD, and you still need integration/smoke tests. The fact that TDD produces a lot of tests that you can check in and continue to use is really just a bonus.

And it’s a bonus that keeps on giving. Because let’s say that you don’t agree with my decision to use inheritance for tenth frame or you think strike would be a more appropriate descriptor of a throw than of a frame. With all of my TDD test artifacts (and the integration tests) in place, you can rip my internal design to pieces without worrying that you’re going to break the functionality that this provides to clients. And that’s incredibly powerful for allowing fearless refactoring and maintenance of this code. So do it to break your problems apart and keep yourself moving and productive, and keep the tests around to make sure the code stays clean and trends toward improvement rather than rot.

Full Code

  • Pingback: Introduction to Unit Testing Part 3: Unit Testing Sucks | DaedTech()

  • I keep rereading this series to more closely examine your process. During one of my rereads I came up with a question.

    The question is about an alternate design for the score calculator. It would be too difficult to explain, so I modified your code to show you what I’m thinking. I put the changed files up on gist if you’d like to see: https://gist.github.com/TheSecretSquad.

    I’m not in any way advocating my design over yours. In fact, I have the usual feeling that there is something blatantly bad about my design. I’m trying to get a better “design sense”, so I’d like to get your opinion on what about this alternate design is obviously bad or good.

    In my alternate design, the Frame class takes on most of the responsibility, and the BowlingScoreCalculator simply keeps track of the total. In your design the BowlingScoreCalculator is much more involved with the Frame class. How do you justify putting these responsibilities in one place or the other? I feel like I have no mental measurement for this.

    Is it simply a matter of semantic differences in the public interfaces of the classes? Your design says score calculation is based on: previous frames, the current frame’s throws, “strike-ness”, and “spare-ness”. My design says score calculation is based on: current frame in relation to previous frames, and any number of other attributes the Frame is constructed with, including but not limited to the aforementioned attributes of the current frame.

    I also have a question regarding getters and setters. Ever since I read about “the evils of getters and setters”, I’m wary of any method that returns something. I can’t help but wonder if I’m exposing implementation details. For example, the FirstThrow and SecondThrow methods of the Frame class. They get and set values of the same primitive types you were trying to remove, and aren’t indicative of “behavior”. I can’t help but wonder if they expose implementation details. At the same time, those properties seem reasonable.

    … …Or, am I overthinking everything?

    I hope I didn’t throw too much at you at once. Thanks for your time.

    • I’m thinking that I might write a post about this concept, but I’ll see how involved the comment gets 🙂 Also, as kind of disclaimer, this is code I wrote a year and a half ago and haven’t really looked at or thought about much since.

      On the subject of how I divide class responsibilities, what I try to do is let myself be guided by the “Principle of Least Surprise.” Often, this is achieved by writing classes that mimic real world objects along with their behaviors and dependencies. A “Car” class has an “Engine” it starts when a client asks the car to start or something along those lines. A second guiding principle for me is to come up with rules about responsibility division and dependencies that are easy to remember. For instance, I might declare up front that “Car knows about engine but not vice-versa and anything that physically happens inside the engine will be a private member of the engine class.” This is sort of a corollary of the Principle of Least Surprise in that the goal is to make it so that maintenance programmers (be they others or myself) have good heuristics about where to look for behaviors and where to define new ones.

      So, I imagine what was going through my head during my design was “frame has two throws in it, and there is a special kind of frame for the 10th frame.” Also going through my head was probably “bowling game would know about all of the frames, but frames know nothing about the game.” This sounds a little weird, but now you create a world in which it is possible to have a “practice frame” or some other kind of bowling activity outside of a game. And, I’ve spent enough years as a software architect to know that a project manager is going to say at the next meeting “what about practice frames?” (I’m kidding in the context of example code, but it’s dead serious in terms of the way I approach software). So a design point I want to avoid is frames understanding/depending on a game or other frames.

      But there’s really no right answer nor is there any reason your design isn’t just as valid. A lot of software design is a matter of trying to anticipate what the users may need later and making sure you’re not creating a design where that’s difficult or dangerous (but beware of confusing this goal with trying to gold plate or bake in things they don’t currently need because you think they might later). There’s generally a lot of subjective ‘feel’ and experience to it, which is what, I think, makes the Craftsmanship/Guild metaphor so appealing to a lot of people in the industry.

      As for the getter methods/properties, I don’t really view these as evil. I think there’s maybe a more specific problem when it comes to this, which I can describe with an example. Going back to Car/Engine, let’s say that I have a method on car where I say if(!Engine.IsCold) Engine.Start(). This is a little icky in that I’m querying Engine about its state and then, depending on the answer, modifying its state. Why not just have a one liner: Engine.Start() and let engine contain its own decision logic for starting. But, if I have if(!Engine.IsCold) OnBoardComputer.ShowStartOption() there’s nothing wrong with the getter here. Someone has to query about this state outside of Engine because showing things to the car operator isn’t the job of Engine. If you can get away from having any state in your application, you can dispense with getters (and have code that’s very easy to reason about, such as math problems), but in the real world, applications have state and we have to manage it. So we have to live with state queries.

      One last point — primitive obsession isn’t about having primitives. It’s about having groups of primitives instead of types. So, there’s nothing wrong with having int types for throw 1 and throw 2 and even exposing ints for them. The problem comes when you have methods all over your code base that take 2 ints as a paramter instead of 1 frame. It’s a problem because (1) it’s a maintenance liability if you ever want to add more information to the frame because you have to go add yet another primitive to all of those method signatures and (2) it makes it hard to return a frame since you’re just representing int as 2 free-floating integers. You’d have to do something weird like return a tuple or use out parameters.

      • I couldn’t ask for a more coherent answer. I’m considering framing this and hanging it on the wall. Thank you.