views:

160

answers:

12

I just found ... AGAIN ... a real time wastage bug as follows

for (int i = 0; i < length; i++)
{ //...Lots of code 
    for (int j = 0; i < length; j++)
    {
        //...Lots of code 
    }
}

Did you notice straight ahead the inner i which SHOULD BE j ? Neither did I. So from now on I am going to use:

for (int i = 0; i < length; i++)
{
    for (int i1 = 0; i1 < length; i1++)
    {
    }
}

What are your tips for inner and outer while and for loops ?

Edit: Thanks for the valuable responses. Herewith short summary of the proposed tips:

  • use meaningful variables names for index variables ( instead i use SomeObjCollectionLength )
  • place the contents of the inner loop into a separate method and call that method from the outer loop
  • not manageable amount of lines of code between the outer and inner loop is a strong signal for code smell
  • avoid copy pasting and rushing , write the index vars with care

You might want to check the summary by LBushkin for the following

  • use foreach and iterators whenever possible
  • initialize the variables just before entering the loops
  • Make each loop perform only one function. Avoid mixing responsibilities in a single loop
  • When possible, make your loops short enough to view all at once
A: 

You just have to take extra care of such issues, there's no magic bullet against this. Even with "better naming" you propose you will once in a while lose track of whether this is Nth or (N+M)th level of nested loop and make an error.

If nested loop is necessary write it carefully. If it can be avoided by extracting the outer loop body into a function that would be a good guard against indices misuse.

sharptooth
+2  A: 

That's a copy-paste mistake, avoid copy paste.

As for your solution, its not much better. The mistake can still slip between tons of code. I tend to use meaningful names even for loop temporary variables.

Nuno Furtado
yup, "row" and "column" are perfectly valid variable names but almost everyone seems to prefer "i" and "j"...
Dan Sydner
David Johnstone
+4  A: 

One of the simplest and cleanest solutions is to place the contents of the inner loop into a method so it becomes:

for (int i = 0; i < length; i++)
{
    DoSomething();
}

private void DoSomething(int outerValue)
{
    for (int i = 0; i < length; i++)
    {
     // Do something else
    }

}
Mark
It doesn't compile :) but seriously, it's a great idea. Most double loops should be in separate functions anyway.
Daniel Daranas
Simon P Stevens
Personally I think that i )
Mark
+6  A: 

Don't use i & j (or any other single letter variable) as index names. Use proper names and you will not get into this type of problems.

Naveen
Simon P Stevens
A: 

I use 'ii' and 'jj' for transient loop counters if I really need them - they are easier to search for than 'i' and 'j' and also easier to spot in examples like the above. To go one better you can actually use a real variable name. If you're looping over a string then you can call it characterIndex or something. It's more typing, but it documents itself and saves time on debugging obscure problems later.

Better still would be to avoid numerical counters and use named iterators over a collection. They make the intent clearer, in my opinion.

Finally, if possible it's nice to do away with the loop entirely: Boost::Foreach is one way of doing this in C++, although I generally prefer to use languages such as Python which natively allow direct iteration over the contents of a container without a need for incrementing an index value or iterator.

Kylotan
+3  A: 

For me, the 'code smell' here is 'lots of code'.

If the amount of code in the loops is particularly large, the distance between the inner and outer loops means that they're not as likely to be compared against each other for correctness.

Admittedly, looking at the start of the inner loop in isolation should bring the issue to your attention, but having the main structure in as small a section of code as possible gives your brain less to digest.

It may be possible to extract the 'lots of code' sections into separate functions/methods, in order to reduce the size of the main structure - but this may not alway be practical.

Also, I'd say that 'i1' isn't a particulary good choice of variable name, as that tends to encourage 'i2', 'i3' etc, which doesn't really lead to understandable code. Maybe replacing all of the loop variables with something more meaningful would help the clarity of the code, and reduce the chances of the original error.

belugabob
A: 

Try to use more declarative loop constructs. For instance, if you don't really need indices (those is and js) and your programming environment allows for it, you can use a foreach construct to iterate over the collection.

Anton Gogolev
+1  A: 

leverage your IDE, on VS, try to use this: http://msdn.microsoft.com/en-us/library/z4c5cc9b(VS.80).aspx

sample: type for, then press Tab Tab successively

Michael Buen
What would be the code snippet than ?
YordanGeorgiev
A: 

As in this as in many things, there's some excellent advice in Steve McConnell's Code Complete. It would be well worth your time to read what he's got to say about building good looping code. I don't have my copy of the book handy here but the whole book is worth your time.

Onorio Catenacci
+1  A: 

I came here to be smart and say "I just write it right the first time". But then I saw your example and, well, I've done that too many times myself.

When you need nested loops like that, my only solution is to be alert and thinking when you write the code.

Where possible, using iterators and for each loops are nice.

Also, I can't see how your suggested solution is going to be any better. And it doesn't look as nice either.

David Johnstone
+1  A: 

First of all, reduce the loop body size, i.e. move stuff to separate functions. It is generally a bad idea to have functions longer than what can fit into the screen, so loops should be even smaller.

Secondly, use meaningful variable names in cases like this. I would only use i and j in simple loops with a few lines of code. For instance, if you are going through a two-dimensional array, "col" and "row" would make much more sense, make the code easier to read ("which was which?") and easier to spot mistakes like this.

Makis
+2  A: 

My top advice (in no particular order) for writing better loop code (much of this is from the excellent book Code Complete):

  1. Avoid multiple exit points for loops.
  2. Use continue/break sparingly.
  3. Refactor nested loops into separate routines, when possible.
  4. Use meaningful variable names to make nested loops readable.
  5. Use foreach() loops when possible, rather than for(i=...) loops.
  6. Enter the loop from one location only. Don't jump into a loop with goto's. Ever.
  7. Put initialization code immediately before the loop.
  8. Keep loop initialization statements with the loop they are related to.
  9. Avoid reusing variables between non-nested loops. 10.Limit the scope of loop-index variables to the loop itself.
  10. Use while(true) for infinite loops, rather than for(;;)
  11. In languages that provide block constructs (e.g. '{' and '}') use them rather than indenting to enclose the statements of a loop. Yes, even for single line loops.
  12. Avoid empty loops.
  13. Avoid placing housekeeping chores in the middle of a loop, place them at the beginning and/or end instead.
  14. Make each loop perform only one function. Avoid mixing responsibilities in a single loop.
  15. Make loop termination conditions obvious.
  16. Don't monkey with the loop index variable of a for() loop to make it terminate.
  17. Avoid code that depends on the loop indexer's final value.
  18. Consider using safety counters in complex loops - they can be checked to make sure the loop doesn't execute too many, or too few times.
  19. Use break statements, when possible, to terminate while loops.
  20. When possible, make your loops short enough to view all at once.
LBushkin