Posted September 16, 2011 at 12:39 pm by Ed Guiness
Here are five lines of code I found during a review not too long ago. This code had been tested and was ready for release.
Feel free to stare at this for a while and absorb the quality.
When I was at school I had an excellent teacher who taught me how to bench-check a loop. The basic idea is to write down the loop variables, their starting values, and then you step through the loop on paper and update these variables by hand to check that everything works as you expect, especially the edge cases.
That was back in 1986. These days I use a debugger and unit tests.
But for the sake of nostalgia let’s bench-check this quirky loop, shall we? I promise it won’t take long, and I do have a point to make.
Let’s assume we start with two tab pages, so we have
The conditional expression of the loop evaluates to true (since 0 < 2) and so the loop is entered.
Next, the first tab page (index 0) is removed from the collection.
Now we have one fewer tab pages, so the count is reduced by one.
Then we hit this weird post-decrement of the loop index.
So now our loop index becomes -1.
That’s the end of the loop, so now the loop expression is evaluated
And the loop index again has a value of zero.
The loop is entered again, since 0 < 1, and once again the first element of the TabPages collection is removed. There are now zero tab pages.
Now both the loop index and the count of tab pages is zero.
Next, the loop expression evaluates to false (since 0 is not less than 0) and the loop exits.
That’s what this weird code does. It removes all the tab pages from a collection.
That explains the code, but it doesn’t really explain how the coder arrived at this strange looping construct.
Let’s pretend for a moment that we are a harassed contract programmer working late, under intense pressure to deliver working code before we can go home.
Let’s also say, just for the sake of an entertaining make-believe scenario, that our boss doesn’t give a filthy tinker’s cuss about code quality.
And let’s finally suppose, entirely on a whim, that a few weeks ago we were unemployed, sitting in a waiting room with 100 other applicants holding a crumpled CV and sweating nervously.
And now here we are, tenuously employed, 9pm at night, hacking out a bit of code to remove all the tab pages from this control. It completely slips our mind that there might be a Clear() method.
At 9pm and all alone our harassed coder might think “…I need to remove ALL the items, I’ll do it one at a time… I need a loop”
But this first attempt doesn’t work unless TabPages.Count == 1.
For 2 or more tab pages it removes half of the Tab Pages then the loop exits.
Think of it like this. The loop index starts at zero and goes up. The Count starts at an arbitrary value and goes down.
At some point – half way – the loop index will no longer be less than Count, and the loop will exit, leaving half of the Tab Pages not removed.
If there is exactly 1 page, the loop will remove it and then exit.
Realising the loop isn’t quite right, our lazy programmer has two choices.
- Take a minute to think about the approach, figure out why it doesn’t work, fix it.
- Keep tweaking the code randomly until it works or the sun comes up and he gets fired
Actually, there are more options, like writing some tests or (gasp) asking for help, but this is my story and I’m keeping it simple.
From past experience our lazy programmer will have found that a few minutes concentrating on a problem, especially an apparently simple problem like this, can pay off, and that some beautiful, elegant code can emerge from moments of clarity.
Clearly then, our programmer will choose option 2; random tweaks.
Inefficient programmers tend to experiment randomly until they find a combination that seems to work.
— Steve McConnell
Look again at the original code.
The weird-looking decrement works by keeping the loop index at zero until TabPages.Count also hits zero.
Crazy like a fox?
Nah. Crazy like an inexperienced, tired, harassed, probably underpaid programmer who will be unemployed unless he produces working code before the sun comes up.
So now I’ve shown the code works perfectly, it just looks a little strange. It’s even shippable, and no one needs to know about this little quirk, right?
So what is my problem, exactly?
When I was asked to justify flagging this code, I recall saying something pretentious like “well, it’s harder to understand the flow of the loop, which will make it harder to maintain in the future”. I kind of thought this was self-evident so I didn’t elaborate (until now).
But why am I so obsessed with ease-of-maintenance?
If you’ve ever been a maintenance programmer you’ll understand why without any further explanation from me.
For everyone else, let me explain. First, look where money is spent over the life span of a product -
You may not believe this, but the evidence is good.
So when I criticise code like this, I’m not just being a prig. There is, I’m fairly convinced, a solid argument that making code easier to maintain will pay off over the life of a product.
And beside the economic argument there’s also the argument for protecting your skin from the violent psychopath who works on your code and knows where you live.
And now you know why, when I’m interviewing a programmer, I usually produce a code sample like this and see what they think.
It’s because I want to see if they are going to increase or decrease the overall burden.
Will they make my world of code a better place, or will they watch as it turns into a slum?