Should I expect to see the counter in `for` loop changed inside its body? [closed]

Question

I'm reading someone else's code and they separately increment their for loop counter inside the loop, as well as including the usual afterthought. For example:

for( int y = 4; y < 12; y++ ) {
    // blah
    if( var < othervar ) {
        y++;
    }
    // blah
}

Based on the majority of code others have written and read, should I be expecting to see this?

Answer 1

The practice of manipulating the loop counter within a for loop is not exactly widespread. It would surprise many of the people reading that code. And surprising your readers is rarely a good idea.

The additional manipulation of your loop counter adds a ton of complexity to your code because you have to keep in mind what it means and how it affects the overall behavior of the loop. As Arkady mentioned, it makes your code much harder to maintain.

To put it simply, avoid this pattern. When you follow "clean code" principles, especially the single layer of abstraction (SLA) principle, there is no such thing as

for(something)
  if (somethingElse)
   y++

Following the principle requires you to move that if block into its own method, making it awkward to manipulate some outer counter within that method.

But beyond that, there might be situations where "something" like your example makes; but for those cases - why not use a while loop then?

In other words: the thing that makes your example complicated and confusing is the fact that two different parts of the code change your loop counter. So another approach could look like:

 while (y < whatever) {
   ...
   y = determineY(y, who, knows);
 }

That new method could then be the central place to figure how to update the loop variable.

Answer 2

I beg to differ with the acclaimed answer above. There is nothing wrong with manipulating loop control variable inside the loop body. For example, here is the classical example of cleaning up the map:

for (auto it = map.begin(), e = map.end(); it != e; ) {
    if (it->second == 10)
        it = map.erase(it);
    else
        ++it;
}

Since I have been rightfully pointed out to the fact that iterators are not the same as numeric control variable, let's consider an example of parsing the string. Let's assume the string consists of a series of characters, where characters prefixed with '\' are considered to be special and need to be skipped:

for (size_t i = 0; i < s_len; ++i) {
    if (s[i] == '\\') {
       ++i;
       continue;
    }
    process_symbol(s[i]);
}

Answer 3

Use a while loop instead.

While you can do this with a for loop, you should not. Remember that a program is like any other piece of communication, and must be done with your audience in mind. For a program, the audience includes the compiler and the next person to do maintenance on the code (likely you in about 6 months).

To the compiler, the code is taken very literally -- set up a index variable, run the loop body, execute the increment, then check the condition to see if you are looping again. The compiler doesn't care if you monkey with the loop index.

To a person however, a for loop has a specific implied meaning: Run this loop a fixed number of times. If you monkey with the loop index, then this violates the implication. It's dishonest in a sense, and it matters because the next person to read the code will either have to spend extra effort to understand the loop, or will fail to do so and will therefore fail to understand.

If you want to monkey with the loop index, use a while loop. Especially in C/C++/related languages, a for loop is exactly as powerful as a while loop, so you never lose any power or expressiveness. Any for loop can be converted to a while loop and vice versa. However, the next person who reads it won't depend on the implication that you don't monkey with the loop index. Making it a while loop instead of a for loop is a warning that this kind of loop may be more complicated, and in your case, it is in fact more complicated.

Answer 4

If you increment inside the loop, make sure to comment it. A canonical example (based on a Scott Meyers Effective C++ item) is given in the Q&A How to remove from a map while iterating it? (verbatim code copy)

for (auto it = m.cbegin(); it != m.cend() /* not hoisted */; /* no increment */)
{
  if (must_delete)
  {
    m.erase(it++);    // or "it = m.erase(it)" since C++11
  }
  else
  {
    ++it;
  }
}

Here, both the non-constant nature of the end() iterator and the increment inside the loop are surprising, so they need to be documented. Note: the loop hoisting here is after all possible so probably should be done for code clarity.