Is WHILE TRUE...BREAK...END WHILE a good design?

Question

I've been looking at some code recently where the author used something similar to this (pseudo-code):

WHILE TRUE
    OpenAFile
    IF Failed
        BREAK
    ENDIF

    DoSomething
    IF NOT OK
        BREAK
    ENDIF

    ...etc...

    BREAK

END WHILE

Arguments for using this design was speed (due to 'fall through' routines) and readability (no nasty indenting of multiple IFs).

Arguments against has been that it's nothing but a nasty GOTO.

My question: Is this a good design pattern?

EDIT: NOTE! The example above isn't intended to 'do' anything, just demonstrate the code style.

Answer 1

I like this design. The only way the code flows is from top to bottom (Or in the loop). That is why it isn't nearly as bad a a GOTO.

Also, as far as readability goes, it is better than the alternative of nested ifs.

Answer 2

No, it's just abusing a loop (unless you're actually looping on something, but it doesn't look like it). This should be a subroutine, and each BREAK should be a RETURN. That both avoids wrapping the entire body in an IF and makes the intention of the check obvious.

Answer 3

Multiple exit points is generally to be avoided. However, if you keep it short and simple enough, this can be used without being too dangerous. Use what's most readable. Sometimes you have to violate one good programming rule to avoid violating another more important one.

Answer 4

unless there's a break at the end, this is an infinite loop!

do { ... } while(false); // or whatever it's written in VB

is a better alternative.

Answer 5

"END WHILE" is just a nasty goto. "IF" is just a nasty goto. You can say the same about any flow-control construct. That means that we just need to have a better definition of what is "nasty". The general rule is that loop constructs and subroutine exits are allowed to go to an earlier point in execution, all others must go to a later point in execution. Since break goes to a later point in execution, we're ok there.

The real rule is something more along the lines of how readable it is. How easy is it for the maintenance guy to follow the flow of execution? If it's easy, problem solved. If not, then we have a bad construct.

This is pretty subjective, but I'd base my decision on:

1. how long is the loop?
2. how close is the break to the beginning or end of the loop?
3. how easy is it to reorder things to move the break (or eliminate it altogether) without repeating code? Moving code to another subroutine may be an option, but can play havoc with scoping, or even just having things close together that are conceptually close together.

That said, what you look like you're doing is implementing exceptions. If you have a language that allows for exceptions, that may provide a cleaner method. If you don't have a language that allows for exceptions, then what you're doing may be about as clean as you get.

Answer 6

Yes, especially for emulating do-while-do loops (Do-Loop constructs in VB) in languages that don't have the construct, such as java. These are loops are somewhat between do-while and while-do loops. Cases where this is a good idea are often easy to spot, because you have some obvious code duplication before and in the loop (for while-do) and in the loop and after it (for do-while). By moving the guard into the body of the loop, you can reduce duplication.

Similarly, it is better to return from a function when an error condition appears, rather than adding another layer:

 int f(int x, int y) {
    if (y < 0) { // or some other error condition
       return 0; // or throw exception
    }
    ... // perform operations
 }

v.

 int f(int x, int y) {
    if (0 <= y) { // or some other error condition
       ... // perform operations
    } else {
       return 0;
    }
 }

See also: Code Complete 2's Chapter 16 on constructing loops.

Answer 7

I think the design pattern is excellent but your example of it is poor.

How many times do you see loops like

Read a line from a file
while (not end of file)
    do stuff
    read a line from a file
end while

In this case you are reading from the file in two different places (hence duplicating yourself). What if you determine that you need to change the variable you are reading into, you need to change the variable pointing to the file, or something else like the number of bytes to read, the record splitting character, etc... You need to do it in two places.

even more dramatic is an example in SQL

fetch from cursor into cola, colb, colc
while not end of cursor
   do stuff
   fetch from cursor into cola, colb, colc
end while

In SQL what happens if you need an additional column? You have to change the code in two places...

Anyway these are just some examples. There are many types of loops like this. I see them often for things like breaking a string apart, etc..

apply regular expression pattern to string
while match
   do something
   apply regular expression pattern to string
end while

If the pattern changes, you have to remember to change the pattern in both places....

Isn't it easier just to go

while true (or 1=1 or whatever)
    prepare results for statement once
    if (do check once)
       break
    end if
    do statement once
end while

I would say that using a goto in this pattern is a valid use of a GOTO. They are not evil if used right. The problem is people used them to jump all over the code (like in the middle of a different function). After all many assembly languages only have GOTO (through branch and jump statements).

As for your example, it is bad because there are a bunch of exits. It should probably be in a function

function codeab (filename)  {
    open file
    if failed return error
    do something
    if failed return error
    return data structure with result
}

while (whatever the loop condition actually is) {
   result = codeab(filename)
   if error
      additional error handling
   do something with result
}

It's more clear. But I don't know a lot about the code you posted so I can't say for sure. But the pattern of exiting from the middle of the loop is very useful and is a "good" use of GOTO.

Answer 8

Breaks and continues are not equivalent to gotos and don't deserve the bad wrap. They are merely syntactic sugar to replace a much more complex if/else over the rest of the block.

Answer 9

Sometimes it's very good design. See Structured Programing With Goto Statements by Donald Knuth for some examples. I use this basic idea often for loops that run "n and a half times," especially read/process loops. However, I generally try to have only one break statement. This makes it easier to reason about the state of the program after the loop terminates.

Answer 10

I routinely use break and continue in loops because using them means I don't have to use something worse. The amount of code you have to write in some circumstances to implement the knee-jerk reaction "good design" can be ridiculous and counter-productive. The code (with break and continue) is readable and easy to understand. I really don't understand why it is considered by anyone to be bad design.

Answer 11

Have you every programmed using AppleEvents? Nearly every single routine returns an OSErr when something (rarely) goes wrong, but being a diligent engineer, you check all the return values. Your code starts to smell bad as a result. It becomes better smelling to do something like this (this was from 12 years ago - forgive my memory):

#define BEGIN_AE_HANDLER while (true) {
#define END_AE_HANDLER break; }
#AE_CHECK_ERROR(exp) if ((exp) != noErr) { break; }

/* code snippet */
OSErr err = noErr;
BEGIN_AE_HANDLER
    AE_CHECK_ERROR(err = AECreateAppleEvent( /* ... */ ));

    AE_CHECK_ERROR(err = AEPutAttributePtr( /* ... */ ));
    /* dozens of other lines, my AppleEvents are wordy */
END_AE_HANDLER
if (err != noErr) { /* tada */ }

And this code reads better than the pile of if's returns and refactoring otherwise. Let me tell you, it was a real blessing when the AE code was wrapped in C++ classes that worked with exceptions instead.

Answer 12

I think I should also add that probably a good use of GOTO (within a function) is centralized error handling (again this does not happen all the time.....). But in this case I would just use GOTO instead of a while loop. If that is what is being tried, just use the GOTO. I should also add that many times it makes sense to handle different errors in different places. But there are times when handling all errors in a central place is the right thing to do.

(note, do not jump to other functions/files/anything else...that is the reason GOTO has such a bad rap)

do something
if error
  goto errorhandler
do something else
if error
  goto errorhandler
if error
  goto errorhandler

....

errorhandler:
handle errors

Some languages have a structure that is better to use

try {
   do something
   do something else
   do yet something else
} catch {
  error handling code
}

But many languages do not have this structure...And even with this structure many languages do not have a way to guarantee that a piece of code is executed no matter what, whether or not there is an error.

Answer 13

Loops are supposed to be used in circumstances where the same code should be capable of running more than once in succession. There are a number of methods to accomplish this, none of which is more correct, but to give you some ideas you can use exception logic, which is useful in reusable compartmentalized code because different portions of code may wish to treat failure differently. Assuming none of the code you are calling throws exceptions you would throw them yourself:

OpenAFile
If Failed Then Throw New IOException("Unable to open file.")
DoSomething
If Not OK Then Throw New ApplicationException("Couldn't DoSomething.")

alternatively you can let natural exception bubble out the calling code or catch exceptions in the method to fail safely depending on the context you use it in:

Public Function TryToDoStuff( ... ) As Boolean
    Try
        OpenAFile
        DoSomething
    Catch ioex As IOException
        Return False
    End Try
    Return True
End Function

For future reference you may wish to be more specific with your question as it will help people understand you true purpose, but ultimately everybody has their own rules and guidelines while they code and the only real mandate is that it gets the job done. Still the methods above are standard approaches because the improve readability, reliability and maintainability (accidentally putting a break in the wrong place is easier than forgetting an End Try since the compiler will notice).

Sidenote: There is a circumstance where using a loop of this nature to accomplish a single action: progressive retry logic can continue trying to do something (assuming it expects there to be issues like waiting for availability or expecting the possibility of a known error), but it does not seem like this is the case here and generally retry and error-ready logic can be implemented in more maintainable ways as well.

Edit:

The example may be causing some confusion; It is difficult to discern whether you are processing more than one item in the loop or if you are using it only as a flow control paradigm. If it is only for flow control many of the above options are more maintainable, though I understand the desire to avoid nested if statements, you can still use something that doesn't insinuate multiple-item processing; chances are if your code is broken down into so many flow-control operations and conditions you can probably improve your abstraction to make your code more maintainable while accomplishing similar results in avoiding overly-nested if statements.

If this is focused toward speed it is achieving such a minor gain, if any, over the alternatives and, if it's really that important to you then you should probably use a Do While instead (and we really ought not even be using VB if optimization is that important to you) since you're currently evaluating an always true condition unnecessarily which is a wasted execution step (probably negating the benefit of the fallthrough); even still you're using just as many conditional evaluations as your code would otherwise and ultimately your callstack is still (presumably) growing and shrinking during execution so compartmentalizing into functions and using returns instead is just as feasible.

Ultimately style is a personal choice, and there's nothing wrong with a break statement, just as there's nothing wrong with a throw or a return; ultimately every branch in code is a goto, we just use different vocabulary to describe the circumstances around the jump. If the code does it's job and is easy to maintain there's nothing wrong with it. If you're using loops instead of other flow control alternatives for the sake of optimization perhaps you should just start writing assembly, then there's absolutely no unnecessary code (if you're good at it ;p).

I think we can all appreciate this comic, but ultimately you will still see goto used, even in well-thought-of code communities. Many linux drivers contain them and, if people really wanted to be nazis about the whole goto thing, this would be an excellent pattern that could change the ways that they use them, but ultimately all of the alternatives are the same thing to the machine.

Answer 14

Pretty much what TheXenocide said:

WHILE TRUE
    OpenAFile
    IF Failed
        BREAK
    ENDIF

    DoSomething
    IF NOT OK
        BREAK
    ENDIF

    ...etc...

    BREAK

END WHILE

could be:

try {
  if (!OpenAFile()) throw new StackOverflowException(); //throws an exception? otherwise the next line would be needed

  if (!DoSomething()) throw new StackOverflowException();

  etc
} catch (StackOverflowException soex) {
  //do nothing, but it can only come from 2 places
}

Of course, if you dont have exceptions, then you are a bit stuffed.... but I'd prefer the IF's myself.

Answer 15

The readability of this code can be improved by moving to GOTO. That's a sign.

OpenAFile
IF Failed GOTO TheEnd
DoSomething
IF NOT OK GOTO TheEnd
...etc...
TheEnd:

Answer 16

Once in a blue moon I feel forced to do something like this. This is the idiom I use, as I believe it's safer, and, to my eye, reads easier.

do {
  doSomething();
  if (error)
    break;
  et c.
} while (false);

Answer 17

One fascinating article is Knuth's "Structured Programming with go to Statements" from 1974 (available in his book 'Literate Programming', and probably elsewhere too). It discusses, amongst other things, controlled ways of breaking out of loops, and (not using the term) the loop-and-a-half statement.

Ada also provides looping constructs, including

loopname:
    loop
        ...
        exit loopname when ...condition...;
        ...
    end loop loopname;

The original question's code is similar to this in intent.

Answer 18

my teacher told me it's bad style but I find it really readable. For example when reading text files and trying to catch chars.

var c : char; arch : text;

begin assign(arch,'text.txt'); reset(arch); while true do begin read(arch,c); if not ( c in [#10,#12,#13,#32] ) then pDoSomething(c); if c = #26 { or eof(arch)} then break; end; end.