Is doing an assignment inside a condition considered a code smell?

Question

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?

Answer 1

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 the variable s 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.

Answer 2

The fundamental problem here, it seems to me, is that you have a N plus one half loop, and those are always a bit messy to express. In this particular case, you could hoist the "half" part of the loop into the test, as you have done, but it looks very awkward to me. Two ways of expressing that loop may be:

Idiomatic C/C++ in my opinion:

for (;;) {
    List<String> currentStrings = getCurrentStrings();
    if (!currentStrings.size())
        break;
    doThingsThatCanAlterCurrentStrings();
}

Strict "structured programming", which tends to frown on break:

for (bool done = false; !done; ) {
    List<String> currentStrings = getCurrentStrings();
    if (currentStrings.size() > 0) {
        doThingsThatCanAlterCurrentStrings();
    } else {
        done = true;
    }
}

Answer 3

That sort of construction shows up when doing some sort of buffered read, where the read fills a buffer and returns the number of bytes read, or 0. It's a pretty familiar construct.

There's a risk someone might think you got = confused with ==. You might get a warning if you're using a style checker.

Honestly, I'd be more bothered by the (...).size() than the assignment. That seems a little iffy, because you're dereferencing the assignment. ;)

I don't think there's a hard rule, unless you're strictly following the advice of a style checker & it flags it with a warning.

Answer 4

Duplication is like medicine. It's harmful in high doses, but can be beneficial when appropriately used in low doses. This situation is one of the helpful cases, as you've already refactored out the worst duplication into the getCurrentStrings() function. I agree with Sebastian, though, that it's better written as a for loop.

Along those same lines, if this pattern is coming up all the time, it might be a sign that you need to create better abstractions, or rearrange responsibilities between different classes or functions. What makes loops like these problematic is they rely heavily on side effects. In certain domains that's not always avoidable, like I/O for example, but you should still try to push side effect-dependent functions as deep into your abstraction layers as possible, so there aren't very many of them.

In your example code, without knowing the context, the first thing I would do is try to find a way to refactor it to do all the work on my local copy of currentStrings, then update the external state all at once. Something like:

for(String string: getCurrentStrings()) {
    doSomething(string);
}
clearCurrentStrings();

If the current strings are being updated by another thread, an event model is often in order:

void onCurrentStringAdd(String addedString) {
    doSomething(addedString);
}

You get the picture. There's usually some way to refactor your code to not depend on side effects as much. If that's not practical, you can often avoid duplication by moving some responsibility to another class, as in:

CurrentStrings currentStrings = getCurrentStrings();
while (!currentStrings.empty()) {
    currentStrings.alter();
}

It might seem like overkill to create a whole new CurrentStrings class for something that can be mostly served by a List<String>, but it will often open up a whole gamut of simplifications throughout your code. Not to mention the encapsulation and type-checking benefits.

Answer 5

The former code seems more rational and readable to me and the whole loop also makes perfect sense. I'm not sure about the context of the program, but there is nothing wrong with the loop structure in essence.

The later example seems trying to write a Clever code, that is absolutely confusing to me in the first glance.

I also agree with Rob Y's answer that in the very first glance you might think it should be an equation == rather than an assignment, however if you actually read the while statement to the end you will realize it's not a typo or mistake, however the problem is that you can't clearly understand Why there is an assignment within the while statement unless you keep the function name exactly same as doThingsThatCanAlterCurrentStrings or add an inline comment that explains the following function is likely to change the value of currentStrings.