IanG on Tap

A few weeks ago, Joel Spolsky wrote about making wrong code look wrong. I'm a big fan of this in particular, and of the more general idea of making a code look like what it is. The combination of syntax highlighting, and carefully selected coding idioms enable the trained eye to take in a vast amount of information by looking at the code. I'm very much in favour of code where you know what you're looking at without having to refer to something else, bring up a tooltip, or think very much. And if that last one sounds lazy, it isn't - I'll think just as hard about the code either way, I'd just prefer to spend that thinking capacity on what the code does, rather than what on earth some variable x represents.

However, there was one part of Joel's article that rather surprised me. He put up this code:

dosomething();
cleanup();

And then he told us what to think:

"... your eyes tell you, what's wrong with that? We always clean up!"

That might be what Joel thinks I was thinking, but it certainly wasn't my reaction to the code - it set off an alarm bell in my head. Joel seems to think that the reaction he describes is natural, and it might be in an inexperienced programmer. But I think it is a good example of a problem Joel describes in the article: failure to distinguish between 'clean' and 'unclean' code.

Joel makes the keen observation that as programmers develop their skills, they "start to smell subtle hints of uncleanliness beneath the surface." What he has failed to notice about this example is that if you've done much real work with exceptions, this is one of those subtle hints you learn to smell.

I'll show you what I see when I look at that code:

dosomething();

*************
*  DANGER!  *
*************
* cleanup();*
*************

Notice how years of experience with exception-based error handling have taught my brain to annotate this kind of code automatically. (It would even have used the <BLINK> tag if that were still widely supported.) Apparently Joel's brain doesn't provide that service in this case. I think that's because he shys away from using exception handling, and has consequently never learnt to recognize this particular smell.

The reason this code leaps out to me in glorious ASCII art is that I know better than to put cleanup code all on its own like that. My first thought with any piece of cleanup code is along the lines of "This needs to run, so I'd better make sure it runs." So I'll either put it in a finally block or use a using statement. (Incidentally, I'd do that even if I weren't using exceptions to raise errors. There are other reasons that flow of execution might leave a block of code without running through to the end besides exceptions, so this cleanup style Joel presents is always a risky proposition at best.)

Despite having been using exceptions for years, I'm still somewhat ambivalent about them. I don't think Joel's example here is a good one, but I think Raymond Chen's example towards the end of this article (the AddNewGuy function) is a good example of how bugs in code that relies on exception handling can often be very hard to spot. Not that I'm convinced that error codes actually solve that particular problem - just because your error handling is verbose and in your face doesn't necessarily make it any easier to see subtle bugs in your flow control - if anything, it obscures that kind of problem even more.

For issues like the one Raymond raises, I think Joel's fourth stage of programmer personal development comes into play:

"You deliberately architect your code in such a way that your nose for uncleanliness makes your code more likely to be correct."

Raymond's principle of "Don't commit data until they are ready" seems to be an example of just this kind of thing. (At least it is as long as your nose is attuned to the right smells.) But that principle holds whether you're using exceptions or not.

I think the only viable solution to this problem has to be fault injection testing. Whether you're using good old error code style handling or exceptions, you won't really know if your code responds correctly in an error situation unless you test it. You can reason all you like, but in my experience, repeatable testing is more reliable than a developer's reasoning skills applied to his or her own code. If fault injection tests are built into the build process, the failure will be very obvious to the developer, even though the problem wasn't obvious from a code inspection.

Given adequate testing, long-winded error handling based on error codes doesn't seem to offer any benefits as far as the visibility of problems is concerned, because it's the tests that make the problems visible. And with inadequate testing, your code won't work properly regardless of which style you use. So I'm sticking with the more concise style for now, thanks.