Heikniemi Hardcoded

Code quality, part II: Proper exception handling

One critical part in good-quality code is the way it handles errors. Error handling is very rarely fun to write, but nevertheless, it's one of the things that your code gets judged by. Or, rather: proper error handling rarely gets praise from the end user, but the lack of one causes lots of irritation. The correct approach to error handling is, naturally, very dependent on the nature of the application and the tools used. The following assumes having an exception propagation mechanism (the try/catch/finally constructs available in most modern languages).

The fundamental problem can be formulated like this: The sum of your ability to pinpoint the exact source of the error and your ability to react to it meaningfully is constant. When you're deep down in your call hierarchy, perhaps executing a SQL statement for saving the state of some business object of yours, you can easily tell what happened and where by examining the exception. However, what can you do about it? Retry perhaps, but even that may not be correct at times. You shouldn't pop up dialogs or write to the console either - you don't know the conditions you're running under, and the UI is the caller's responsibility anyway. Well, you let the exception propagate upstream.

Now, you're back in the UI calling code (let's say a button click handler). The exception arrives from your business logic library. Now what? At this point, it's easy to give a meaningful error message ("Saving the foobar failed because of a database error") - but giving useful advice for the next step is harder. Should the user retry? What is the likely cause for the error? Some applications dump the provider error message (such as the SQL Server error, usually available through the exception object), but they're pretty likely to be extremely confusing to the user, and possibly even in the wrong language.

Wrapping your exceptions

It's obvious that just rethrowing the technical exceptions doesn't give you many good choices at the UI level. One reasonable alternative is packaging all the additional information you have into a custom exception class whose InnerException property (something like this exists in every class library) contains the original, technical error message. This way, you can go into extreme lengths of providing context information. The example below illustrates the concept (pseudocode very reminiscent of C#).

public enum DBOperationType { InsertFoobar, UpdateFoobar, DeleteFoobar }

public class MyDBException : Exception 
{
  public enum DBOperationType FailedOperation;
  public bool ShouldRetry;
  public string SqlStatement;
}

// inside a class
...
public void UpdateFoobar(Foobar f) 
{
  string sql = CreateFoobarUpdateSql(f);
  try 
  {
     ExecuteNonQuery(sql);
  }
  catch (DBException d) 
  {
     MyDBException md = new MyDBException();
     md.InnerException = d;
     md.SqlStatement = sql;
     md.FailedOperation = DBOperationType.EditFoobar;
     md.ShouldRetry = IsTimeoutOrDeadlockVictimError(d);
     throw md;
  }
}

Now, you'd have considerably more information at your disposal when catching a MyDBException in your UI code. You can access the original DBException through the InnerException property.

Not catching what you don't understand

Another important rule: Never catch what you don't understand - or at least rethrow it. The code above only catches DBExceptions. This is usually desired, as catching everything á la catch (Exception e) { ... } very easily hides errors. For example, a NullReferenceException caused by one of your code bugs could be nastily hidden by a catch-all exception statement. For the example above, ExecuteNonQuery is unlikely to cause such situations, but for the typical "executereader and loop through, constructing objects into an arraylist" is a totally different case with vast error possibilities.

Of course, if you just analyze some aspects of the error and then rethrow it, there's no problem in catching everything. But don't make the mistake of assuming all errors are acceptable even if some are. I've seen quite a few examples where the coder had thought something like "Writing this log entry to the DB is non-critical - thus it's OK if I just swallow all the timeout exceptions with an empty catch block". Yeah right - one day somebody breaks your logging SQL, sending you into a situation where nothing _ever_ gets logged and you have no way to find out (until you - or the end user - look at the database). Point being: Even swallowing all exceptions of a certain class isn't usually a good idea! Something like the following may be acceptable, though:

try { ... }
catch (DBException d) 
{
  if (!IsTimeoutException(d))
    throw;
}

All that said, it's of course a good idea to catch all exceptions at the topmost level of your code thread (usually the UI code), as not doing so may result in an abnormal termination of the program. Even then, silently swallowing exceptions is a bad move. It almost always bites you back.

Don't waste the trace

The stack trace is fundamental to the debugging process. The trace shows where the error came from, and with the proper tools, it can also tell a lot about the program flow. However, there's a caveat. In C#, using the throw statement works a bit differently from f.e. Java (but like C++). When you've done catch (Exception e), there's a difference between throw; and throw e;. They both result in a very similar exception being thrown, but with one key difference: the parameterless throw ("rethrow") preserves the stack trace, while throwing the same exception again makes it look like the exception originated from this method.

It's extremely rare for you to actually want to mask your exception's true source, although it is theoretically a good idea for encapsulation purposes. For example, a public method of a tool library might want to hide the internal implementation details - after all, they don't belong to the caller. In this case, catch (Exception ex) { throw ex; } would do the trick. Anyway, it's not worth it most of the time - the harm made for debugging is too big. When you receive your first bug reports from the end users you'll regret the hiding. Obfuscation works better.

Most cases of "throw ex" are the result of programmers not knowing about the parameterless throw statement. Even when used intentionally, I consider it an anti-pattern that should ring your warning bells.

Some personal implementation notes

Proper exception management is somewhat cumbersome. Often writing it first (with the code) results in wrong design decisions. Typically you end up catching and handling exceptions at too deep a level. I've recently taken the path of first writing the whole call graph for a single operation (the simple example of updating something at the DB will do here) from UI to DB, then going back to the lowest level of code and starting upwards, thinking of possible problem scenarios and how to catch them properly.

This approach provides a rather good end result; the catch constructs are usually placed somewhat optimally, as long as care is taken to refactor them when more call sites join the graph. However, the price is high. At first there exists a preliminary version of code where very little error handling is made. Now, if I forget to go through some branches of the call graph, I may actually end up checking in code without error handling, which is a rather undesirable event. Thus, the rule of "never write code without proper exception handling" is justified, even though it tends to result in need for refactoring later on.

As one solution, I've been trying to write unit tests that would clearly indicate the presence or lack of error handling code. This way, I could formally check for proper exception handling even if I don't write it at the same time with the normal implementation logic. However, particularly with DB objects and methods, unit testing is already complex enough as it is. Ideas on improving this process, anyone?

Posted by Jouni Heikniemi at January 1, 2005 10:41 AM
Misc. programming