55

From time to time, I use a while(1) block to flatten a succession of if..else going out of proportion. It goes along these lines.

Instead of doing:

// process 
if (success) {
  // process 
  if (success) {
    //process
    if (success) {
      // etc
    }
  }
}

I do:

while (1) {
  // process
  if (!success) break;
  // process
  if (!success) break;
  // process
  if (!success) break;
  // etc
  break;
}

I am a little annoyed by the implicit jump at the end of the while. Could I get away with a leaner construct (ie no break at the end)?

I could trade the final break with a variable (or register?). That's not exactly leaner or clearer.

int once = 1;
while (once--) {
  // process
  if (!success) break;
  // process
  if (!success) break;
  // process
  if (!success) break;
  // etc
}

A for loop would look a bit better (C99):

for (int once = 1 ; once--; once) {
  // process
  if (!success) break;
  // process
  if (!success) break;
  // process
  if (!success) break;
  // etc
}

I thought about using a switch case. It does not look much better , though it would work.

switch (1) { default:
  // process
  if (!success) break;
  // process
  if (!success) break;
  // process
  if (!success) break;
  // etc
}

In that particular case the concept of a label seems unbeatable.

// process
if (!success) goto end;
// process
if (!success) goto end;
// process
if (!success) goto end;
// etc

end:

What other approach do you guys know/use?

Philippe A.
  • 2,885
  • 2
  • 28
  • 37
  • 70
    Allow me to express strong, strong disapproval of using a loop construct for something that isn't a loop. @ouah's suggestion of hoisting nested conditions out to a separate function is by far the best option here. – Russell Borogove Aug 05 '14 at 20:49
  • 9
    related: [How to avoid “if” chains?](http://stackoverflow.com/questions/24430504/how-to-avoid-if-chains) – chue x Aug 06 '14 at 02:36
  • 4
    Whatever you do, don't try to make a loop into a label, just because someone told you labels are bad and loops aren't. – user253751 Aug 06 '14 at 03:06
  • 2
    Advance to c++ and throw() ;) – SF. Aug 06 '14 at 09:36
  • Your while loop is not the same as your original code. Your original code processes etc if first 2 succes and if first 3 succes. – Pieter B Aug 06 '14 at 11:15
  • 4
    If you had used a `goto` for the second solution `while(1) { if(error) break; }`, it would be much more honest. You use the loop exit as a goto label without telling it anywhere. This is not a `while` loop, it is just obfuscated code. – JensG Aug 06 '14 at 14:34
  • Note that the jump at the end of the while is really only implicit. Even without optimization, any decent compiler will not generate a `jmp` at the end (I tried it with g++) – Hagen von Eitzen Aug 06 '14 at 15:55
  • Might be good on codereview.stackexchange.com – Ryan Dougherty Aug 06 '14 at 16:35
  • Is it really worth going to these lengths to avoid the simple `goto`? Just because it has been abused many times, doesn't mean you have to avoid it like typing those four letters in succession would give you Ebola. – Dolda2000 Mar 05 '17 at 03:56

6 Answers6

146

What other approach do you guys know/use?

You can encapsulate your while loop in a function (and call this function where you had your while loop):

static void process(void)
{
   // process
   if (!success) return;
   // process
   if (!success) return;
   // process
   if (!success) return;
   // process
}

Any halfway decent compiler (e.g., even gcc with optimizations disabled) will inline a static function if it is called once. (Of course some variables may have to be in the lexical scope of process function, in that case just provide them as parameters of the function).

Note that writing code from top to bottom instead of horizontally (e.g., your example with nested if) is called duffing. There is a nice article on the subject here:

"Reading Code From Top to Bottom"

Also, in the Linux kernel coding style there is a specific warning writinh against horizontal code:

"if you need more than 3 levels of indentation, you're screwed anyway, and should fix your program"

ouah
  • 142,963
  • 15
  • 272
  • 331
54

The following is a method very similar to what you're doing with the loops, but without the need for a counter or a break statement at the end.

do
{
    // process
    if (!success) break;
    // process
    if (!success) break;
    // process
    if (!success) break;
    ...
    // No need for a break statement here
}
while(0);
Fiddling Bits
  • 8,712
  • 3
  • 28
  • 46
  • 2
    I already feel the shame of not having thought to that one already! – Philippe A. Aug 05 '14 at 18:01
  • 65
    `while` loops like this are an odd way to avoid `goto`. `goto` can and should be used in the cases that O.P. mentioned. – Pavan Yalamanchili Aug 05 '14 at 18:05
  • @PavanYalamanchili Right. There's very little difference, execution-wise, between this and `goto`, but most of us have been taught to avoid `goto`, whether legitimately or not, no? – Fiddling Bits Aug 05 '14 at 18:14
  • 11
    This is infinitely to goto; the target of break is well defined and immutable. With a goto you could have any number of destination targets - the restrictiveness of break enforces structure. I can imagine copy&pasting a block of code an omitting to change the label name for example. – Clifford Aug 05 '14 at 18:59
  • 43
    @FiddlingBits People are taught not to use `goto` because it can lead to complex and obfuscated code. In this particular case you are essentially replicating what `goto` does naturally by using `do / while`. If someone not familiar with the code starts looking at the code, they will have no idea why you have a loop until they come across the `while(0)`. It does not immediately convey what the code is supposed to be doing. – Pavan Yalamanchili Aug 05 '14 at 19:00
  • 6
    @PavanYalamanchili : Well said, it deserves a clear comment at least - for the poor (and often inexperienced) maintainer! – Clifford Aug 05 '14 at 19:23
  • 3
    An ideal flow-of-control abstraction doesn't require the reader to backtrack or "sidetrack" to determine the semantics. This is about as close as you're going to get in C. – Scott Leadley Aug 05 '14 at 20:04
  • "goto can and should be used" -- No they shouldn't. One bad practice can't be used to justify another. – Jim Balter Aug 06 '14 at 11:24
  • 23
    The hatred of goto derives from Dijkstra trying to force structured programming into a world that didn't want it. Not using goto isn't "good practice"; it's _a 40-year-old irrelevancy taken out of context_. The OP's use case is actually a standard use for goto--one in which it is the _right_ language feature: resulting in cleaner, simpler, and faster code. – geometrian Aug 06 '14 at 11:40
  • 2
    This is the first time I see someone calling this "common", except in macro writing, so in my view, it's everything, but not common. – Sebastian Mach Aug 06 '14 at 15:27
  • @phresnel You're right. I've only seen it in macros as well. I'll update my answer. – Fiddling Bits Aug 06 '14 at 15:28
  • 2
    ... and in macros it is rather used to turn a block into a statement than to allow simpler jump-outs – Hagen von Eitzen Aug 06 '14 at 15:47
  • 2
    While this does provide an alternative, it isn't in any way simple or easy to understand. Converting it into a function call results in the same behaviour without the added logical meltdown. – tcooc Aug 06 '14 at 15:53
  • 2
    "Dijkstra trying to force structured programming into a world that didn't want it." -- What a bizarre assertion. At the time, languages such as FORTRAN and assembler lacked structured control statements, so all code was spaghetti code. "it's a 40-year-old irrelevancy taken out of context" -- No, it isn't. The unstructured nature of gotos and labels makes code harder to read and analyze. In C they are sometimes hard to avoid because of lack of catch/try/finally, destructors, multilevel break/continue, and other features. "faster code" -- uh, no. Compilers optimize 40 years later. – Jim Balter Aug 06 '14 at 19:13
  • 1
    @JimBalter: In the days before structured programming, a common way to implement an "if/then/else" pattern was to have an `if` go to some code way off in the middle of nowhere, which would do the `then` portion of the logic, and then `goto` back to the instruction following the `else` portion, and it was common to share a section of code by setting a variable to indicate where execution should resume after the code is executed, then `goto` that piece of code, and have it `goto` back someplace as indicated by the variable. – supercat Aug 06 '14 at 23:40
  • 3
    @JimBalter: `goto` statements only cause major trouble when they cause execution to cross certain kinds of semantic boundaries. Other constructs are more readable for the 98% of coding structures that they fit, and it would be helpful if languages had a block structure which would behave like a `do { ... } while(false)` but would allow a reader to know it wasn't a loop without the reader having to examine the last line. Perhaps something like `do once { ... }`. – supercat Aug 06 '14 at 23:45
  • @supercat "In the days before structured programming" -- I've been programming since 1965, and I have also studied language design extensively, including psychological factors such as error rates for various constructs ... there really isn't anything you can tell me about this that I don't already know. – Jim Balter Aug 07 '14 at 03:03
  • 1
    @JimBalter, Any commentary of "Go To Statement Considered Harmful" is going to mention the structured programming wars, the structured programming proof, and other aspects of the emergent paradigm of structured programming. Eventually it was realized that structured programming is possible using goto, and some modern style guides tentatively recommend its limited use. The historical prejudice has remained, however. – geometrian Aug 14 '14 at 00:49
  • 1
    Re: speed, there are plenty of trivial examples where goto speeds up program code. The most familiar has been solved by named break, but similar issues exist in such cases as the OP's, where locality is saved. Even modern compilers _can't (in general) optimize a solution using e.g. a boolean to an equivalent goto_. The "loop" solution _does_ compile to the same thing, but it's semantically bletcherous. My point is that sometimes an unconditional break _just makes sense in the algorithm_. Sure you can _emulate_ it with other constructs, but why would you want to? – geometrian Aug 14 '14 at 00:50
  • 1
    Lots of strawmen there. Of course structured programming is possible *in the presence of an occasional goto*; no one ever said otherwise ... but structured programming isn't possible "using goto" as the sole means of flow control, of course. And named break isn't a goto any more than while(1) is a goto. "using e.g. a boolean" -- right; Pascal was a teaching language with limited control structures and the "approved" method was using booleans, which was a bad thing and polluted the minds of many students. – Jim Balter Aug 14 '14 at 01:57
  • 1
    Anyone who took my comment "goto can and should be used -- No they shouldn't" to mean that *named breaks/continues* shouldn't be used doesn't understand this subject, and ignored my comment "The unstructured nature of gotos and labels makes code harder to read and analyze. In C they are sometimes hard to avoid because of **lack of** catch/try/finally, destructors, **multilevel break/continue**, and other features." In any case, the language designers have spoken, as no modern language has goto statements. – Jim Balter Aug 14 '14 at 01:59
  • It's really sad that in the 21st century we are still having these debates based on the features of horrid, obsolete languages like C (and even C++), when there are bright new modern languages like Rust and Nimrod ... the Nimrod compiler generates C code and provides every efficiency and feature of C, including macros (but much much better), with perhaps the sole exception of Duff's Device. – Jim Balter Aug 14 '14 at 02:09
  • @PavanYalamanchili please make that an answer. I think that would be one of the best answers here. – exilit Aug 09 '16 at 09:41
  • @exilit ouah's answer below is the best solution method mentioned here. – Pavan Yalamanchili Aug 09 '16 at 13:34
38

If you arrange that the body of each conditional block generating success is a function as follows or each // process can otherwise be reduced to a boolean expression, such as:

success = f1() ; 
if( success ) 
{
  success = f2() ; 
  if( success ) 
  {
    success = f3() ; 
    if( success ) 
    {
      success = f4()
    }
  }
}

Then you can reduce this to a single boolean expression exploiting short-circuit evaluation:

success = f1() && 
          f2() && 
          f3() && 
          f4() ;

Here f2() will not be called if f1() returns false and the same for each successive call - expression evaluation aborts on the first && operand sub-expression to return false.

Salman A
  • 262,204
  • 82
  • 430
  • 521
Clifford
  • 88,407
  • 13
  • 85
  • 165
  • I like your solution cold logic. However it is not very practical to encapsulate my processing in many different functions. @ouah's variant is more useful in my case. – Philippe A. Aug 05 '14 at 20:11
  • 3
    Agreed it is not perhaps generally applicable and therefore not "the answer", but it is worth mentioning. – Clifford Aug 05 '14 at 20:27
  • Worth an upvote too btw. – Philippe A. Aug 05 '14 at 20:38
  • imho this is the best solution. Perhaps writing multiple functions is more work to do, but it's easier to read, refactor, reuse and test. – Carsten Aug 06 '14 at 08:45
  • 1
    Or you could write just one additional function, as in oauh's answer. It depends on whether these functions are really separable or not. – Jim Balter Aug 06 '14 at 11:19
  • 1
    @JimBalter : Of course but the "What other approach do you guys know/use?" rather invites alternatives and they cannot all be accepted. This kind of question is not a good fit for SO perhaps. – Clifford Aug 06 '14 at 14:45
  • If we have more than 4 functions, we could even have `bool (*funcs[])(void) = {f1, f2, f3, f4}; bool success = true; for (int i=0; success && i < sizeof funcs/sizeof funcs[0]; i++) success = funcs[i]();`. – glglgl Aug 07 '14 at 08:01
  • @glglgl : My use of simple functions with no parameters was illustrative, they could be any function returning bool or indeed a boolean expression. Your suggestion is probably worth an separate answer, but is far more restrictive, requiring that all the functions have the same signature, and do not need any parameters that cannot be determined from `i`, and that they are indeed all functions rather *any* boolean sub-expression. – Clifford Aug 07 '14 at 09:22
  • @Clifford That's right. Your way is much more flexible, but mine is a good option if we e. g. have a registry of functions which can be registered and unregistered on the fly. – glglgl Aug 07 '14 at 14:16
25

Not clear why you'd need to nest or break. I do this all the time when a sequence needs to bail at first failure:

// process

if (success) {
  // more process
}

if (success) {
  // still more process
}

if (success) {
  // even more process
}
Paul Roub
  • 36,322
  • 27
  • 84
  • 93
  • 1
    The only problem I can see, which is probably very minimal, is that you waste CPU cycles, especially if `success == 0` early. – Fiddling Bits Aug 05 '14 at 18:03
  • 21
    Beginners save nanoseconds. Real programmers save milliseconds. And use a compiler that handles that kind of situation. – gnasher729 Aug 05 '14 at 22:41
  • 6
    "you waste CPU cycles" -- Only if your compiler was written by an incompetent. A decent optimizing compiler will skip tests of the same condition. – Jim Balter Aug 06 '14 at 11:16
  • And on `!success` while single-step debugging you still go through each `if`. Not nice. – Ruslan Aug 06 '14 at 15:33
  • 2
    @JimBalter The compiler can't skip/combine these `if` statements because `success` can change at any time during the process. Only during execution can the state of `success` be known. – Fiddling Bits Aug 06 '14 at 19:20
  • 4
    @FiddlingBits The compiler can skip the if statements as soon as success = false. – Moby Disk Aug 06 '14 at 21:01
  • @MobyDisk Do you mean something other than compiler (e.g. branch predictor)? – Fiddling Bits Aug 06 '14 at 21:04
  • 4
    No, it would be a fairly trivial bit of static analysis for the compiler to pick up that once `success` is `false` for any of those conditions, it will be false for *all*, since we never enter the "process" blocks and change it. – Paul Roub Aug 06 '14 at 21:08
  • I think you taking the example too literal. – Fiddling Bits Aug 06 '14 at 22:14
  • Nope. I run into situations like this all the time, when dealing with things like database operations in C -- open, begin tx, update, insert, end tx, close -- all returning the same success code, all guarded very much like this. – Paul Roub Aug 06 '14 at 22:16
  • 3
    "because success can change at any time during the process" -- once success is false, all the subsequent code falls through. Again, the compiler can skip the next test of `success` if it's known to be false and simply branch to the false destination ... and it can do this repeatedly, so no wasted cycles. "I think you taking the example too literal" -- no, you're simply wrong about wasted CPU cycles. – Jim Balter Aug 06 '14 at 22:20
  • 1
    @FiddlingBits: with optimizations enabled, most compilers will realize they can return as soon as the flag turns false in this case. [Here is an example of what clang produces](https://godbolt.org/g/yqUQem), gcc gives similar output too. Note the jump to the end of the function after each case (`test al, al`, `je .LBB0_4`). – vgru Mar 13 '18 at 14:45
3

Fiddling bits has provided a common approach. Another common approach is to use a single status variable/flag to achieve a similar result.

bool bErr = false;

if (!bErr && success) {
   // do something
} else {
   bErr = true;
}
if (!bErr && success2) {
   // do something
} else {
   bErr = true;
}

if (bErr) {
   // hanlde cleanup from errors
}
Cloud
  • 18,753
  • 15
  • 79
  • 153
  • 1
    You could change the if statements to `if (!(bErr |= !success)) {...}` (or something similar) and forego any need for those verbose else statements (my opinion) – Serge Aug 05 '14 at 18:06
  • @Serge The point of the `else` statements is to set the error flag. The point of this is so that if any single block fails, the error flag is set, and no further blocks are executed. – Cloud Aug 05 '14 at 18:07
  • In my sample, the bErr flag is set to true any time success is false, or stays set to true even if success is true, and forces the if statement to fail. Same idea, different implementation. But yes, I know some people don't like using assignment operators within if-statements. – Serge Aug 05 '14 at 18:18
  • @Serge Yeah, you should really avoid assignments within an evaluation (ie: `if` statement), since it could be "short-circuited out of existence" (http://en.wikipedia.org/wiki/Short-circuit_evaluation), or within a logging or debug statement, since the optimizer or even compile-time settings (ie: `ifdefs`) may prevent the assignment from ever occurring. – Cloud Aug 05 '14 at 18:20
  • 3
    they're not detrimental when people know what they're doing (in the `if` statement scenario). Short circuiting should apply to the `||`, `&&` and `?` operators (according to the source supplied), rather than the `if` statement itself. Still, it is a trade-off of succinctness vs readability. – Serge Aug 05 '14 at 18:31
  • @Serge You make a good point. Even I use such tests in specific cases, ie `if ((NULL == (ptr = malloc(1))) { }`. – Cloud Aug 05 '14 at 18:55
2

Another option would be using a simple flag variable

bool okay = true;

if(okay &= success){
    // process
}

if(okay &= success){
    // process
}

if(okay &= success){
    // process
}
frogatto
  • 28,539
  • 11
  • 83
  • 129
  • 4
    If a single unconditional jump at the end of the original while-loop is annoying, then lots o fsuperfluous conditional jumps are probably more annoying – Hagen von Eitzen Aug 06 '14 at 15:45