7

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?

  • 5
    Doing a call like GetCurrentStrings() once outside a while loop, and then calling it inside the loop, is a very common, well understood, accepted as best practice pattern. It's not code duplication; you have to call GetCurrentStrings() once outside the loop to establish the initial condition for the while. – Robert Harvey Mar 20 '14 at 15:57
10

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.

| improve this answer | |
  • Liked the multiple explanations and examples. Thanks! – vainolo Mar 24 '14 at 8:28
4

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;
    }
}
| improve this answer | |
  • Indeed, for(;;) is a dead giveaway, that there's a break (or return) inside the loop. When not abused, it can make for a very clear yet concise loop construct. – hyde Mar 20 '14 at 16:12
1

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.

| improve this answer | |
0

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.

| improve this answer | |
0

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.

| improve this answer | |
  • 2
    Why would you ever write something as a for loop when you don't know ahead of time how many iterations you're going to have? That's what while is for. – Robert Harvey Mar 20 '14 at 16:01
  • That's a whole question in itself, but for loops have a defined initial condition, stop condition, update, and body. Usually when those four elements exist and are not overly complex, it's preferable to use a for loop. For one thing, it limits the scope of loop variables to the loop body itself. For another, it provides an unambiguous place to look for those elements, which is a big readability boost in a large loop. Loops with fixed iteration counts happen to fit the for loop criteria, but that doesn't mean they're the only kind of loops that do. – Karl Bielefeldt Mar 20 '14 at 16:30
  • Well, if I were using a loop variable (that increments or decrements in each loop iteration), I wouldn't use a while anyway. That's not the case in the OP. – Robert Harvey Mar 20 '14 at 16:32
  • Sure it is. currentStrings is a loop variable. It changes on every iteration and is the basis for the stop condition. It doesn't have to be a counter. – Karl Bielefeldt Mar 20 '14 at 16:36
  • Yeah, thought I qualified my use of the term "loop variable" in my last comment. Oh, well. – Robert Harvey Mar 20 '14 at 16:37

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.