Monday, March 26, 2012

Catch != Handle

I'm getting really tired of this:

try
{
    // do something
}
catch
{
}

I'm sure we all are, really. Hopefully you don't encounter it nearly as often as I do. I would say that I should just lay off the Stack Overflow for a while to try to avoid this construct, but all too often I actually see this in production code at work.

(Needless to say that if I ever see it from someone on my team, rather than legacy code owned by a client, then someone's going to get chewed out military style. But I digress...)

However, this isn't that bad. The code is terrible, yes. But encountering it, even though it causes me to die a little inside, isn't really a bad thing. It's easy to explain to the owner of the code why it's bad. It's easy to correct the situation.

What is really starting to bother me is this:

try
{
    // do something
}
catch
{
    throw;
}

Or, worse, this:

try
{
    // do something
}
catch (Exception ex)
{
    throw ex;
}

Or, even worse, this:

try
{
    // do something
}
catch
{
    throw new Exception("Failed to do something");
}

I encounter this nonsense a lot, and the truly sinister part is that the owner vehemently defends this position. It's most commonly seen wrapping some database code, and the owner usually makes a point to try to wrap as little code as possible with this construct. Usually just the part which executes the database query. (Though I've even seen this construct repeated multiple times within a method. Once to surround opening the database, once to surround executing the query, etc.)

Whenever I try to point out the flaws in this approach, I'm met with variations of the same argument... "But you're supposed to wrap all database code in a try/catch! That's the right way to do it! Otherwise you're going to have exceptions happening!"

I'm sure at some impressionable point in the owner's career this concept was hammered into his head. Always catch database exceptions. And, in a way, he's right. But he interpreted it wrong. He's following the letter of the law with no regard for the spirit of the law. He's blindly applying a coarsely-defined policy at the expense of his code. (Something about which I've written before, and something against which I often argue.)

The key thing to note is the difference between "catching" an exception and "handling" an exception. They are not the same thing. Catching is a physical construct. It's part of the language. Handling, on the other hand, is a logic construct. It's part of the design of the code, independent of the language being used.

If the current context can handle the exception... that is, if it can meaningfully respond to it and logically continue the execution of the application... then that's where you catch it. If there's nothing meaningful to be done with the exception in the current context, then there's no reason to catch it.

The standard response to this notion from those who defend their position is that one should never let exceptions be unhandled. That would be madness, apparently. Exceptions being thrown? God help us. Seriously, though. This isn't a bad thing. Exceptions are not a bad thing. Don't be afraid of them.

Exceptions are not a bad thing. Don't be afraid of them.

Exceptions are our friends. They are how the framework reports errors. Use them. Especially use them instead of return codes. (Man do I hate return codes. They're a particular flavor of magic numbers/strings, and a distasteful flavor at that. It's one thing for a class or a method to have a magic string. It's another thing for that class or method to leak that magic string all over the place. Some designs call for return messages, such as standard response envelopes with Errors collections and what not. They're the exception, not the rule. No pun intended, honestly.)

The compiler even hints at the usefulness of exceptions in a very subtle way...

public User GetUser(string name)
{
    // TODO: implement this
}

The compiler doesn't like that. It tells us that this method has no code paths which return a value. This, however...

public User GetUser(string name)
{
    throw new NotImplementedException();
}

This is perfectly acceptable. Why? Because throwing an exception is a perfectly valid exit path for a method. It's a way to end the method and pop back up the stack.

Throwing an exception is a perfectly valid exit path for a method.

Again, it's all about context. If the method can meaningfully respond to the error in some way, then it should. But if it can't, then it shouldn't. It shouldn't even try. The method should exit in a state of error, pop back up the stack, and let something that can meaningfully respond to the exception do so.

Does this mean that the exception can go entirely unhandled? Absolutely. And that's a good thing. Well, it's not really a good thing. But it's good to call light to it as being a bad thing, rather than hide it away where bugs can fester. If the application ends up failing due to unhandled exceptions, that's a clear indication that, well, you need to go in there and handle them. If the application ends up with long-lived subtle bugs due to improperly handled exceptions, that's not a clear indication of anything and will cost more time and effort to determine the root cause and repair it.

So, in that sense, it's a good thing for a bad thing to happen. Assuming that a bug will exist, would you rather it make itself clearly known or hide away for years and corrupt more things along the way? This is another reason why exceptions are not to be feared. Don't suppress errors, embrace them. Errors tell you what's wrong and how to fix it.

Don't suppress errors, embrace them. Errors tell you what's wrong and how to fix it.

So what universal standard should one employ for error handling? I've heard arguments at both extremes. Some developers defend that exceptions should be caught as close to the event as possible. Others defend that exceptions should be caught as close to the UI as possible. They're both wrong. Or, at the very least, misguided in their approach. As I've said, exceptions should be caught where they can logically be handled.

For example...

Consider a web application. In true web application fashion, it handles requests and crafts responses. The structural life of an "instance" is that request/response scenario. Being RESTful (hopefully), there's no meaningful context outside of that scenario.

Say that a user submits a form to update a record in a database. The update fails. Maybe the database is down, maybe there's an error with the data, whatever. It's not important. What is important is that an exception was thrown. Where should that exception be handled?

Well, where is it most meaningful to handle the exception? The update failed, so the method which performs the update doesn't have anything more to do. It gives up. It doesn't try to recover, it doesn't try to make you feel warm and fuzzy. It has one responsibility... Update the data. When that fails, it gives up.

So the exception starts to bubble up the stack. This is good. It carries useful and meaningful context about the error with it. Now, where should it stop? In this case, it should probably stop very near to the UI. After all, contextually what actually failed? The request. So the response, which is a fairly immediate thing that the user needs to see, should contain an indication of the failure.

So, in this case, let the exception bubble up to the web application's global exception handler, or perhaps a unit-of-work exception handler, or some other request-wide handler which has the responsibility of handling errors in the request and reporting them back in the response. Show the user a meaningful error message, log details of what happened, and let the application continue about it's merry way. (In this case, continuing about its merry way means delivering the response and disposing of the current context, awaiting the next request.)

Now consider a different example. We can even suppose that this example is part of the overall system of which that web application was part. Let's say a user can upload a "file" to be "processed." In this case it's a CSV or otherwise record-based file, potentially large, where a back-end service will "process" each record (it doesn't matter what that means right now) and send some notification to the user when it's done.

So this back-end service has a fairly simple structure. It's probably a Windows Service or a console app or something that's just running unattended with no user interface. It checks a data store (database, directory on the file system, etc.) for new data to be processed and then processes it in a loop of some kind, record by record.

Now, let's imagine that one of the records can't be processed due to an error. Maybe some validation check failed, maybe there was a temporary hiccup in network connectivity to a necessary resource, whatever. Again, not important right now. The loop encountered a record that couldn't be processed. Where should the exception be handled?

Well, in the case of the developers who defend that it should happen as close to the UI as possible, we have a problem. What UI? The web application? No, that request is long since gone. The user has moved on to other things. The back-end service? It doesn't have a UI.

Should the service itself error out and let the system handle the error? (Basically end up in the Event Log, adrift in a sea of useless information?) No, definitely not. Just because one record couldn't be processed doesn't mean the whole service should give up. The method which processes that record should give up. But the method which processes that file has every reason to keep going.

So in this case the meaningful place to handle the exception is in the loop of the back-end service. Catch the exception, log it, in some way mark the record as unprocessed (a flag in the database, an edit to the file, some notification to the user such as an email or an in-app notification system in the database, etc.), and move on to the next record. That's where it logically makes sense to handle that particular error in that particular stack.

In either of these scenarios, we could have caught the exception anywhere. As a physical construct, that can happen anywhere the language supports it. But there were more specific places where it made sense to handle the exception. As a logical construct, there are relatively few places where it truly makes sense.

Catching an exception is easy. Handling it requires one to actually think about the context of the given scenario. And thinking is a good thing.

2 comments:

  1. To be fair, database queries should be wrapped in a try/catch, however, there needs to be a finally.

    try{
    //open connection, do work
    }catch
    {
    throw;
    }
    finally
    {
    // Close connection
    }

    But a try with a throw catch and nothing else is stupid.

    ReplyDelete
    Replies
    1. Database queries should be wrapped in a try/finally. You don't need a catch. And the "using" statement in C# makes light work of a try/finally. (That's all it is, really. The compiler turns it into a try/finally statement with a call to Dispose().)

      If nothing meaningful happens in a catch statement, get rid of it.

      Delete