Geeks With Blogs
Dave Chestnutt: SparklingCode and CodeGaffes Writing better code; for fun and profit

I was fixing a bug the other day, when I ran across this comment (marked in red):

 
if (condition1)
{
    //… some useful code …
}
else if (condition2)
{
    //… some useful code …
}
else
{
    // this can never happen
}
 
My first thought was, if this can never happen then why not throw an exception in case it does? So I added a line to throw an exception.
 
if (condition1)
{
    //… some useful code …
}
else if (condition2)
{
    //… some useful code …
}
else
{
    // this can never happen
    throw new ApplicationException("this can never happen");
}
 
And you can guess what happened. The next night, some of our tests failed. Because of course "it" did happen.

 

There are a couple of reasons why this pattern appears in code.

  • Overconfidence. The programmer is sure that their prior code has caught all cases, so it won't happen – but they want to be good citizens and document that fact with a comment or assertion. And their comment is correct – at first. But of course, all code gets modified and unfortunately, over time the original assumptions no longer hold. Since the assumptions are just that – assumed – and not checked by the code, you end up with brittle code.
  • Time Crunch. The programmer, under the gun to fix a bug or finish a feature, doesn't really know what the code should do if this actually happens. There's no apparent recovery that the code can do, but they're pretty sure it won't happen. They're not 100% sure so they leave a comment or Debug Assertion behind.  In this scenario, unit tests are usually skipped, too.

So what's the right thing to do?

 

  1. Throw an exception. You can leave the comment or assertion in, but DO throw an exception so you see the problem exactly when it happens.  It will be much easier to spot the problem when you have an exception pointing directly at the offending line, than it is to figure out what's wrong when the code doesn't fall over until another thousand lines of code execute. When it breaks, as it did in my case, you can fix the code to handle the new situation and move on.
  2. Write recovery code.  Sometimes, there's a perfectly reasonable action you can do - so recover and let the code continue. For example, this code was checking values from a settings file, and if the file was ever corrupted, there was a perfectly reasonable default we could use.

 

In essence, this CodeGaffe is like neglecting to add a default case to a switch statement. Many times, you really should have a final else on your if statements. They won't all throw exceptions of course, but you should always think about what should happen if your execution did fall through your if statements. Then do the right thing with some actual code, not with clueless comments.  Ah, if only the compiler would do what I meant, not what I wrote.

 

Have you ever seen this gaffe in your projects?

 

Technorati tags: ,

Posted on Wednesday, January 18, 2006 5:45 PM CodeGaffes | Back to top


Comments on this post: CodeGaffe: Clueless Comments - Never Say Never

No comments posted yet.
Your comment:
 (will show your gravatar)


Copyright © Dave Chestnutt | Powered by: GeeksWithBlogs.net