Code Smell – Is Assignment Inside a Condition Bad Practice?

code smellconditionsloops

Many times I have to write a loop that requires initialization of a loop condition, and an update every time the loop executes. Here's one example:

List<String> currentStrings = getCurrentStrings();
while(currentStrings.size() > 0) {
  doThingsThatCanAlterCurrentStrings();
  currentStrings = getCurrentStrings();
}

One things I dislike about this code is the duplicate call to getCurrentStrings(). One option is to add the assignment to the condition as follows:

List<String> currentStrings;
while( (currentStrings = getCurrentStrings()).size() > 0) {
  doThingsThatCanAlterCurrentStrings();
}

But while I now have less duplication and less code, i feel that my code is now harder to read. On the other hand, it is easier to understand that we are looping on a variable that may be changed by the loop.

What is the best practice to follow in cases like this?

Best Answer

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.

Related Topic