I was leafing through my Atlassian CVS-spam this evening (yes, my Saturday night has been that exciting), and came across the following commit-message.

Improve the speed of accessing enabled modules by class. Checking if a module is enabled is relatively slow, and seeing if it is assignable is very fast, so check assignability first.

A pretty trivial checkin, but one thing that worried me was that there wasn't a similar comment in the code itself. (I feel bad picking on one particular checkin to make my point, but that's blogging for you. It's nothing personal!)

If you make a change to a piece of code that doesn't change what it does, but just makes it faster (or more secure, more flexible, or any other of the engineering concerns that cross-cut pure functionality), Murphy's Law makes it almost inevitable that your good work will be undone by some future refactoring. Someone will go through, reorganise the class, and because it doesn't change the behaviour they'll reorder the operations again without thinking.

A certain proportion of my audience will now be saying "But Charles! Shouldn't that be picked up by your unit tests?" A (hopefully smaller) proportion, who I've seen argue this exact point on mailing-lists, will be saying that you shouldn't need to comment your code at all, that unit tests are sufficient documentation, and that any comment is just a sign your tests aren't good enough.

Yes. Please unit test the change. Then comment the code as well. To both groups of objectors, especially the latter, I might point out the following different courses my afternoon might take:

• Refactor the code
• Run the tests
• Read "testWeDoFooBeforeBar()" to understand what it's testing, and why it's failing
• Get annoyed that "testWeDoFooBeforeBar()" still doesn't explain why we have to do foo before bar, and wonder if it's some weird, obselete requirement I can delete now?
• Do a CVS blame on the original code
• Read the commit message, which hopefully explains things correctly
• If it doesn't, track down the original developer and ask them to remember a one-line fix they committed six months ago

Compared to:

• Read the comment
• Don't break the test in the first place

Of course, sometimes you (OK... I) can go overboard. The following comment is from deep inside the Confluence wiki-to-HTML converter, where I found that inserting a completely arbitrary and unnecessary newline between attributes inside an HTML element overcame some obscure IE/Firefox incompatibility:

/*
The newline before the title parameter below fixes CONF-4562. I have absolutely no idea HOW it fixes CONF-4562, but the simple fact that it does fix the problem indicates that I could spend my whole life trying to work out why and be none the wiser.

I suggest you don't think too hard about it either, and instead contemplate the many joys that can be found in life -- the sunlight reflecting off Sydney Harbour; walking through the Blue Mountains on a dew-laden Autumn morning; the love of a beautiful woman -- this should in some way distract you from the insane ugliness of the code I am about to check in.

Oh, and whatever you do, don't remove the damn newline.
*/
buffer.append("\n");