Clean Code by Example: Throwing Exceptions in a Java Constructor

In the last post I weighted the utility of checked exceptions against unchecked exceptions. In this article I will investigate the perils of throwing exception from a constructor. As previously, I will try to stick to the guidelines given in Robert C. Martins «Clean Code». But since Joshua Bloch's «Effective Java» contributes much more to the topic of error handling than «Uncle Bob», it will be the authoritative source for this post. You can find the program code discussed here in this Github repository.

Exceptions thrown by a constructor

The architect reviewing my bowling example did not quiet like that my BowlingGame constructor threw an (checked) exception. My argument went, that it was there for mere convenience, to ease testing. With its help bowling games could be rolled wholesale, like so:

@Test
public void neitherStrikeNorSpare() {
  BowlingGame 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());
}

He countered, this could also be achieved by a normal method. And right he is! Additionally, with such a method, rolls could be added to a game in several bulks. So lets re-factor the constructor from:

public BowlingGame(int[] rolls) throws BowlingException {
  for (int roll : rolls) {
    addRoll(roll);
  }
}

into a «normal method» which the constructor calls:

public BowlingGame(int[] rolls) {
  addRolls(rolls);
}

public void addRolls(int[] rolls) {
  for (int roll : rolls) {
    addRoll(roll);
  }
}

Well, this some sleight of hand in that change. Where did the «throws BowlingException» declaration go? It already went during the last post in favor of an unchecked exception! The declaration is no longer required!

Notice something with the constructor? I did not, when I initially wrote this piece of code: the constructor called—and still calls—a non-final method! This is generally considered bad practice. If BowlingGame was sub-classed and addRoll/addRolls was overridden, then the constructor would call the sub-classes implementation although the sub-class constructor would have not been executed yet. That means the instance might not be in a usable state [Bloch, p. 89]! That makes one argument against using complex initialization within constructors: the temptation to call virtual methods.

As to further arguments against throwing exceptions in constructors—both Uncle Bob and Mr. Bloch give non. Most of the community on Stack Overflow seem to have no strong feelings against throwing exceptions from within constructors. Author and Stack Overflow veteran Jon Skeet, however, names two instances where one must be wary. The first is that the object under construction might escape immediate destruction and garbage collection:

It's possible for the «half-constructed» object to stick around though, if it's made itself visible earlier in the constructor (e.g. by assigning a static field, or adding itself to a collection).

—Jon Skeet, «Can constructors throw exceptions in Java?», Stack Overflow

The second pitfall is less exotic. It is related to resources that require clean-up:

Because the caller (usually) will have no way of using the new object, the constructor ought to be careful to avoid acquiring unmanaged resources (file handles etc) and then throwing an exception without releasing them. For example, if the constructor tries to open a FileInputStream and a FileOutputStream, and the first succeeds but the second fails, you should try to close the first stream. This becomes harder if it's a subclass constructor which throws the exception…

—Jon Skeet, «Can constructors throw exceptions in Java?», Stack Overflow

Both issues do not apply to the bowling constructor. The problem of calling a virtual method however must be fixed! There are various options: ditch the constructor altogether and call addRolls(int[]) instead on a freshly created instance. Alternatively, mark BowlingGame final thus preventing any sub-classes that could override addRolls(int[]). A little less drastic would be to just mark addRolls(int[]) and all the methods it calls recursively final`, which achieves the same effect. Or we can go for a static factory method:

public static BowlingGame fromRolls(int[] rolls) {
  BowlingGame game = new BowlingGame();
  game.addRolls(rolls);
  return game;
}

Apart from being permitted to call virtual methods, it also improves readability at the call site. In the following code the purpose of the int array is not immediately clear:

@Test
public void neitherStrikeNorSpare() {
  BowlingGame 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());
}

Replaced by a static factory method, it is clear:

@Test
public void neitherStrikeNorSpare() {
  BowlingGame game = BowlingGame.fromRolls(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());

This is clearly the best way to go, in this case.
One questions remains: what about unit-tests for the newly created addRolls(int[]) method? I guess we are somewhat absolved from this obligation since the static factory method (and before that, the constructor) are already quiet thoroughly tested. If we opted to also test addRolls(int[]) the tests would be almost duplicated, with only a method call renamed. I do not know yet, if there is a pattern to efficiently write such test-cases.

Conclusion

There is nothing principally bad about throwing exceptions from a constructor. The danger of zombie objects or unreleased resources advise more against doing overly complicated stuff inside constructors as against throwing exceptions. So does the danger of accidentally calling virtual methods from the constructor that may easily break the object.

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.

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.

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.

Modelio – a free and open-source UML modeling tool

It has been a while since I last fired up a UML CASE-tool. I hardly can remember, which one I used back then. Was it Poseidon or MagicDraw? Most of the free and open-source tools sucked back then. I was quite surprised—not pleasantly—when I saw how little matters progressed. Well, at least I found one product, which did not completely frustrate within the first fifteen minutes.

After some rather annoying experiences with (free) Eclipse UML modeling tools, like Papyrus and Umlet, I stumbled over «Modelio». To be fair, Umlet is a nice tool also, and I really like how rapid one can enter properties to a class. I failed however to create an association class, and that was that. Papyrus was a complete usability disaster. It took me about fifteen minutes to just figure out how to add a property to a class and how to name it. And even when I got it, it took a dozen or so clicks to add simple attribute to class.

Modelio, on the other hand, is rather convenient to use and produces nice looking diagrams. Properties cannot be added as rapidly as with Umlet, but the procedure is still acceptable. It provides the modeling elements I need at the moment, and that contents me.

Maybe you ask yourself, how can all of a sudden a free and open-source project blossom a quality CASE tool? Well simple, it used to be a commercial product. But now it goes the «freemium» way. The base product is free (and open-source) but more professional features, like round-trip engineering for C++, C# and (commercial) Java projects must be bought.

Unlike other freeium products or so called «community editions», Modelio appears to to be uncripled for open source Java development, including round-tip engineering, though I have not yet given it a try. It even has Hibernate support, which I will test-drive presently.

I like Modelio's business model because it gives open-source developers a professional tool at hand, while getting financed by commercial software producers, who usually can afford the license fee. Thumbs up!

Creating and filling MySQL database tables

In a previous posting I summarized the steps required to start your own private instance of the MySQL daemon. Hibernate needs a few things more than just the bare DBMS instance, however. You will need a database, at least one table in it, and a user with permission to access that table. The user is not strictly needed. The user «root» has full permission to do anything. And this is precisely the reason why to create a dedicated user.

All the stuff following is not fit for a production environment, due to the lax security precautions.

Creating a database and and user

Creating a database is very simple. Connect to your daemon:

mysql -u root mysql

This will only work if the Unix socket or IP-address and port are specified in either a configuration file or by a environment variable. Then, from within the mysql client, create a database,

create database simple;

and a user:

create user 'hibernate'@'localhost';

My user «hibernate» may only connect from the local host. He has no password set, though. I am not going to change that, as it is more convenient during development.

Then grant him full permission on the database you created earlier:

grant all privileges on simple.* to 'hibernate'@'localhost';

The MySQL manual explains the whole topic on creating and confining users in greater detail.

Filling the database

Initializing a database is a cumbersome and error-prone task. It usually involves sending quite a few SQL commands to the DBMS instance. All the repeatedly needed stuff better goes into a plain text file for re-use. By convention one with the file extension «sql». I have one sql file that creates the tables. It is named «simple_db_create_tables.sql» and looks something like this:

drop table if exists songs;
drop table if exists albums;

create table albums (
  id integer primary key auto_increment,
  title varchar(20),
  publisher varchar(20)
);

create table songs (
  id integer primary key auto_increment,
  title varchar(40),
  artist varchar(20),
  performer varchar(20),
  album integer,

  foreign key (album) references albums (id)
);

Yes, it is about music, as my Android example. Another sql script loads sample data from a text file. It utilizes the «load data» statement for that purpose. My sample data is arranged as list of comma-separated values, one file per table to be filled, e.g. in this example one for albums and one for songs. The album data file contains this:

Contains sample values for the "simple" database table "albums".
Values separated by comma. First two lines ignored.
1, Distraction Pieces, Speech Development
2, A Canticle for Leibowitz, "Blackstone Audio, Inc."

The «load data» statement is quite flexible as to the structure of the input data. My CSV files are loaded by statements like this:

load data local infile 'simple_db_albums.csv'
      replace
      into table simple.albums
      fields terminated by ','
      ignore 2 lines;

Did you wonder why the data file above explicitly sets the «id» field for each row? Should not «auto_increment» accomplish that? Well, it would. Just leave the first field empty and it kicks in. But «id» is a foreign key used by the table «songs». That means each song must give the exact id of its album. Furthermore, subsequent loading of the CSV-file would happily insert row after row of the same two albums into the database, each having a different id.

All scripts can be executed with the «mysql» command-line tool, e.g.:

mysql -u hibernate simple < simple_db_create_tables.sql

The tutorial chapter of the MySQL documentation explains creating and filling tables much more thoroughly.

Eclipse SQL Explorer

All the things above—and much more—can be accomplished conveniently from within Eclipse with the help of the Eclipse SQL Explorer extension. Installing the JDBC-driver for MySQL (or any other DBMS) is—while not overly complicated–also not exactly trivial. Mark E. DeYoung tells you what needs to be done.

SQL Explorer sucks however when it comes to executing «load data» statements, though not entirely to its own fault. The file-name given to that statement can be either an absolute or a relative path. In the former case the behavior of «load data» is pretty obvious. In the latter case however, the path is interpreted relative to the directory where the executing command—the mysql client or in this case Eclipse—has been started. With the mysql client you would simply change directory to where the data file resides, but as for Eclipse this is not within you power. Most likely Eclipse had been started with your home directory as current working directory, and this is where «load data» would resolve the relative path from. Setting the path of «load data» relative to your home directory will most likely not be viable because anyone else sharing the sql script would have to have the data file at the exact same relative path within their home directories too. For the same  reason absolute paths fail altogether.
In consequence, it means that for loading data the «mysql» command is still the best.

With tables created and filled, one can proceed to finally accessing them from a Java program. But not in this posting.

Updates

08/14/2012, 21:00 UTC Changed «grant all privileges on simple…» to «grant all privileges on simple.*…» Otherwise MySQL would interpret «simple» as a table name in the currently selected database, which would most likely be the «mysql» database.
Also, my MySQL user is no longer called «eclipse» because it is mostly not Eclipse accessing my database. Chose «hibernate» instead.

Showing Delicious booksmarks in a Blogger widget

I just added a widget to this blog which shows my recent Delicious bookmarks. My first thought was to look for a dedicated Delicious widget, but there seems to be none in Blogger's widget library. Google search lists a few rather dated do-it-yourself articles. They all stem from a time before the «new» Delicious. That is why I did not even attempt them. It is much easier to show the bookmarks with the off-the-shelf RSS-widget.

There was as slight problem however. I mark all the bookmarks relating to the topic of this blog with a «The Convalescent Coder» tag. I could not see a direct way to show the URL for just this restricted RSS-feed. The obvious appoach seemed to be to take the browsing URL which was «http://www.delicious.com/jhunovis/the-convalescent-coder» and insert «v2/rss» just before the user-name. Surprisingly this produced an empty feed—but only for tags that contain spaces. Some unsuccessful web-searches later, I simply tried to replace the dashes by escaped spaces, and—it worked. For instance, the RSS feed for my bookmarks tagged «The Convalescent Coder» is «http://www.delicious.com/jhunovis/the%20convalescent%20coder».