In my last entry, I set some guidelines for re-throwing exceptions. The flip side from re-throwing exceptions is swallowing exceptions. In nearly all cases I can think of, swallowing exceptions is a Very Bad Idea (tm). So what am I talking about when I say "swallowing exceptions"? Check out the following code snippet:
public void ProcessRequest()
{
Message msg = null;
try
{
msg = ReadMessage();
}
catch (Exception ex)
{
LogException(ex);
}
ProcessMessage(msg);
}
This code is fairly straightforward. I declare a Message, try and read the message, and if something messes up while reading the message, I log the exception. Finally, I'll process the message I read.
There's a monster lurking about...
Astute code readers will notice that even if there is a problem in ReadMessage, this function will always process the message. The "try" block has caught an exception, but did not re-throw the exception, so the rest of the execution context in this method is basically unaware of the exception.
Swallowing exceptions is when a Catch block does not re-throw the exception.
ProcessMessage might throw a NullReferenceException, or even an ArgumentNullException. Now we've left it up to the code following the "try" block to handle error cases, invalid states, etc. We've just made our lives that much more difficult.
Effect on maintainability
So Bob coded the above snippet, shipped it, then moved on to bigger and better things. Joe comes along six months later, needing to change the message processing, and notices he gets very strange behavior when ReadMessage doesn't work. When Joe looks at the code, he's a little baffled. Hoping to find clear, intention-revealing code, what he found was exception swallowing choking the system. Joe doesn't know if the original author intended to process bad messages or not. Joe doesn't know if ProcessMessage is able to handle bad messages. If Joe has to add a lot of pre- and post-processing to the message, now his life is harder since he has to worry about bad messages. Why should I process a message if I wasn't able to read it?
Since Joe knows that 99 times out of 100, you shouldn't swallow exceptions, he has to take time to figure out if this was one of those times, or if this is just a bug. Either way, Joe is wasting time trying to figure out what the system does rather than delivering business value. Whenever you spend most of your time in the former rather than the latter, that's a clear sign that you don't have a maintainable codebase.
A better solution
So we have a requirement to log exceptions. That's a good thing. Ideally, our codebase should be largely unaware of having to catch and log exceptions, since this extra code tends to clutter up what we're working on. I believe I have two options, either re-throwing the exception or removing the try-catch block and applying it on a higher level. Here's the former:
public void ProcessRequest()
{
Message msg = null;
try
{
msg = ReadMessage();
}
catch (Exception ex)
{
LogException(ex);
throw;
}
ProcessMessage(msg);
}
Now it's fairly clear to Joe that there is a requirement to log exceptions, but this function show never continue its message processing if it encountered problems reading the message.
Some guidelines
So when should we swallow exceptions?
- When execution needs to continue despite an error
- The expected behavior of the system is to continue operating and not to crash
By following these guidelines, you'd probably notice that 99 times out of 100, you'll always re-throw the exception. And when you do swallow exceptions, it's usually in the top level modules of a system. So be kind, don't swallow exceptions, and some day Joe will thank you for it.
.png)
No comments:
Post a Comment