ForEach vs. While Loop – Coding Style and Performance

coding-styleinvariantsloopssearchstructured-programming

This is the most popular way (it seems to me) of checking if a value is in an array:

for (int x : array)
{
    if (x == value)
        return true;
}
return false;

However, in a book I’ve read many years ago by, probably, Wirth or Dijkstra, it was said that this style is better (when compared to a while-loop with an exit inside):

int i = 0;
while (i < array.length && array[i] != value)
    i++;
return i < array.length;

This way the additional exit condition becomes an explicit part of the loop invariant, there are no hidden conditions and exits inside the loop, everything is more obvious and more in a structured-programming way. I generally preferred this latter pattern whenever possible and used the for-loop to only iterate from a to b.

And yet I cannot say that the first version is less clear. Maybe it is even clearer and easier to understand, at least for very beginners. So I’m still asking myself the question of which one is better?

Maybe someone can give a good rationale in favor of one of the methods?

Update: This is not a question of multiple function return points, lambdas or finding an element in an array per se. It’s about how to write loops with more complex invariants than a single inequality.

Update: OK, I see the point of people who answer and comment: I mixed-in the foreach loop here, which itself is already much more clear and readable than a while-loop. I should not have done that. But this is also an interesting question, so let's leave it as it is: foreach-loop and an extra condition inside, or a while-loop with an explicit loop invariant and a post-condition after. It seems that the foreach-loop with a condition and an exit/break is winning. I will create an additional question without the foreach-loop (for a linked list).

Best Answer

I think for simple loops, such as these, the standard first syntax is much clearer. Some people consider multiple returns confusing or a code smell, but for a piece of code this small, I do not believe this is a real issue.

It gets a bit more debatable for more complex loops. If the loop's contents cannot fit on your screen and has several returns in the loop, there is an argument to be made that the multiple exit points can make the code more difficult to maintain. For example, if you had to ensure some state maintenance method ran before exiting the function, it would be easy to miss adding it to one of the return statements and you would cause a bug. If all the end conditions can be checked in a while loop, you only have one exit point and can add this code after it.

That said, with loops especially it is good to try and put as much logic as possible into separate methods. This avoids a lot of cases where the second method would have advantages. Lean loops with clearly separated logic will matter more than which of these styles you use. Also, if most of your application's code base is using one style, you should stick with that style.

Related Solutions

Visual Basic Loops – Best Practice for Loop Exit via Iterator Modification in Visual Basic

Performance shouldn't be your first priority. Of higher importance should generally be code readability and maintainability. Exit For states, in code, exactly what you want to do, semantically. It's blatantly obvious to the reader exactly what the code is doing.

When setting the loop variable it's a lot less clear. I need to spend time trying to figure out what's going on; is the loop variable set to the end, ahead one, the start, somewhere in the middle, or what? That's time I shouldn't need to be spending.

Beyond that, you're also now repeating the constant 10 in that loop. What if someone goes to change the loop variable to go to 15 instead. They may not bother to look through the entire body of the loop and just expect it to work fine going up to 15 instead. You now have a very evil bug. When I come along to debug the code I may not know if someone intended the loop variable to be moved to another internal location, or if it really should be the end of the loop.

Another key difference is that End For ends the current iteration of the loop right now. Setting the loop variable means that the current iteration ends and then the loop stops. If there is more code in the current iteration of the loop then that's different. It's only the same if the code is at the end of the loop.

As for the performance differences, I wouldn't expect there to be any particularly significant difference. In the event there is a difference at all, it's almost certainly going to be very, very, very tiny, and not enough to matter. If, by some surprising series of events you've managed to determine that this path of this loop is executed so frequently, and in a context where performance is so vital, that this change results in a noticeable and essential performance difference through your in depth performance tests and profiling, then consider using the less readable version (possibly with supporting comments).

Code Smell – Is Assignment Inside a Condition Bad Practice?

First, I would definitely frame the first version as a for-loop:

for (List<String> currentStrings = getCurrentStrings();
     currentStrings.size() > 0; // if your List has an isEmpty() prefer it
     currentStrings = getCurrentStrings()) {
  ...
}

Unfortunately there's no idiomatic way in C++, Java or C# that I know of to get rid of the duplication between initializer and incrementer. I personally like abstracting the looping pattern into an Iterable or Enumerable or whatever your language provides. But in the end, that just moves the duplication into a reusable place. Here's a C# example:

IEnumerable<T> ValidResults<T>(Func<T> grab, Func<bool, T> validate) {
  for (T t = grab(); validate(t); t = grab()) {
    yield return t;
  }
}
// != null is a common condition
IEnumerable<T> NonNullResults<T>(Func<T> grab) where T : class {
  return ValidResults(grab, t => t != null);
}

Now you can do this:

foreach(var currentStrings in NonNullResults(getCurrentStrings)) {
  ...
}

C#'s yield makes writing this easy; it's uglier in Java or C++.

C++ culture is more accepting of assignment-in-condition than the other languages, and implicit boolean conversions are actually used in some idioms, e.g. type queries:

if (Derived* d = dynamic_cast<Derived*>(base)) {...}

The above relies on the implicit conversion of pointers to bool and is idiomatic. Here's another:

std::string s;
while (std::getline(std::cin, s)) {...}

This modifies within the condition.

The common pattern, however, is that the condition itself is trivial, usually relying completely on some implicit conversion to bool. Since collections don't do that, putting an empty test there would be considered less idiomatic.

C culture is even more accepting, with the fgetc loop idiom looking like this:

int c;
while((c = fgetc(stream)) != EOF) {...}

But in higher-level languages, this is frowned upon, because with the higher level usually comes lesser acceptance of tricky code.

Best Answer

Related Solutions

Visual Basic Loops – Best Practice for Loop Exit via Iterator Modification in Visual Basic

Code Smell – Is Assignment Inside a Condition Bad Practice?

Related Topic