My wife showed me some code she was reviewing for the system she supports which was not working. She had been looking for where the problem was located. Obligitory dumb question: Didn't it have error messages? Of course it did! But if the error message isn't correct, you can easily end up on a wild goose chase. In medicine they call it 'treating the symptoms' when they know something is wrong but can't find the root cause. In this case, she had found a problem with the error handling code. Don't get on my case about proper syntax, this is just an illustration of the C code she was reviewing.
errCode = findError();
sprintf(errorMsgBuffer, "There was an error. ", errCode);
errCode contains the actual error message encountered and errorMsgBuffer is what is used to output the message. How quickly can you spot the error? Did you find it already? Well, too bad. I'll tell you anyway. The error message in the sprintf is missing "%s" so the errCode never gets copied into errorMsgBuffer. Simple fix, what's with all the fuss, right? Well, if exception (or error) handling is so important, why do we keep finding the same mistakes over and over. If this was tested, it escaped notice or was intentially ignored. I don't know which was the case for the code in question but it had indeed made it to production.
For you Exception bigots out there, here is an equivelently heinous bug, Exception Hiding.
try{ foo();}
catch(Exception e)
{
throw new Exception("There was an error");
}
How is anyone supposed to know about the original exception when all the information is tossed aside? This is bad code smell of garbage dump proportions. A better question is why this isn't caught in code reviews. When I started searching for this anti-pattern it was easy to get Eclipse to find where exceptions were being constructed. If there was an exception class constructed inside a throw clause it was instantly suspect. There are reasons when you want to create a new exception in a catch() block but those situations are, well, the exception rather than the rule. I need to get a copy of Anti-Patterns but this might be a good candidate if it isn't already. I'll call it Exception Upchuck. The code is sick. Swallowing an uncooked exception but not purging itself of the problem. Instead, it is vomiting back a new problem and making it that much harder for the code doctor to diagnose.
*Update: constructing an exception in a catch block is not always a bug but should at least invite review.
Sunday, August 06, 2006
Subscribe to:
Post Comments (Atom)
No comments:
Post a Comment
I reserve the right to delete inappropriate comments at my discretion