Don't monkey with the loop index

Question

One of Steve McConnell's checklist items (pdf) is that you should not monkey with the loop index.

This makes intuitive sense and is a practice I've always followed except maybe as I learned how to program back in the day.

In a recent code review I found this awkward loop and immediately flagged it as suspect.

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

It's almost amusing since it manages to work by keeping the index at zero until all TabPages are removed.

This loop could have been written as

        while(MyControl.TabPages.Count > 0)
            MyControl.TabPages.RemoveAt(0);

And since the control was in fact written at about the same time as the loop it could even have been written as

        MyControl.TabPages.Clear();

I've since been challenged about the code-review issue and found that my articulation of why it is bad practice was not as strong as I'd have liked. I said it was harder to understand the flow of the loop and therefore harder to maintain and debug and ultimately more expensive over the lifetime of the code.

Does anyone have a better articulation of why this is bad practice?

Answer 1

I'm not sure if I understand completely what you're after, but maybe something like this?

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

or simply this of the TabPages collection implement IList:

    this.MyControl.TabPages.Clear();

Answer 2

Well, this adds confusion for little purpose - you could just as easily write:

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

or (simpler)

while(MyControl.TabPages.Count > 0)
{
    MyControl.TabPages.RemoveAt(0);
}

or (simplest)

MyControl.TabPages.Clear();

In all of the above, I don't have to squint and think about any edge-cases; it is pretty clear what happens when. If you are modifying the loop index, you can quickly make it quite hard to understand at a glance.

Answer 3

I think your articulation is great. Maybe it can be worded like so:

Since the logic can be expressed much clearer, it should.

Answer 4

I think you could build a stronger argument by invoking Knuth's concepts of literate programming, that programs should not be written for computers, but to communicate concepts to other programmers, thus the simpler loop:

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

more clearly illustrates the intent - remove the first tab page until there are none left. I think most people would grok that much quicker than the original example.

Answer 5

This might be clearer:

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

Answer 6

One argument that could be used is that it is much more difficult to debug such code, where the index is being changed twice.

Answer 7

It's all about expectation.

When one uses a loopcounter, you expect that it is incremented (decremented) each iteration of the loop with the same amount.

If you mess (or monkey if you like) with the loop counter, your loop does not behave like expected. This means it is harder to understand and it increases the chance that your code is misinterpreted, and this introduces bugs. Or to (mis) quote a wise but fictional character:

complexity leads to misunderstanding

misunderstanding leads to bugs

bugs leads to the dark side.

Answer 8

The original code is highly redundant to bend the action of the for-loop to what is necessary. The increment is unnecessary, and balanced by the decrement. Those should be PRE-increments, not POST-increments as well, because conceptually the post-increment is wrong. The comparison with the tabpages count is semi-redundant since that's a hackish way of checking that the container is empty.

In short, it's unnecessary cleverness, it adds rather than removes redundancy. Since it can be both obviously simpler and obviously shorter, it's wrong.

Answer 9

I think pointing out the fact that the loop iteratations is beeing controled not by the "i++" as anyone would expect but by the crazy "i--" setup should have been enough.

I also think that altering the the state of "i" by evaluating the count and then altering the count in the loop may also lead to potential problems. I would expect a for loop to generally have a "fixed" number of iterations and the only part of the for loop condition that changes to be the loop variable "i".

Answer 10

I agree with your challenge. If they want to keep a for loop, the code:

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

reduces as follows:

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

and then to:

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

But a while loop or a Clear() method, if that exists, are clearly preferable.

Answer 11

For reference, the advice to avoid monkeying with the loop index is found on page 377 of Code Complete 2 (2004) ISBN 0-7356-1967-0

Answer 12

The only reason to bother with an index at all would be if one were selectively erasing things. Even in that case, I would think it preferable to say:

  i=0;
  while(i < MyControl.Tabpages.Count)
    if (wantToDelete(MyControl.Tabpages(i))
      MyControl.Tabpages.RemoveAt(i);
    else
      i++;

rather than jinxing the loop index after each removal. Or, better yet, have the index count downward so that when an item is removed it won't affect the index of future items needing removal. If many items are deleted, this may also help minimize the amount of time spent moving items around after each deletion.