2017-May-15, Monday

One of my philosophies about exceptions is that they should be very specific. It's not enough to throw a generic Exception, or even a LibraryException, you should throw a specific exception for any event you want to handle separately. For instance, at a previous employer, our database layer would catch the generic exceptions (PDOException, in our case), inspect them, and re-throw as very specific exceptions (DuplicateEntry, CannotConnect, etc.).


I've formed this opinion over the years, seeing things like the following code from a real application, which decides whether a nonced token has been seen before, and rejects the token if so:

try {
  // insert ...
  return false;
} catch (PDOException $e) {
  // 23000 is SQL error code for duplicate entry
  if (23000 == $e->errorInfo[0]) {
    return true;
  throw $e;

That's ugly, and an annoying amount of boilerplate. It's much clearer if you can do something like:

try {
  // insert ...
  return false;
} catch (DuplicateEntry $e) {
  return true;

Particularly when you start getting into multiple scenarios, or start needing to support multiple database types. It can also be handy in the case of testing, since your mock can literally just throw the scenario you're trying to test.

If you're looking at that DuplicateEntry exception and wondering why it doesn't just check if the value exists first: race conditions, man.

Fun fact: that particular codebase has over half a dozen instances of catching DuplicateEntry for varying tables. Some of which are caught all the way in controllers to expose information to the user (hey, we already have an account with that e-mail address). The pain saved is very real.

Don't Wrap That Exception

This of course leads to another opinion about exception handling: libraries shouldn't wrap exceptions thrown by lower levels. At least, not in a generic, everything-is-now-a-LibraryException sense; clearly I'm okay with wrapping an exception to make it more specific.

There's a conflict of interest here: what exceptions get thrown is absolutely a part of your public API, and you want to be able to swap out the libraries you happen to be using underneath. Wrapping exceptions enables you to do that, whereas passing them through does not.

But here's the thing: in the age of Dependency Injection, your caller knows more about the exceptions thrown by the implementing object than you do. So if you're a Frobnicator, and your caller gives you a Fetcher implementation, you're actually hiding information the caller expects by catching exceptions thrown by the Fetcher and wrapping them in your FrobnicatorException. That's actively hostile to your calling code, because now to catch the exceptions they were expecting they must first catch your exception, unwrap it, then rethrow.

I guess another way of saying that is any layer in the middle should refine exceptions, not conglomerate them. Please, be mindful of which you're doing.

May 2017

14 151617181920

Most Popular Tags