while(true) and loop-breaking - anti-pattern?

Question

Consider the following code:

public void doSomething(int input)
{
   while(true)
   {
      TransformInSomeWay(input);

      if(ProcessingComplete(input))
         break;

      DoSomethingElseTo(input);
   }
}

Assume that this process involves a finite but input-dependent number of steps; the loop is designed to terminate on its own as a result of the algorithm, and is not designed to run indefinitely (until cancelled by an outside event). Because the test to see if the loop should end is in the middle of a logical set of steps, the while loop itself currently doesn't check anything meaningful; the check is instead performed at the "proper" place within the conceptual algorithm.

I was told that this is bad code, because it is more bug-prone due to the ending condition not being checked by the loop structure. It's more difficult to figure out how you'd exit the loop, and could invite bugs as the breaking condition might be bypassed or omitted accidentally given future changes.

Now, the code could be structured as follows:

public void doSomething(int input)
{
   TransformInSomeWay(input);

   while(!ProcessingComplete(input))
   {
      DoSomethingElseTo(input);
      TransformInSomeWay(input);
   }
}

However, this duplicates a call to a method in code, violating DRY; if TransformInSomeWay were later replaced with some other method, both calls would have to be found and changed (and the fact that there are two may be less obvious in a more complex piece of code).

You could also write it like:

public void doSomething(int input)
{
   var complete = false;
   while(!complete)
   {
      TransformInSomeWay(input);

      complete = ProcessingComplete(input);

      if(!complete) 
      {
         DoSomethingElseTo(input);          
      }
   }
}

... but you now have a variable whose only purpose is to shift the condition-checking to the loop structure, and also has to be checked multiple times to provide the same behavior as the original logic.

For my part, I say that given the algorithm this code implements in the real world, the original code is the most readable. If you were going through it yourself, this is the way you'd think about it, and so it would be intuitive to people familiar with the algorithm.

So, which is "better"? is it better to give the responsibility of condition checking to the while loop by structuring the logic around the loop? Or is it better to structure the logic in a "natural" way as indicated by requirements or a conceptual description of the algorithm, even though that may mean bypassing the loop's built-in capabilities?

Answer 1

Stick with your first solution. Loops can become very involved and one or more clean breaks can be a lot easier on you, anyone else looking at your code, the optimizer, and the program's performance than elaborate if-statements and added variables. My usual approach is to set up a loop with something like the "while (true)" and get the best coding I can. Often I can and do then replace the break(s) with a standard loop control; most loops are pretty simple, after all. The ending condition should be checked by the loop structure for the reasons you mention, but not at the cost of adding whole new variables, adding if-statements, and repeating code, all of which are even more bug prone.

Answer 2

It's only a meaningful problem if the loop body is non-trivial. If the body is so small, then analyzing it like this is trivial, and not a problem at all.

Answer 3

I've trouble to find the other solutions better than the one with break. Well, I prefer the Ada way which allows to do

loop
  TransformInSomeWay(input);
exit when ProcessingComplete(input);
  DoSomethingElseTo(input);
end loop;

but the break solution is the cleanest rendering.

My POV is that when there is a missing control structure, the cleanest solution is to emulate it with other control structures, not using data flow. (And similarly, when I have a data flow problem to prefer pure data flow solutions to the one involving control structures *).

It's more difficult to figure out how you'd exit the loop,

Don't be mistaken, the discussion wouldn't take place if the difficulty wasn't mostly the same: the condition is the same and present at the same place.

Which of

while (true) {
   XXXX
if (YYY) break;
   ZZZZ;
}

and

while (TTTT) {
    XXXX
    if (YYYY) {
        ZZZZ
    }
}

render better the intended structure? I'm voting for the first, you don't have to check that the conditions matches. (But see my indentation).

Answer 4

while(true) and break is a common idiom. It's the canonical way to implement mid-exit loops in C-like languages. Adding a variable to store the exit condition and an if() around the second half of the loop is a reasonable alternative. Some shops may Officially Standardize on one or the other, but neither one is superior; they are equivalent.

A good programmer working in these languages should be able to know what's going on at a glance if he or she sees either one. It might make a good little interview question; hand somebody two loops, one using each idiom, and ask 'em what the difference is (proper answer: "nothing", with maybe an addition of "but I habitually use THAT one.")

Answer 5

People say it is bad practice to use while (true), and a lot of the time they are right. If your while statement contains lots of complicated code and it is unclear when you are breaking out of if, then you are left with difficult to maintain code and a simple bug could cause an infinite loop.

In your example you are correct to use while(true) because your code is clear and easy to understand. There is no point creating inefficient and harder to read code, simply because some people consider it always bad practice to use while(true).

I was faced with a similar problem:

$i = 0
$key = $str.'0';
$key1 = $str1.'0';
while (isset($array][$key] || isset($array][$key1])
{
    if (isset($array][$key]))
    {
        // Code
    }
    else
    {
        // Code
    }
    $key = $str.++$i;
    $key1 = $str1.$i;
}

Using do ... while(true) avoids isset($array][$key] unnecessarily being called twice, the code it shorter, cleaner, and easier to read. There is absolutely no risk of an infinite loop bug being introduced with else break; at the end.

$i = -1;
do
{
    $key = $str.++$i;
    $key1 = $str1.$i;
    if (isset($array[$key]))
    {
        // Code
    }
    elseif (isset($array[$key1]))
    {
        // Code
    }
    else
        break;
} while (true);

It's good to follow good practices and avoid bad practices but there are always exceptions and it's important to keep an open mind and realise those exceptions.

Answer 6

Your alternatives are not as bad as you may think. Good code should:

1. Clearly indicate with good variable names / comments under what conditions the loop should be terminated. (The breaking condition should not be hidden)

2. Clearly indicate what is the order of doing operations. This is where putting the optional 3rd operation ( DoSomethingElseTo(input) ) inside the if is a good idea.

That said, it is easy to let perfectionist, brass-polishing instincts consume a lot of time. I would just tweak the if as mentioned above; comment the code and move on.

Answer 7

If a particular flow of control is naturally expressed as a break statement, it is essentially never a better choice to use other flow control mechanisms to emulate a break statement.

(note that this includes partially unrolling the loop and sliding the loop body down so that the loop begin coincides with the conditional test)

If you can reorganize your loop so that the flow of control is naturally expressed by having a conditional check at the beginning of the loop, great. It might even be worth spending some time thinking about whether that is possible. But it not, don't try to fit a square peg into a round hole.

Answer 8

You could also go with this:

public void doSomething(int input)
{
   while(TransformInSomeWay(input), !ProcessingComplete(input))
   {
      DoSomethingElseTo(input);
   }
}

Or less confusingly:

bool TransformAndCheckCompletion(int &input)
{
   TransformInSomeWay(input);
   return ProcessingComplete(input))
}

public void doSomething(int input)
{
   while(TransformAndCheckCompletion(input))
   {
      DoSomethingElseTo(input);
   }
}

Answer 9

When the complexity of the logic becomes unwieldy then using an unbounded loop with mid-loop breaks for flow control does actually make some sense. I have a project with some code which looks like the following. (Notice the exception at the end of the loop.)

L: for (;;) {
    if (someBool) {
        someOtherBool = doSomeWork();
        if (someOtherBool) {
            doFinalWork();
            break L;
        }
        someOtherBool2 = doSomeWork2();
        if (someOtherBool2) {
            doFinalWork2();
            break L;
        }
        someOtherBool3 = doSomeWork3();
        if (someOtherBool3) {
            doFinalWork3();
            break L;
        }
    }
    bitOfData = getTheData();
    throw new SomeException("Unable to do something for some reason.", bitOfData);
}
progressOntoOtherThings();

To do this logic without unbounded loop breaking, I would end up with something like the following:

void method() {
    if (!someBool) {
        throwingMethod();
    }
    someOtherBool = doSomeWork();
    if (someOtherBool) {
        doFinalWork();
    } else {
        someOtherBool2 = doSomeWork2();
        if (someOtherBool2) {
            doFinalWork2();
        } else {
            someOtherBool3 = doSomeWork3();
            if (someOtherBool3) {
                doFinalWork3();
            } else {
                throwingMethod();
            }
        }
    }
    progressOntoOtherThings();
}
void throwingMethod() throws SomeException {
        bitOfData = getTheData();
        throw new SomeException("Unable to do something for some reason.", bitOfData);
}

So the exception is now thrown in a helper method. Also it has quite a lot of if ... else nesting. I'm not satisfied with this approach.

However, there is a third approach, which is probably most acceptable. Put the logic into a method, and just return when done. So if we take the first approach above and slightly refactor it, we get this:

void method()
{
    myMethod(someBool);
    progressOntoOtherThings();
}

void myMethod()
{
    if (someBool) {
        someOtherBool = doSomeWork();
        if (someOtherBool) {
            doFinalWork();
            return;
        }
        someOtherBool2 = doSomeWork2();
        if (someOtherBool2) {
            doFinalWork2();
            return;
        }
        someOtherBool3 = doSomeWork3();
        if (someOtherBool3) {
            doFinalWork3();
            return;
        }
    }
    bitOfData = getTheData();
    throw new SomeException("Unable to do something for some reason.", bitOfData);
}