Monday, September 24, 2012

Clean Code by Example: Checked versus unchecked exceptions

This post is part of a mini-series documenting my strife to improve the code quality of a small Java application I wrote. In this article I will weight the utility of checked against unchecked exceptions. I will try to stick to the guidelines given in Robert C. Martins «Clean Code». But since it does not discuss exceptions in great detail, Joshua Bloch's «Effective Java» will augment «Uncle Bob's» advises.

In the last post of this little series I focused on unit testing. In later posts I will deal with further topics of error handling, code commenting, the length of the methods, and readability issues.

Checked versus unchecked exceptions

You may remember, that during a job interview the chief architect of the company that issued the little programming exercise to me, discussed various aspects of that application with me. Regarding error handling, he specifically called my attention to the following items:
  1. The preference of checked exceptions over unchecked exceptions.
  2. The light-hearted use of exceptions.
  3. The throwing of an exception by a constructor.
  4. The apparent swallowing of an exception by the part of the application that catches it.
This post focuses only on the first two items, follow ups will cover the remainder.

The short chapter seven of «Clean Code» is dedicated to error handling. The one point it stresses with vigor is: Do not use checked exceptions!
The price of checked exceptions is an Open/Closed Principle violation. If you throw a checked exception from a method in your code and the catch is three levels above, you must declare that exception in the signature of each method between you and the catch. This means that a change at a low level of the software can force signature changes on many higher levels.
—Robert C. Martin, «Clean Code», page 107
The open/closed principle states that software modules should be open to extension and closed to modification.
When a single change to a program results in a cascade of changes to dependent modules, that program exhibits the undesirable attributes that we have come to associate with “bad” design. […] The open-closed principle attacks this in a very straightforward way. It says that you should design modules that never change.
—Robert C. Martin, «The Open-Closed Principle», 1996
Let's accept this for now. My little bowling applications has only one spot where checked exceptions originate from: the method «addRoll(int)». It is the focal point of the whole application. All the more complicated computations happen here. All other methods either serve it or feast on it. Its skeleton looks like this:

public void addRoll(int roll) throws BowlingException {
  if (isComplete())
    throw new BowlingException("No more rolls allowed.");
  if (roll < 0 || roll > 10)
    throw new BowlingException("Roll out of bounds.");
  if (mExtraRolls > 0) {
    …
  } else if (mFrameState[mCurrentFrame] != FrameState.FIRST_ROLLED) {
    …
  } else if (roll + mRolls[mCurrentRoll - 1] <= 10) {
    …
  } else
    throw new BowlingException("Frame out of bounds.");
}
It throws a BowlingException in three spots, and although one is thrown at the very end of the method—at least in terms of writing—they all signal a violations of a precondition: Either the game is already finished, i.e. all frames have been rolled, or an individual roll was out of range (either a negative number or above ten), or the sum of two rolls of the same frame exceeded ten in sum.

Violations of precondition make good candidates for unchecked exceptions, remarks Joshua Bloch in «Effective Java» (page 244), because they often indicate programming errors. Programming errors are often not recoverable. For Mr. Bloch, this is the main criterion for choosing between checked and unchecked exceptions:
Use checked exceptions for conditions from which the caller can reasonably be expected to recover. […] By confronting the API user with a checked exception, the API designer presents a mandate to recover from the condition.
—Joshua Bloch, «Effective Java», page 244
You see, he is far less rigorous about the use of checked exceptions than Uncle Bob. Joshua Bloch does not mention the breach of encapsulation as an argument against checked exceptions. He brings, however, an another argument for favoring unchecked exceptions over checked exceptions that «Clean Code» misses: In places where the programmer knows that he will not violate the preconditions, he is not forced to injure the readability of his code my polluting it with try-catch-clauses that will never trigger!

In the bowling example this would mean: if the «user interface» (i.e. main()) would check the validity of the numbers entered itself, and not merely pass them whatever-they-may-be to the BowlingGame class, then the whole try-catch-construct would be unnecessary and could disappear—if an unchecked BowlingException was thrown by «addRoll(int)»!

Unfortunately, the two heuristics «checked exceptions, if the error can and should be recovered from» and «unchecked exceptions for programming errors, like most precondition violations are» are competing in case of the bowling example. The BowlingException of «addRoll(int)» certainly indicates a violation of a precondition. But it also signals an issue that the application can and should deal with! The real problem here is not for or against checked exceptions!

Imagine we would decide to not use exceptions at all and use a different way of signaling an error condition. There are quite a few option how to do this. Returning an error code is the most common. Changing internal state that can be queried by the client is another, and using a call-back-method yet another. Would our problem change if we decided for one of these option instead of throwing an exception? I would not! The true problem is that the «user interface», i.e. the main()-method, depends on the error code for doing its job! There is no other way for it to check that a roll entered by the user is not acceptable in the running game! The application lacks an API that prevents the occurrence of errors!
One technique for turning a checked exception into an unchecked exception is to break the method that throws the exception into two methods, the first of which returns a boolean that indicates whether the exception would be thrown.
—Joshua Bloch, Effective Java, page 247
Let's see what that means for the bowling application. The main()-method that contains all the code responsible for dealing with user input, currently looks like this—never mind the German, I will fix that too:

public static void main(String[] args) {
  BowlingGame game = new BowlingGame();

  System.out.println("BOWLING! Bitte Ergebnisse eingeben!");
  int roll = 0;
  do {
    int curFrame = game.getCurrentFrame();
    int curRoll = game.getCurrentRoll();
    System.out.printf("Ergebnis für Frame %d, Wurf %d: ", curFrame,
        curRoll);
    try {
      roll = new Scanner(System.in).nextInt();
    } catch (InputMismatchException ex) {
      System.out
          .println("Bitte Ganzzahl zwischen 0 und 10 eingeben!");
      continue;
    }
    try {
      game.addRoll(roll);
    } catch (BowlingException ex) {
      ex.printStackTrace();
      System.err.println(ex.getMessage());
    }
    if (game.isSpare(curFrame))
      System.out.print("SPARE! ");
    else if (game.isStrike(curFrame))
      System.out.print("STRIKE! ");
    System.out.printf("Zwischenstand: %d%n", game.getScore());
  } while (!game.isComplete());
  System.out.printf("Endstand: %d%n", game.getScore());
}
We are after the second try-catch-clause, but notice that the first one follows the same pattern and could be fixed in the same manner by calling «hasNextInt()». After re-factoring, main() should look like this:

public static void main(String[] args) {
  BowlingGame game = new BowlingGame();
  do {
    …
    if ( game.canAddRoll(roll) ) {
      game.addRoll(roll);
    }else {
      System.out.println("This is not a valid roll!");
    }
    …
  } while (!game.isComplete());
  System.out.printf("Endstand: %d%n", game.getScore());
}

First, we turn BowlingException into an unchecked exception by inheriting from RuntimeException. Many signatures of the test-suite change due to that, as they no longer must declare to throw a BowlingException. Then we add unit tests for canAddRoll(int). Many of the tests mirror those for checking the throwing of BowlingException by addRoll(int). By now, there are so many test-cases pertaining to error handling that moving them into a separate class «ErrorHandlingTests» seems admissible. Finally, canAddRoll(int) can be implemented:

public boolean canAddRoll(int roll) {
  return !(isComplete()
       || (roll < 0 || roll > 10)
       || (!isFirstRoll() && mExtraRolls==0 &&
          (roll + mRolls[mCurrentRoll - 1] > 10)));
}

These are the precondition checks which you saw in addRoll(int) above. Their original occurrences can now be replaced by a single call to our new check function:

public void addRoll(int roll) {
  if (!canAddRoll(roll))
    throw new BowlingException(roll, mRolls);
  if (mExtraRolls > 0) {
    …
  } else if (mFrameState[mCurrentFrame] != FrameState.FIRST_ROLLED) {
    …
  } else {
    …
  }
}

Two things are noteworthy about the implementation of canAddRoll(int). First, some of its checks are still duplicated by addRoll(int), but there is little we can do about it. Second, the meaning of the boolean expression is not easy to grasp. Still, a small step towards better readability has been taken: The method «isFirstRoll()» is new. In essence it corresponds to the (negated) expression in line 6 of the previous listing. It was just too awkward to have that beast also in the already complicated boolean expression that defines canAddRoll(int). But since readability issues are not the main concern of this post, I will make no further moves in that direction.

I introduced two more subtle changes that regard error handling. The first is a more explicit documentation of the preconditions whose violation would cause a BowlingException to be thrown:

/** …
 *
 * @throws BowlingException
 *             if the roll is not within range 0–10, if the current frame
 *             would sum up to more than 10, or if the game is already
 *             complete, i.e. 10 frames have been rolled.
 */
public void addRoll(int roll) {
   …
}

Also note, that the thrown exception is documented although it is unchecked and no longer appears as part of the signature of addRoll(int). Both (non-)changes realize item 62 of Joshua Bloch's «Effective Java»: «Document all exceptions thrown by each method», because
A well-documented list of the unchecked exceptions that a method can throw effectively describes the preconditions for its successful execution.
—Joshua Bloch, «Effective Java», page 252
The second subtle change is the introduction of a parameterized BowlingException:

public class BowlingException extends RuntimeException {
  private static final String MESSAGE
    = "Cannot add roll %d to current game with rolls %s";

  public BowlingException(int roll, int[] currentGameRolls) {
    super(String.format(MESSAGE, roll,
                        Arrays.toString(currentGameRolls)));
  }
}

Both Martin (page 107) and Bloch (item 63) stress the importance of informative error messages. It helps to locate the source of an error faster. The idiom for letting the exception take care of the formatting of the error message is taken from Bloch. It further reduces clutter in the business code. The exception could also store the parameters for later evaluation by the error handler. It is mandatory in cases where the client could otherwise be tempted to parse the error message. But as this unchecked exceptions is not meant to be caught, there is little sense in storing those parameters.

One last remark. The introduction of canAddRoll(int) reduced the expressiveness of the thrown exceptions and hence the clarity of the error message the user sees if he enters a nonsensical roll. Before the change the message would say whether the roll or the frame was out of bounds, or that the game was already complete. Now it merely states, that there was an error. This certainly is no improvement, and we later we might want to mend that.

Conclusion

The preference of unchecked exceptions over checked exceptions is not backed as much by technical considerations as by programming conventions. A checked exception cries out a warning to the programmer: Something might go wrong here, and you will have to deal with it! An unchecked exception might be overlooked, until it strikes. Some people might not feel comfortable with that.

On the other hand, checked exceptions might cause a breach of the open/closed-principle. They do not improve the readability of the code over other approaches to error handling—at best! If they indicate a programming error then they might just cause unnecessary clutter to the caller code. And finally, they mandate a solution, effectively crippling the approach of preventing an error rather than detecting it. Unchecked exceptions value flexibility over safety, and this is not meant to sound spiteful. No documenting them, however, is negligence.

Tuesday, September 18, 2012

Ways of Signaling Errors by Return Value

This post summarizes the principal ways by which a method can signal its computation status (e.g. an error) to a caller—that is, by return value or by exception. Originally, I intended to do this as a short intermission in my upcoming post about re-factoring the exception handling of my bowling example to «Clean Code». Well, it grew and grew, so I decided to pull it out to keep the other article crisp.

There are basically three ways a method can signal an error—or any status—to its caller: by return value, by internal state change, or by method call. Returning a value is the most natural way, so I will focus on that. It comes in flavors. The most obvious is by means of the return statement in Java. Out-put parameters would be another way, but Java does not allow parameters to be passed out. Languages like C++ or PASCAL allow that through call-by-reference. In Java one would have to change the state of an input-parameter to archieve this effect. But that places the burden of managing this out-parameter on the caller. Hence, it is not really an option.

Yet another way for returning a value is—by throwing an exception. Exceptions try to solve two problems. The first is cramping computation result and computation status into a single return value. If a method is called merely for its side-effects, normal return values work fine. Instead of void, the methods is changed to yield a status value. The method «addRoll(int)» of the bowling example would change from:

public void addRoll(int roll) throws BowlingException {
  if (isComplete())

    throw new BowlingException("No more rolls allowed.");
  if (roll < 0 || roll > 10)
    throw new BowlingException("Roll out of bounds.");
  if (mExtraRolls > 0) {
    …
  }
}

to something like this:

public enum Status {
  GAME_COMPLETE,
  ROLL_OUT_OF_BOUNDS,
  OK
}

public Status addRoll(int roll) {
  if (isComplete())
    return Status.GAME_COMPLETE;
  if (roll < 0 || roll > 10)
    return Status.ROLL_OUT_OF_BOUNDS;
  if (mExtraRolls > 0) {
    …
  }
  return Status.OK;
}

While the exception is now gone, the code on the caller site is not going to look better. The try-catch-construct that mars the «main()»-method of the bowling example would have to be replaced by switch-case or if-then-else.

In cases where the method does not use all values of its co-domain–well, it is not swell but still viable to encode the return state into the unused values. In the bowling example, the method «getScore()»—if it would have to signal errors—could use any number of type int except for the range between 0 and 300. A negative number could signal an error, for instance. Constants have to be defined (and used!) for those error states or things might get out of hand. Readability of the code on the caller site suffers, however, by the additional state parsing.

With a bit-mask, both a value and a status can be encoded into a single integer. It is a quite common practice, especially in low level functions or on devices where memory is rare. Android does it in various places, for instance with «MotionEvents». As not only checking the state but also decoding the return value is required by the caller, readability suffers even more. Also debugging gets no easier. Defining special composite return types or using general collections less obfuscates the structure of the return value, but decoding and checking on the caller site will not go away.

As said before, exceptions address two problems. The first was coercing computation status and result into a single return value. This no longer happens, and decoding becomes unnecessary. The second is checking the state. It now happens in a code path altogether different from normal execution. Still readability is not great, because—as aspect-oriented programming would say–two concerns are intermingled that cannot clearly be expressed in the linear structure of source code.

Readability-wise it does not really matter whether to signal errors by return value or by exception. The caller has to treat the error-states separately from the normal return values and to act upon it. This might shroud the business logic of the caller. The best solution to this, it seems, is to avoid errors altogether! I will focus on this—among other things—in my next post.

Updates

09/18/2012: Use tags in references to the Github sources.

Friday, September 7, 2012

Clean Code by Example: Unit Tests and Mindfulness

In this posting I going to do a bit of a code review. I am trying to apply the guidelines given in Robert C. Martin's «Clean Code» to spot problems in a little sample application of mine, and then try to fix them.

Only recently I applied for a job at a major financial service provider that has its headquarters in the heart of Berlin. It is a real pearl, and I am in high hopes still to get hired. (Update 09/18/2012: On the evening of the very same day I posted this article, I received a phone call that I would not get the job—bummer!) Part of their rather thorough procedure was a little programming challenge. The assignment was to compute the overall score for a bowling game. In an afterward interview, the chief architect of the department went through the code with me. He pointed out spots that could be improved, and in many of them I agree.

The developers of that financial service provider follow Robert C. Martin's book «Clean Code» as a coding guideline. I did not know it at the time, and so, I guess, it is high time I read it now. In this code review I will follow both the comments of the chief architect and the principles of «Uncle Bob»—which may overlap. Also, I will try to mend those parts which I do not like about my code—there are a few.

The program code that went with my job application you can find on Github. For those not familiar with the scoring rules of ten pin bowling, Wikipedia has a sum-up.

The chief architect focused on the following issues. Maybe there were more, but these I remember:
  1. Missing dependencies.
  2. Test-cases are too large.
  3. Test-cases contain redundant code.
  4. Something is fishy about the exceptions thrown. (Posted on 09/24/2012)
  5. There are too much comments.
  6. There are too few comments.
  7. Some methods or to long,
  8. Some methods are hard to understand.
I ordered them by increasing complexity and difficulty to fix. Some of them are related, like the second or fourth, or the last four items. I will try to recollect the point the chief architect made about them. I also will rationalize—reason I mean—why I programmed them that way in the fist place. In this blog posting I will only cover the first three items, covering the other in later articles.

Missing dependencies

This is not really a code issue, but it is an issue nonetheless. The chief architect complained that my solution depended on external libraries (JUnit here), but neither did I package them with my solution nor did I provide a way to easily satisfy the dependency, like a Maven project. The odd fact is, first thing I did, when I had my solution ready, was to put it in a Maven project. Before that, Eclipse did all the magic for me. But I was familiarizing myself with Maven at the time, so it was the natural thing to do.

I deliberately did not send the Maven project along with the source code, and that is the real problem. It is a problem specifically if you are in for an employment, mind you. Because in such a situation you might want to consider, what the employing party is expecting of you. Also you certainly do not want to annoy them in a task they are actually doing for you, like evaluating your working samples. I failed in both aspects, because I neither minded the small but avoidable trouble the missing libraries would cause. Nor did I reason, that a professional Java programmer would be familiar with the prevalent build tool of his field. My line of thinking went more in the direction of avoiding a zip-archive that contained considerably more directories than files, which was the first thing I disliked about Maven in the beginning (but got fairly quick used to).

There nothing to fix here—anymore. So let's go on to the real stuff.

Test-cases are too large and are full of redundant code

Have a look at BowlingGameTest.java. It contains a few short test-cases but also three test-cases that are between 40 and 70 lines long, the largest containing 27 assertions!

When I was asked, why they were so long, I started blabbering something about Python doc-tests and telling stories with a test-case, and non-sense like that. I was taken off my guard; then trying to make up some reasons on the spot because I could not remember or reconstruct my original line of thinking. A better reaction in an job interview would be to either admit that you see the point, or alternatively be honest and say that it seemed reasonable then, but you cannot recollect, why, now.

In truth, I was too lazy to split the test-cases into separated entities. This would have meant to come up with meaningful names. And that would have taken time. Instead I grouped related tests under one name.

The problem with such bloated test-cases is, that you are never sure how many tests are actually failing. Because with JUnit test-cases it is all or nothing. As soon as the first assertion of a test-case is violated the whole test-case will be aborted. You will never now, whether the following assertions would hold, or not. This is not a problem if the assertion are strongly related and test a single aspect of the program. In an aggregated test-case this is rarely the case. Look at this:

@Test
public void gameProgress() throws BowlingException {
  BowlingGame game = new BowlingGame();

  for (int i = 0; i < 9; i++) {
    assertEquals(i + 1, game.getCurrentFrame());
    assertEquals(1, game.getCurrentRoll());
    game.addRoll(2);
    assertTrue(!game.isComplete());

    assertEquals(2, game.getCurrentRoll());
    game.addRoll(3);
    assertTrue(!game.isComplete());
  }

  assertEquals(10, game.getCurrentFrame());
  assertEquals(1, game.getCurrentRoll());
  game.addRoll(4);
  assertTrue(!game.isComplete());

  assertEquals(2, game.getCurrentRoll());
  game.addRoll(5);
  assertTrue(game.isComplete());

  // 10th frame is spare
  game = new BowlingGame();
  for (int i = 0; i < 9; i++) {
    game.addRoll(2);
    game.addRoll(3);
  }
  assertEquals(10, game.getCurrentFrame());
  …
}

…and so on for forty odd lines. There is, of cause, the problem of poor intelligibility. What does the first for-loop up to the comment actually test for? But I will go into this matter later. Clearly, the loop after the comment tests a completely independent aspect, as it creates a whole new BowlingGame object. And thanks to the comment the reader might guess what it actually does. Without it, the reader has to figure out the meaning of the test for himself. But that's the job of good, succinct method name, right?

Let's see what Uncle Bob has to say about the matter. Chapter 9 of «Clean Code» is dedicated to unit testing. Two rules apply for certain: minimize the number asserts per test (he is not advertising strict one-assert-per-test discipline), and a single concept per test. Fewer assertions make «those tests come to a single conclusion that is quick and easy to understand» (page 130). The single concept rule states basically what I wrote in the last paragraph.

So let's split the huge methods up, starting with «invalidRolls()», which originally looked like this:

@Test
public void invalidRolls() {
  BowlingGame game;
  boolean caught;

  game = new BowlingGame();
  caught = false;
  try {
    game.addRoll(-1);
  } catch (BowlingException x) {
    caught = true;
  }
  if (!caught)
    fail("A bowling exception should have been thrown.");

  game = new BowlingGame();
  caught = false;
  try {
    game.addRoll(11);
  } catch (BowlingException x) {
    caught = true;
  }
  if (!caught)
    fail("A bowling exception should have been thrown.");

  game = new BowlingGame();
  caught = false;
  try {
    game.addRoll(2);
    game.addRoll(9);
  } catch (BowlingException x) {
    caught = true;
  }
  if (!caught)
    fail("A bowling exception should have been thrown.");

  game = new BowlingGame();
  caught = false;
  try {
    game.addRoll(2);
    game.addRoll(-2);
  } catch (BowlingException x) {
    caught = true;
  }
  if (!caught)
    fail("A bowling exception should have been thrown.");
}

This one shows with in-your-face directness that laziness and ugliness go hand in hand. Each individual test-case of the four aggregated ones expects an BowlingException to be thrown. JUnit test can express this intend explicitly by annotating the test-case with «@Test(expected=BowlingException.class)». The aggregate test-case cannot make use of this because the first thrown exception would successfully terminate the test-case leaving all the following code un-executed. Refactoring the four kinds of invalid rolls into separate methods will not only make their nature more obvious, it will make the test-case a great deal shorter and more to the point, as the whole exception-catching and flagging construct disappears.

I started to extract the methods using the equally named refactoring tool of the Eclipse IDE, but so much code had become unnecessary that I just did one test-case and copied it three times. The result is a stunning improvement:

@Test(expected=BowlingException.class)
public void invalidRollBelowZero() throws BowlingException {
  BowlingGame game = new BowlingGame();
  game.addRoll(-1);
}

@Test(expected=BowlingException.class)
public void invalidRollAboveTen()  throws BowlingException {
  BowlingGame game = new BowlingGame();
  game.addRoll(11);
}

@Test(expected=BowlingException.class)
public void invalidSecondRollBelowZero() throws BowlingException {
  BowlingGame game = new BowlingGame();
  game.addRoll(2);
  game.addRoll(-2);
}

@Test(expected=BowlingException.class)
public void invalidFrameAboveTen()  throws BowlingException {
  BowlingGame game = new BowlingGame();
  game.addRoll(2);
  game.addRoll(9);
}

Next up is «gameInProgress()», part of which you already saw above. Again the code is highly redundant. First, nine frames are rolled in a loop without spare or strike. The last frame is special, as a spare or a strike allows for extra rolls to compute the bonuses. A strike, a spare, or none of the former make it three test-cases in total. Splitting by «extract method» is straight forward, yielding three almost identical tests, that look like the following:

@Test
public void tenthFrameIsStrike() throws BowlingException {
  BowlingGame game;
  game = new BowlingGame();
  for (int i = 0; i < 9; i++) {
    game.addRoll(2);
    game.addRoll(3);
  }

  assertEquals(10, game.getCurrentFrame());
  assertEquals(1, game.getCurrentRoll());
  game.addRoll(10);
  assertTrue(!game.isComplete());

  assertEquals(10, game.getCurrentFrame());
  assertEquals(2, game.getCurrentRoll());
  game.addRoll(10);
  assertTrue(!game.isComplete());

  assertEquals(10, game.getCurrentFrame());
  assertEquals(3, game.getCurrentRoll());
  game.addRoll(10);
  assertTrue(game.isComplete());
}

One of them differs a bit as it also makes some assertions during the rolling of the first nine frames. The latter test-cases do not do this as the executed code would be exactly the same. Clearly, the former test-case violates the «one concept per test-case» rule, as it test first the correct filling of the first nine frames, and only then its advertised concept, the rolling of the final frame. Resolving this puts us in a tight spot between minimizing redundancy and having one concept per test-case, because these are the options I see:
  1. Make the rolling of the first nine frames a separate test-case. That would solve the one-concept-per-test-case-issue. Still, rolling nine frames is required before testing the rolling of the tenth frame. Which would indicate to…
  2. isolate the rolling of the first nine frames as an utility operation. This utility cannot be a test-case itself. This would violate the I of the F.I.R.S.T. virtues of unit-testing that «Clean Code» declares: Fast, Independent, Repeatable, Self-Validating, and Timely (pp. 132). Independent means, no test-case must depend on another. This again implies that we have a test-case and a utility operation, both having virtually the same code. Which throws us into redundancy hell. The third option would be to…
  3. add some assertions to the utility operation, call it from a test-case that covers the rolling of the first nine-frame, and also use it to prepare the testing of the tenth frame. The price to pay for this is the overhead introduced by implicitly testing the rolling of the first nine frames over and over again.
But what is life without ambivalence? Hence, I went for option three as the lesser of the evils, and ended up with code like this:

protected BowlingGame rollNineFrames() throws BowlingException {
  BowlingGame game = new BowlingGame();
  for (int i = 0; i < 9; i++) {
    assertEquals(i + 1, game.getCurrentFrame());
    assertEquals(1, game.getCurrentRoll());
    game.addRoll(2);
    assertTrue(!game.isComplete());

    assertEquals(2, game.getCurrentRoll());
    game.addRoll(3);
    assertTrue(!game.isComplete());
  }
  assertEquals(10, game.getCurrentFrame());
  assertEquals(1, game.getCurrentRoll());
  return game;
}

@Test
public void firstNineFramesNoStrikeNoSpare() throws BowlingException {
  rollNineFrames();
}

@Test
public void tenthFrameIsStrike() throws BowlingException {
  BowlingGame game = rollNineFrames();

  game.addRoll(10);
  assertTrue(!game.isComplete());

  assertEquals(10, game.getCurrentFrame());
  assertEquals(2, game.getCurrentRoll());
  game.addRoll(10);
  assertTrue(!game.isComplete());

  assertEquals(10, game.getCurrentFrame());
  assertEquals(3, game.getCurrentRoll());
  game.addRoll(10);
  assertTrue(game.isComplete());
}

Now, should the tests that cover the rolling of the final frame be split up further? Uncle Bob advises so, because they still cover more than one concept. The first is the way the final frame is rolled: without clearing all pin, with a strike, or with a spare. The latter two continue the game. The second concept is the correct handling of the bonus rolls.

But is the splitting mandatory? I would argue, it is! But only if following a strike, spare, or none of the former, there would be more than one test-relevant way to roll the bonus balls. As a matter of fact, I realized there is at least one: after a spare in the last frame, a following «strike» should not allow for another bonus roll.

Dutifully I extracted three more utility operations for rolling ten frames: with the last frame being either a spare, a strike, or none of them.

Again the utility methods make assertions. That bothers me for the following reason. In their current form, the utilities test whether the game is complete, which frame is rolled and which roll within a frame is due. Now suppose, we decided that we would like to check the game score as well. Computing the game score is the most expensive operation of the application, and now it would be called again and again, most of the time testing the very same aspect of the application. Admittedly, the scoring function is not really expensive in computation complexity terms, in fact its complexity is O(1), i.e. constant time. But suppose it was not cheap, what then? We would violate the F of the F.I.R.S.T. virtues: fastness. I see no generally viable way out of this dilemma, so I let it rest for now.

Only, two test-cases are left to go. First I split «strikesAndSpares()» in two, right at the «And». This test-case checks the valid flagging of frames as strike or spares. Then I realized there is a third concept tested: neither spare nor strike was rolled. That made it three test-cases, that look like this one:

@Test
public void frameIsSpare() throws BowlingException {
  BowlingGame game = new BowlingGame();

  game.addRoll(5);
  game.addRoll(5);
  assertTrue(game.isSpare(1));
  assertTrue(!game.isStrike(1));
}

The last test-case «score()» feeds a list of rolls into a game. Then it checks where the score is computed correctly:

@Test
public void score() throws BowlingException {
  BowlingGame game = new BowlingGame(new int[] { 10, 10, 10, 10, 10, 10,
      10, 10, 10, 10, 10, 10 });
  assertEquals(300, game.getScore());

  game = new BowlingGame(new int[] { 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0,
      0, 0, 0, 0, 0, 0, 0, 0 });
  assertEquals(0, game.getScore());

  game = new BowlingGame(new int[] { 1, 2, 3, 4, 5, 1, 2, 3, 4, 5, 1, 2,
      3, 4, 5, 1, 2, 3, 4, 5 });
  assertEquals(60, game.getScore());
  …

Eleven combinations of rolls are tested altogether. Again, multiple, though admittedly related, concepts are merged into one test-case. Again, comments are required to signify the role of the rolls. Again, the first failing assert will veil the outcome of the following. Nurse, scissors!

@Test
public void scoreAllZeroLastFrameStrikeExtraRollsStrikes()
    throws BowlingException {
  BowlingGame game = new BowlingGame(new int[] { 0, 0, 0, 0, 0, 0, 0, 0,
      0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 10, 10, 10 });
  assertEquals(30, game.getScore());
}

@Test
public void midGameScoreInterruptedStrikeStreak() throws BowlingException {
  BowlingGame game = new BowlingGame(new int[] { 10, 10, 10, 5 });
  assertEquals(75, game.getScore());

The resulting eleven test-cases are crisp and short, but not without flaw. The names are awful! They all start with either «score» or «midGameScore». Clearly this is grouping prefix. Classes lend themselves much better for grouping, and since the BowlingGameTest has «grown» from five to twenty eight methods, it seems sensible to break the class up.

I created two classes «CompleteGameScoringTests» and «IncompleteGameScoringTests» and moved the methods by drag-and-drop in the Eclipse package explorer. The method names are much more readable now:

public class IncompleteGameScoringTest {

  @Test
  public void interruptedStrikeStreak() throws BowlingException {
    BowlingGame game = new BowlingGame(new int[] { 10, 10, 10, 5 });
    assertEquals(75, game.getScore());
  }
  …

Done! It is quite possible than further splitting of the BowlingGameTest class would be wise, but I defer this until later, when I know more about «Clean Code.»

Conclusion

Applying the rules of «Clean Code» to the test-cases of even such a simple piece of code as mine, already greatly improved its readability—and yes, its beauty. But there is further room for improvement. Because Robert C. Martin states that the most important thing about automated testing is that the tests are first-class citizen, just like the application code is, and must be kept clean by the same high standards (page 124). That's why I will frequently revisit my tests during the course of cleaning up my Bowling example.