CodeSOD: Exceptional Control

Sebastian started a new job recently. Like a lot of “I started a new job,” stories, this one starts with a 1,000 line class definition. What’s notable about this one, however, is that most of that code is error handling. Now, you might think to yourself, “well, there’s nothing inherently wrong with loads of error handling, if the problem calls for it.

This code is getting posted here. Do you think the problem calls for it?

object Method1(MyOtherClass parameter)
{
try
{
if(parameter == null) throw new ArgumentNullException();

//… 5 lines of code
}
#region catching
catch(FormatException)
{
return null;
}
catch(InvalidOperationException)
{
return null;
}
catch(Exception)
{
return null;
}
#endregion
}

bool Method2(MyOtherClass parameter)
{
try
{
result = Method1(parameter);
if(result == null)
throw new Exception();

// … 3 lines of code
}
catch(Exception)
{
return false;
}
}

Names have been anonymized by Sebastian.

We open with a mostly reasonable bit of code- if the input parameter violates our contract, we throw an exception. I don’t love exceptions for communicating contract violations- it’s much better if you can enforce that at compile time- but I won’t fault anyone for that. But gee, isn’t it a bit odd that we throw that exception inside of a try block?

Oh, that’s because we catch the exceptions and return null. The fact that we catch multiple kinds of exceptions and just return null is already bad. It gets worse when we note that the last caught exception is Exception, the root of all exception classes.

Normally, when we talk about the anti-pattern of using exceptions for flow control, we tend to think of them as spicy gotos, which is exactly what’s happening here. It’s just… we’ve removed the spice. It’s Minnesota Spicy Gotos- where the three grains of black pepper are too much zing. We’re jumping to the appropriate return statement. Which we could have just replaced the throw with a return. And you can’t even say that they’re trying to follow the rule of “only have one return”.

The calling function makes the whole pattern worse. We invoke Method1, and if it returns null (that is to say, if it throws and catches its own exception), we… throw another exception. Which leads us to a return false.

Sebastian tells us that this 1kloc class is about 70% error handling code, by volume, and as we can see, none of these errors need to happen.

Source: Read More