What’s wrong with this code, really?

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.

for ( int i=0 ; i < this.MyControl.TabPages.Count ; i++ )
{
   this.MyControl.TabPages.Remove ( this.MyControl.TabPages[i] );
   i--;
}

 

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.

this.MyControl.TabPages.Remove ( this.MyControl.TabPages[0] );

 
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.

i--;

 
So now our loop index becomes -1.

Yeah, really.

That’s the end of the loop, so now the loop expression is evaluated

i++;

 
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.

this.MyControl.TabPages.Clear();

 
Oops.

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”

for ( int i=0 ; i < this.MyControl.TabPages.Count ; i++ )
{
   this.MyControl.TabPages.Remove ( this.MyControl.TabPages[i] );
}

 
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.

Wait… half?

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.

  1. Take a minute to think about the approach, figure out why it doesn’t work, fix it.
  2. 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.

for ( int i=0 ; i < this.MyControl.TabPages.Count ; i++ )
{
   this.MyControl.TabPages.Remove ( this.MyControl.TabPages[i] );
   i--;
}

 
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.

“…software maintenance consumes 60% to 80% of the total life cycle costs” (pdf)

“..two-thirds of a software system’s lifetime cost involves maintenance…”

“…CIOs are spending 80% of their budgets on maintenance…”

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?

@KiwiCoder

Entry filed under: Interviewing, Programming. Tags: .

IT recruiters, in their own words Programmers are not in thrall to IT recruiters

46 Comments

  • 1. Que le pasa a este codigo? [EN]  |  September 16, 2011 at 1:52 pm

    [...] Que le pasa a este codigo? [EN] cvmountain.com/2011/09/whats-wrong-with-this-code-really/  por dokkillo hace nada [...]

  • 2. Ed  |  September 16, 2011 at 5:43 pm

    Discussion on Hacker News here….http://news.ycombinator.com/item?id=3004080

  • 3. trevelyan  |  September 16, 2011 at 6:25 pm

    What is the problem with this code? It is typical to need to *do something* with objects before removing them (such as running debugging functions during development), in which case a loop is the only practical solution.

    And then we have the reality that for many kinds of structures, you simply cannot trust high-level clearing functions. A common example is deleting a vector containing pointers. In languages like C/C++ removing the pointers with clear() won’t trigger the destructors of the underlying objects. So your suggested change would introduce a nasty memory leak in a case when – as you say – the code has already been confirmed as ready for production.

    If I were writing this, the only real problem I can see is that a loop is less efficient than an iterator in many cases so if we are nitpicking the code should be MORE complex rather than less. I personally take the efficiency hit in the interests of greater readability, but….

    • 4. Brian Szymanski  |  September 17, 2011 at 8:16 am

      You sir, have missed the point altogether.

      Just because you have to loop doesn’t mean you have to write it in such a wrongheaded fashion.

      while(this.MyControl.TabPages.Count > 0) {
      this.MyControl.TabPages.Remove ( this.MyControl.TabPages[0] );
      }

  • 5. Maintenance Man  |  September 16, 2011 at 6:27 pm

    At first I did not see the i– statement. So I thought this thing would not be removing all the tabs. Just goes to show you that the code is not good for maintenance.

  • 6. MH  |  September 16, 2011 at 6:53 pm

    Aside from the .Clear() method, one could simple leave the alterations:
    for(i=0;i<array.count;){
    tabpages.remove(tabpages[i]); // we could even change the "i" with "0".
    }

    Good example though.

  • 7. trevelyan  |  September 16, 2011 at 6:54 pm

    Incidentally, while I agree the i–/i++ is a bit strange in this case, there are some cases where keeping the index at zero is reasonably defensive programming, especially when you’re dealing with mutable data structures (where the size might be different each time through) and you run the risk of race conditions.

  • 8. Que le pasa a este codigo? [EN] | Noticias - d2.com.es  |  September 16, 2011 at 7:21 pm

    [...] » noticia original [...]

  • 9. Erhan Hosca  |  September 16, 2011 at 7:54 pm

    this.MyControl.TabPages.Clear(); anyone ?

    • 10. Albert  |  September 17, 2011 at 12:22 am

      Read the whole article, anyone?

      • 11. D_Jay  |  September 17, 2011 at 7:25 pm

        owned

  • 12. Glen Robertson  |  September 16, 2011 at 8:04 pm

    Classic. I have a strong urge to send this article to some of my past project managers.

  • 13. Uriel  |  September 16, 2011 at 8:06 pm

    Yeah, i saw a lot of code everyday, and I try to seeing what were they thinking when they were writting it. But it’s funny, because it only takes a second to understand that with a better understanding not of what is doing the code, but what is supposed to do and wich (to say) objects are involved, you can come with a better approach, don’t you think?

  • 14. Alex  |  September 16, 2011 at 8:21 pm

    To those of you saying that the i++, i– construct makes sense, it really doesn’t. If you really want to do this in a loop, defensively and with mutable data structures, you should be doing the following:

    while (this.MyControl.TabPages.Count > 0)
    {
    this.MyControl.TabPages.Remove ( this.MyControl.TabPages[0] );
    }

    • 15. bryce  |  September 16, 2011 at 8:53 pm

      I completely agree. Why the hell do we need a variable at all if it doesn’t ever change from zero. The i–; and i++; are evaluated literally one after the other. Nothing ever sees it as anything other than 0.

    • 16. rdx  |  September 17, 2011 at 7:07 pm

      Just chimed in here to say this.
      This should be a while loop, not a for loop.

  • 17. Bronsa  |  September 16, 2011 at 8:35 pm

    what about simply omitting the counter incrementer?
    for ( int i=0 ; i < this.MyControl.TabPages.Count ; )
    {
    this.MyControl.TabPages.Remove ( this.MyControl.TabPages[i] );
    }

    • 18. Loz  |  September 16, 2011 at 10:43 pm

      Very sneaky, but completely non-idiomatic :)

    • 19. asdffewf  |  September 17, 2011 at 1:08 am

      okay, but why have a variable that is never changed? replace it with 0.

      for ( ; 0 < this.MyControl.TabPages.Count ; )
      {
      this.MyControl.TabPages.Remove ( this.MyControl.TabPages[0] );
      }

      as an added benefit, this skeleton shows exactly how you got to where you are. It's steeped in history.

      • 20. Ove Gram Nipen  |  September 17, 2011 at 6:04 pm

        A for-loop without initializer or incrementer is otherwise known as a while-loop. And I believe this is quite beside the point, since the article is about people who just don’t care what the code looks like or how it reads, as long as it runs.

  • 21. Io  |  September 16, 2011 at 8:41 pm

    So, the only correct answer upon seeing that code is to go “WTF!?” or to offer to submit it to the Daily WTF, right?

    • 22. Albert  |  September 17, 2011 at 12:24 am

      Yes you are pretty much right there. Also, finding another job ASAP to avoid working with the original author of that code is recommended.

  • 23. debel  |  September 16, 2011 at 8:55 pm

    Or you could just loop it in reverse:

    for (int i = MyControl.Items.Count – 1; i >= 0; i–)
    {
    MyControl.Items.Remove(MyControl.Items[i]);
    }

    • 24. joan  |  September 16, 2011 at 10:20 pm

      +1
      With the while() solution suggested in the comments, each time we remove an element all the internal pointers that the list keep to index items have to be updated…
      .Clear() is still best of course.

  • 25. adasd  |  September 16, 2011 at 9:23 pm

    well, if there was no “clear” method, a reverse loop would do the job perfectly.

  • 26. drive-by commenter  |  September 16, 2011 at 10:07 pm

    is Count read-only? if not then
    this.MyControl.TabPages.Count = 0;

    • 27. Dixon  |  September 17, 2011 at 1:24 am

      I love this solution.

  • 28. Matti Virkkunen  |  September 16, 2011 at 10:44 pm

    I am flabbergasted and appalled by the few comments that seem to find something good about this mindblowingly dumb loop. “Defensive programming”? No. Just no.

  • 29. packetpirate  |  September 16, 2011 at 11:42 pm

    Actually, I’m pretty sure this code would cause an out of bounds exception.

    Counter | Tabs
    ——————-
    0 | 2 (Post-Loop: -1 | 1) -> True
    0 | 1 (Post-Loop: -1 | 0) -> True
    0 | 0 (Post-Loop: -1 | 0) -> True (It will not end on this iteration because the variable decrements after removing the last tab, so on the next iteration, before “i” is incremented, it will check -1 against 0, which will evaluate to true, then it will increment “i”, leaving you with 0, 0 again. The loop will attempt to remove element [0], but since it was removed in the last iteration, there will be an out of bounds exception.)

    Maybe I’m wrong, but someone should check it for themselves with a simple array.

    • 30. Paul  |  September 17, 2011 at 12:37 pm

      So true! I couldn’t believe the OP was claiming that this code was working right and was about to post the same thing when I saw your comment. Kind of shocking that all the other commenters seem to have missed this!

    • 31. Ed  |  September 17, 2011 at 4:21 pm

      After an iteration, but before the loop’s conditional expression, the loop-expression i++ is evaluated, which brings i back to 0 before the conditional is evaluated.

      • 32. Alex  |  September 17, 2011 at 6:32 pm

        Agreed. In general, you can translate a for loop by putting the initialization before the loop, the increment at the end, and renaming “for” to “while”. So the original loop is equivalent to:

        int i=0;
        while ( i < this.MyControl.TabPages.Count )
        {
        this.MyControl.TabPages.Remove ( this.MyControl.TabPages[i] );
        i–;
        i++;
        }

        And from this, it is obvious that i == 0 everywhere that matters.

  • 33. David Grant  |  September 17, 2011 at 12:52 am

    Was this code covered by unit tests? If so, who really cares that the implementation is a bit crappy. There is an endless supply of programmers who write inscrutable code. Isn’t it more important whether a piece of code is unit tested or not? If it was, then is it worth wasting time over that ugly loop? If there are no unit tests, focus on that.

    • 34. Nick Kwiatkowski  |  September 18, 2011 at 9:19 pm

      Just because a piece of code passes a unit test, does not mean that it is bad code. In this case, the code works, but could easily cause issues later on. It passes a unit test, but that’s it.

      Unit tests don’t protect bad code. I could write methods that pass unit tests all day long, yet still manage to do bad stuff — really bad stuff.

  • 35. Matt  |  September 17, 2011 at 1:13 am

    in the absense of a clear method, the best option for removing all the elements is something like this:
    for (; this.MyControl.TabPages.Count != 0;)
    {
    this.MyControl.TabPages.Remove ( this.MyControl.TabPages[0] );
    }

  • 36. Kevin Reid  |  September 17, 2011 at 2:05 am

    There is a case where that loop structure makes sense: if elements are removed from a collection conditionally. In that case, you need a loop index to advance past the skipped elements, and you decrement the index whenever you delete an item. So, this code could arise as a modification-without-refactoring of such a loop.

    I’m not saying that such code should be left alone; just that it’s not completely absurd.

  • 37. Victor  |  September 17, 2011 at 12:21 pm

    I think that a lot of people are missing the author’s point. He is not saying that the code does not work, he is making the point that there should be more criteria for “correct” code then that if it works.

    I know that where I work, maintenance of code takes about 4x longer then development… Part of the issue is that the maintenance developers are generally less experienced then the original developers of the code. So maybe be nice to the poor shmuck that has to hunt through our clever solutions in the future.

  • 38. Mårten Gustafson  |  September 17, 2011 at 3:53 pm

    Related to the part about software maintenance in the last part of the post I recently wrote about “visualizing cost and improvement areas for software maintenance” here:
    http://marten.gustafson.pp.se/content/visualizing-cost-and-improvement-areas-for-software-maintenance

  • 39. Steven Sargent  |  September 17, 2011 at 6:32 pm

    Depending on the representation, removing element 0 may be considerably less efficient than removing the last element (if the Remove method has to slide the remaining elements over the removed element). In an array, you would see about n*(n-1)/2 element moves from removing at 0, vs. 0 element moves from removing at the end.

    The authors of the Clear() method would be expected to know the representation and write the proper loop; for this and other reasons mentioned, Clear is still the winner until proven broken.

  • 40. Beerwulf  |  September 17, 2011 at 9:54 pm

    really? the whole problem the harrassed conract programmer had was that this.MyControl.TabPages.Count would change with each iteration and was unsuitable for a loop termination criterion. The answer (if he doesn’t know about the .Remove() method) is to either do this:

    int Count = this.MyControl.TabPages.Count;
    for( int i=0; i=0; i–)
    this.MyControl.TabPages.Remove ( this.MyControl.TabPages[i] );

    although the authors of a control that keeps tab pages around would surely have written a method like

    this.MyControl.TabPages.RemovePageAtIndex(i)

  • 41. anon  |  September 18, 2011 at 4:38 pm

    Mutability is the real problem with the original and revised code.

  • 42. Nick Kwiatkowski  |  September 18, 2011 at 9:24 pm

    The other problem this code might run against at a later date is — what happens if the compiler gets smarter in a later revision and opimises this bit of code. I ran into that exact same situation where all of a sudden a method stopped working. It still passed the unit test, but the compiler decided to opimize the code so that it caused memory overrun errors.

  • 43. Pavithra  |  September 19, 2011 at 7:16 am

    If he needed a loop so badly he could have easily gone for a while loop instead of for loop as what he does is neutralizing the increment.

    while(this.MyControl.TabPages.Count > 0) {
    this.MyControl.TabPages.Remove ( this.MyControl.TabPages[0] );
    }

  • 44. fx86  |  September 19, 2011 at 8:24 am

    Thank God(?) for Python!

  • 45. Chris  |  September 19, 2011 at 4:38 pm

    Ignoring that there is an already existing method to do this, I would find it hard to stop myself noticing that the ++ and — cancel out. I’d remove them without thinking.

    for ( int i=0 ; i < this.MyControl.TabPages.Count ; )
    {
    this.MyControl.TabPages.Remove ( this.MyControl.TabPages[i] );
    }

    Then I'd notice that i is always zero. I'd hard code it

    for ( ; 0 < this.MyControl.TabPages.Count ; )
    {
    this.MyControl.TabPages.Remove ( this.MyControl.TabPages[0] );
    }

    Now it's clearly a while loop disguised as a for loop

    while ( 0 < this.MyControl.TabPages.Count )
    {
    this.MyControl.TabPages.Remove ( this.MyControl.TabPages[0] );
    }

    I'm also not a fan of long chains. I'd create a variable to stop me repeating them. Your mileage may vary. I'm unsure what language this is in so I'm assuming that the class is called TabPages.

    TabPages tabs = this.MyControl.TabPages;
    while (0 < tabs.Count)
    {
    tabs.Remove(tabs[0]);
    }

    For me, this is a very clear refactoring path and one I'd find it difficult not to do automatically as I was reading through the code.

  • 46. A possible job interview question  |  September 28, 2011 at 3:19 pm

    [...] possible job interview question? What is wrong with this code? Here are five lines of code I found during a review not too long ago. This code had been tested and [...]


ABOUT.ME

Ed Guiness

I am the author of Ace the Programming Interview, published 2013 by John Wiley and Sons. In 2012 I founded SocialCoder.org, a volunteering organisation for programmers. I've been a professional programmer for more than 20 years, and a hiring manager since 2004. Ask me anything.

Recent Posts