235

When the code flow is like this:

if(check())
{
  ...
  ...
  if(check())
  {
    ...
    ...
    if(check())
    {
      ...
      ...
    }
  }
}

I have generally seen this work around to avoid the above messy code flow:

do {
    if(!check()) break;
    ...
    ...
    if(!check()) break;
    ...
    ...
    if(!check()) break;
    ...
    ...
} while(0);

What are some better ways that avoid this work-around/hack so that it becomes a higher-level (industry level) code?

Any suggestions which are out of the box are welcome!

Sankalp
  • 2,796
  • 3
  • 30
  • 43
  • 38
    RAII and throw exceptions. – ta.speot.is Aug 29 '13 at 09:50
  • Did you mean the C++ tag, or C? – doctorlove Aug 29 '13 at 09:51
  • 135
    To me, this sounds like a good time to use `goto` - but I'm sure someone will mark me down for suggesting that, so I'm not writing an answer to that effect. To have a LONG `do ... while(0);` seems like the wrong thing. – Mats Petersson Aug 29 '13 at 09:53
  • 11
    Does using nested `if`s counts as a solution? The `do/while` "cure" is worse than the "illness", because it amounts to using a hidden `goto`. – Sergey Kalinichenko Aug 29 '13 at 09:53
  • 42
    @dasblinkenlight: Yes, indeed. If you are going to use `goto`, be honest about it and do it in the open, don't hide it by using `break` and `do ... while` – Mats Petersson Aug 29 '13 at 09:56
  • 45
    @MatsPetersson: Make it an answer, I will give you +1 for your `goto` rehabilitation effort. :) – wilx Aug 29 '13 at 09:56
  • 4
    Getting to the point of something you consider a "messy code flow" like this may indicate you have lost the plot slightly WRT *cohesion* http://en.wikipedia.org/wiki/Cohesion_(computer_science) -- which is how *spagetti* gets made. – CodeClown42 Aug 29 '13 at 10:00
  • 1
    `goto` http://stackoverflow.com/a/18507873/96780 Step 3 – Daniel Daranas Aug 29 '13 at 10:06
  • 1
    goto is a reasonable way to implement a finite state machine. That may or may not be what Sankalp is trying to do. – john Aug 29 '13 at 10:09
  • 4
    would you consider using `[&](){...}();` and returns instead? – ratchet freak Aug 29 '13 at 11:36
  • @ChristianRau: Indeed, this sort of thing (though hopefully not with the `do` hack) is common in C when you need to clean up local resources before returning from a function. Whether C is "stupid" or "rubbish" for not allowing invisible function calls is a matter of opinion; but doing it in C++ usually would be stupid. – Mike Seymour Aug 29 '13 at 11:50
  • @MikeSeymour *"though hopefully not with the do hack"* - Yet this `do`-hack was exactly the thing I was refering to with my comment, no sentiments against anything else in the question or answers. – Christian Rau Aug 29 '13 at 11:54
  • 3
    @ChristianRau: OK. The hack is stupid in just about language. (I've only encountered it once, working for a team with a "no `goto`" policy. Interestingly, the code was written by the same chap who wrote the policy). – Mike Seymour Aug 29 '13 at 11:56
  • 27
    @ta.speot.is: "RAII and throw exceptions". By doing that, you'll be emulating flow control with exceptions. I.e. it is kinda like using expensive, bleeding edge hardware as a hammer or a paperweight. You can do that, but that definitely looks like a very bad taste for me. – SigTerm Aug 29 '13 at 11:56
  • 4
    Agree with @SigTerm, RAII is great, using exceptions for routine control flow is a bad code smell. – sfjac Aug 29 '13 at 13:26
  • 3
    Using goto is not an improvement in C++ as you cannot jump over object initialization. And we initialize objects in C++ all the time. – Pedro Lamarão Aug 29 '13 at 13:35
  • 5
    I have to admit that I don't have a big problem with do{}while(0). It wouldn't be my first choice but it is a common idiom, especially in C, and it can be the best structural match to the logic of the code. When I see it I immediately understand that the loop will be terminated by code inside the loop and look for the conditions that do it. Unless the code is more complex than this or the blocks get too big, I wouldn't spend much time refactoring it. – Tim Allman Aug 29 '13 at 13:49
  • 2
    @sfjac I agree with you. Using exceptions for routine control flow is a bad code smell. Using do-break-while(0) is not a bad code smell. Using goto is not a bad code smell. Having a zillion nested ifs is not a bad code smell. Having a zillion sequentional if-gotos is not a bad code smell. – R. Martinho Fernandes Aug 29 '13 at 16:48
  • 2
    @MatsPetersson : there's nothing like knowing that giving a correct answer (=solves the problem, compiles, etc) isn't accepted because some people dislike 'goto'... but the amount of upvotes proves that there is a good number of people who understand that 'goto' has it uses. – woliveirajr Aug 29 '13 at 18:25
  • 2
    You're using C++, yet still relying on `0` instead of `false`? :) Even a newly bastardized C variant has `false` now. – Kaz Aug 29 '13 at 19:20
  • 3
    @UpAndAdam judging from the windows that I get to view the industry that from, industry level code is crap code. – R. Martinho Fernandes Aug 29 '13 at 19:41
  • 1
    @UpAndAdam: "WTF is industry level code?" Stuff that'll give any decent programmer nightmares, I guess. In my experience, sometimes under a hood of popular product hides something truly horrible. – SigTerm Aug 29 '13 at 19:56
  • @Kaz: `0` is four characters shorter than `false`. Anyway, didn't you know that `0` evaluated as a boolean *is* `false`? – wallyk Aug 29 '13 at 21:11
  • Just curious — why isn't this a good situation for a `switch-case` construct? Seems like people scoff at those, but I'm not sure why...? – beroe Aug 29 '13 at 21:31
  • 1
    What's wrong with the nested if statements? I like that option better than using break or goto statements. – Ben Miller - Remember Monica Aug 30 '13 at 03:13
  • @BenMiller It gets very complicated and error-prone to maintain when you need to release particular resources after each if statement. – Thomas Aug 30 '13 at 04:05
  • 5
    Wow, the answers here are pretty ugly. The proper advice is "fix your logic so you dont have to do that". or, write a proper FSM. – Matt D Aug 30 '13 at 04:22
  • My colleague defined a "once" macro for that purpose... of course leaving us wonder why some of his member functions are called "Once" while all others start with lowercase letters... – Johannes Schaub - litb Aug 30 '13 at 09:04
  • 1
    Related to: [Indented if/else approach](http://codereview.stackexchange.com/questions/5453/indented-if-else-approach) – Alfredo Osorio Aug 30 '13 at 13:49
  • 1
    *"do-while(0)"* hack put me in mind of a different hack entirely: the one used to make pre-proccessor macros behave like real language statements (i.e. require a `;` to terminate them exactly where a language statement would). – dmckee --- ex-moderator kitten Sep 02 '13 at 03:05
  • See also http://stackoverflow.com/questions/268132/invert-if-statement-to-reduce-nesting/6342924#6342924, which at the time of writing is not listed as related. – Ingo Schalk-Schupp Sep 04 '13 at 06:02

27 Answers27

312

It is considered acceptable practice to isolate these decisions in a function and use returns instead of breaks. While all these checks correspond to the same level of abstraction as of the function, it is quite logical approach.

For example:

void foo(...)
{
   if (!condition)
   {
      return;
   }
   ...
   if (!other condition)
   {
      return;
   }
   ...
   if (!another condition)
   {
      return;
   }
   ... 
   if (!yet another condition)
   {
      return;
   }
   ...
   // Some unconditional stuff       
}
Mikhail
  • 20,685
  • 7
  • 70
  • 146
  • 2
    That assumes that you are actually FINISHED in the funciton, rather hand having another block to do stuff in after the current block of code (e.g. cleanup or final processing of the results found). – Mats Petersson Aug 29 '13 at 09:54
  • 22
    @MatsPetersson: "Isolate in function" means refactoring into a new function which does only the tests. – MSalters Aug 29 '13 at 10:12
  • 36
    +1. This is also a good answer. In C++11, the *isolated function* can be a lambda, as it can also capture the local variables, so makes things easy! – Nawaz Aug 29 '13 at 10:18
  • This is much cleaner than using a `goto`. – Patrick Collins Aug 29 '13 at 15:37
  • 4
    @deworde: Depending on situation this solution might become much longer and less readable than goto. Because C++, unfortunately, does not allow local function defintions, you'll have to move that function (the one you're gonna return from) somewhere else, which reduces readability. It might end up with dozens of parameters, then, because having dozens of parameters is a bad thing, you'll decide to wrap them into struct, which will create new data type. Too much typing for a simple situation. – SigTerm Aug 29 '13 at 15:56
  • 2
    @SigTerm: Longer certainly (goto is very short), but your "dozens of parameters" has already prevented any readability aspirations. In terms of maintenance, goto is going to be worse, as you'd have to check multiple paths to solve any issue, whereas with this, "Input correct? Output wrong? Bug inside func. Drill." Granted, you could clean this up *more*, but as a patch-and-fix, prefer return to goto. – deworde Aug 29 '13 at 16:03
  • @SigTerm Also, if the options are dozens of parameters or goto's buried within code that interacts with dozens of variables, go with the parameters every time, the chances of you skipping an important step wrt to a vital variable are so much lower in the former. – deworde Aug 29 '13 at 16:12
  • 2
    @SigTerm it appears you failed to read Nawaz's comment above yours. – R. Martinho Fernandes Aug 29 '13 at 16:16
  • 4
    I don't think this is necessarily any cleaner or better than `goto` (other than for religious reasons), as it simply disguises `goto` as `return` (with additionally pushing/popping args on stack!) just like disguising it with `break` in the original code. Nevertheless +1 for a very creative way of using/abusing the language in a legitimate way. – Damon Aug 29 '13 at 16:17
  • 25
    @Damon if anything, `return` is cleaner because any reader is immediately aware that it works correctly and what it does. With `goto` you have to look around to see what it is for, and to be sure that no mistakes were made. The disguising *is* the benefit. – R. Martinho Fernandes Aug 29 '13 at 16:29
  • 4
    @Nawaz: While I prefer this over `goto`, wrapping it up in a *long* lambda is worse than `do {} while(0);` in my opinion. Long lambdas are harder to read, as it quickly becomes confusing which function `return` is associated with (the lambda, or the containing function?). – Cornstalks Aug 29 '13 at 16:44
  • 3
    @R.MartinhoFernandes: The dude suggested to cram potentially huge code block into single lambda, that'll be called only by this function. **Just** to avoid goto. Is that supposed to be a better solution? Unless you're using code block multiple times in multiple scenarios, there should be no lambdas. – SigTerm Aug 29 '13 at 19:52
  • 4
    @R.MartinhoFernandes: Anyway, I think that some people forget that the point of functions/lambdas/whatever is not to avoid goto, but to improve readability, prevent bugs, and avoid code duplication. (opinion) Repeating code pattern means there should be a function. If that pattern is only used only within one function, it should be a lambda. However, if pattern does not repeat, or a function has only one caller, it might indicate, that there was no real need to turn this block of code into separate function. In the same way, lambda that gets called only once is pointless. – SigTerm Aug 29 '13 at 20:01
  • I think the current MISRA would prefer gotos rather than a bunch of returns. – Jiminion Aug 29 '13 at 21:27
  • 11
    @SigTerm: Sometimes code should be moved to a separate function just to keep the size of each function down to an easy-to-reason-about size. If you don't want to pay for a function call, mark it `forceinline`. – Ben Voigt Aug 30 '13 at 03:49
  • Just to point out: Although this solution works well for simple functions, if you need to reverse any state change before leaving the function (e.g,. freeing memory, closing handles, modifying data structures, etc), using `return` is **not** the best approach. See this solution for an explanation: http://stackoverflow.com/questions/17804005/how-to-prevent-the-arrowhead-anti-pattern/17815995#17815995 – user1354557 Aug 30 '13 at 15:36
  • 3
    @user1354557 There is RAII for cleanup purposes. – Mikhail Aug 30 '13 at 16:18
  • In a situation like this, I would refrain from omitting the last `return` statement. That eliminates the possibility of an error caused by reordering the statements and forgetting to introduce a `return` into what used to be the last conditional block. Leaving it in doesn't reduce clarity, either. – ashastral Sep 03 '13 at 21:11
  • Guidelines for critical software discourage the use of multiple return statements within a function because it makes the analysis (static analysis, model checking, automatic proofs (Coq, Why) etc.) of the code harder. – klearn Sep 03 '13 at 23:10
  • Using lambda is better if you can – Amos Nov 09 '18 at 08:59
262

There are times when using goto is actually the RIGHT answer - at least to those who are not brought up in the religious belief that "goto can never be the answer, no matter what the question is" - and this is one of those cases.

This code is using the hack of do { ... } while(0); for the sole purpose of dressing up a goto as a break. If you are going to use goto, then be open about it. It's no point in making the code HARDER to read.

A particular situation is just when you have a lot of code with quite complex conditions:

void func()
{
   setup of lots of stuff
   ...
   if (condition)
   {
      ... 
      ...
      if (!other condition)
      {
          ...
          if (another condition)
          {
              ... 
              if (yet another condition)
              {
                  ...
                  if (...)
                     ... 
              }
          }
      }
  .... 

  }
  finish up. 
}

It can actually make it CLEARER that the code is correct by not having such a complex logic. 

void func()
{
   setup of lots of stuff
   ...
   if (!condition)
   {
      goto finish;
   }
   ... 
   ...
   if (other condition)
   {
      goto finish;
   }
   ...
   if (!another condition)
   {
      goto finish;
   }
   ... 
   if (!yet another condition)
   {
      goto finish;
   }
   ... 
   .... 
   if (...)
         ...    // No need to use goto here. 
 finish:
   finish up. 
}

Edit: To clarify, I'm by no means proposing the use of goto as a general solution. But there are cases where goto is a better solution than other solutions.

Imagine for example that we are collecting some data, and the different conditions being tested for are some sort of "this is the end of the data being collected" - which depends on some sort of "continue/end" markers that vary depending on where you are in the data stream.

Now, when we're done, we need to save the data to a file.

And yes, there are often other solutions that can provide a reasonable solution, but not always.

Mats Petersson
  • 126,704
  • 14
  • 140
  • 227
  • 77
    Disagree. `goto` may have a place, but `goto cleanup` does not. Cleanup is done with RAII. – MSalters Aug 29 '13 at 10:13
  • I would suggest `finalize` (or `done`) label instead of `cleanup` as it belongs to only RAII. – Nawaz Aug 29 '13 at 10:14
  • 15
    @MSalters: That assumes that the cleanup involves something that can be solved with RAII. Maybe I should have said "issue error" or some such instead. – Mats Petersson Aug 29 '13 at 10:15
  • 25
    So, keep getting downvotes on this one, presumably from those of the religious belief of `goto` is never the right answer. I'd appreciate if there was a comment... – Mats Petersson Aug 29 '13 at 11:00
  • `goto` may have a place, but `goto finish` is not it. You should consider RAII instead (with either `return` or `throw` statements in place of `goto` in your example). – utnapistim Aug 29 '13 at 11:07
  • 2
    @utnapistim If the finishing code isn't doing any cleanup, one could argue that a destructor isn't the right thing to use. Of course you can use RAII for anything, but if there isn't a "resource", there isn't a point in "acquiring" or "releasing" it. And exceptions are to be used for, well, exceptional cases. While I probably would have used nested ifs or a separate function/lambda or mabye even a destructor, this doesn't change anything in the validity of the use of `goto` here. – Christian Rau Aug 29 '13 at 11:28
  • 19
    people hate `goto` because you have to think to use it / to understand a program that uses it... Microprocessors, on the other hand, are built on `jumps` and `conditional jumps`... so the problem is with some people, not with logic or something else. – woliveirajr Aug 29 '13 at 11:34
  • 6
    @woliveirajr: That's why I think programmers should start in assembly language (even 8086) and tehn move onto higher-level concepts. – SigTerm Aug 29 '13 at 11:51
  • @ChristianRau, I didn't make that distinction (cleanup vs. finish) because the Op's question was explicitly about nested ifs and error codes. In this case ("finishing code" not being used for cleanup), I would prefer returning before the finishing code if it's not necessary and falling through it otherwise (i.e. no goto or label necessary). – utnapistim Aug 29 '13 at 11:59
  • 5
    @woliveirajr Let alone function calls, conditional blocks, threading, process scheduling or interrupts, all of which are done in the hardware, OS or language! The problem with `goto` isn't the jump, it is that it facilitates writing unclear code because you can (in theory) jump to anywhere from anywhere causing any mayhem anyone can think of anytime (OK, I think I'm done with "any" now `:)`). Used with good judgment, it can easily *improve* readability over at least some alternate techniques, and what professional programmer wants code to be less readable than it could potentially be? – user Aug 29 '13 at 12:01
  • 19
    +1 for appropriate use of `goto`. RAII is *not* the correct solution if errors are expected (not exceptional), as that would be an abuse of exceptions. – Joe Aug 29 '13 at 12:08
  • 2
    @MichaelKjörling `goto` can not jump anywhere in C++ (or even C) – Joe Aug 29 '13 at 12:13
  • 1
    @Joe Precisely, which is why I said *in theory*. Back [in the 1960s](http://www.u.arizona.edu/~rubinson/copyright_violations/Go_To_Considered_Harmful.html), it commonly could. Maybe I should have made it more clear, but I ran out of space in the comment box and it isn't an answer in itself to the question as asked. – user Aug 29 '13 at 12:16
  • 1
    +1 The issue is not goto but structure. This is a structured use of it and eliminates a lot of ugly work-arounds. Definitely use RAII to manage resources in the local scope. – gregg Aug 29 '13 at 12:45
  • 14
    I have to admit that my use of `goto` tends to be in C code, not C++ code. In which case, RAII doesn't really work. I have worked on quite a bit of driver code, and it tends to be written in C, not so much because it couldn't be written just as well in C++, but because C++ isn't allowed in most driver runtime environments (e.g. Linux and until recently at least Windows). So even when the driver is written for an OS that supports C++ in drivers, it may not use C++ to allow it to be portable. – Mats Petersson Aug 29 '13 at 12:50
  • 1
    See [this question](http://stackoverflow.com/questions/11306799/why-does-c-enforce-such-behavior-in-crosses-initialization) for one reason this goto solution is worse than the do-while solution. – Pedro Lamarão Aug 29 '13 at 13:33
  • 6
    This is also a situation where blindly putting `{ ... }` around every single-statement block makes the code harder to read. – zwol Aug 29 '13 at 13:46
  • 1
    `goto` is unnecessary. See my answer below. – Dan Bechard Aug 29 '13 at 14:07
  • 1
    Just out of interest, what is the correct answer in Python for this sort of example? – Tomcat Aug 29 '13 at 14:16
  • 1
    @Snipergirl I know the equivalent of the original `do..while..break` is `while True: ...code... break`, but Python likes to use exceptions for control flow, so I'm not sure "fixing" it like this applies – Izkata Aug 29 '13 at 14:25
  • @Izkata makes sense. I've only just started with Python, and I'd just been doing the whole if (condition): break business, so will read more about using exceptions! Cheers. – Tomcat Aug 29 '13 at 14:33
  • @Snipergirl Nono, I'm not saying you _should_ use exceptions, I'm saying that I think `while True: ... break` would be clearest in Python, if you can't turn it into a separate function like in Dan's answer – Izkata Aug 29 '13 at 14:53
  • There are situations when using `goto` is necessary... but this is most probably not one of them. – einpoklum Aug 29 '13 at 15:08
  • 1
    @Snipergirl In python you'd much sooner split this up in separate little functions that each hold their own condition-checking. Actually you should do that in C++ also, but legacy projects do often contain huge constructs that are not easy to refactor and it's not always feasible in time-critical code like drivers. – KillianDS Aug 29 '13 at 15:30
  • 1
    -1: There are better ways to do this than goto, and use of goto in "the right place" leads to use of goto in the wrong place. There are necessary places to use goto, especially in legacy or tightly efficient code. From the parts between the dash, this is using goto to prove a point, and thus not a good place to use a goto. – deworde Aug 29 '13 at 15:48
  • 5
    @deworde: You know, if there are better methods, you should post them as an answer. Without goto, do/while is the best solution. RAII/exceptions are not the best solution, because they emulate flow control. – SigTerm Aug 29 '13 at 15:51
  • I have just read the wikipedia page about RAII and I still don't understand what's wrong with goto cleanup.. Is it just a naming issue? Someone cares to explain please? – BlackBear Aug 29 '13 at 15:52
  • 4
    @BlackBear: Nothing, really. RAII simply guarantees that objects will be cleaned up even if exception is thrown (destructors are called during stack unwind), and goto shouldn't jump over object initialization, so you'll end up using simple types or pointers. Goto is an acceptable technique, but it is more C-like, which is why some people dislike it. – SigTerm Aug 29 '13 at 16:00
  • 3
    I always think goto is a sympton rather than a cause, rather like global state/singletons. It flags code that needs rethinking, though sometimes this rethinking is never done, then the goto becomes a fungus that forces use of less maintainable code elsewhere. That's why people get religious about it - the door needs to be shut quickly or it stays open. – Benedict Aug 29 '13 at 16:05
  • @SigTerm [This has nothing to do with C/C++](http://www.u.arizona.edu/~rubinson/copyright_violations/Go_To_Considered_Harmful.html) (Compare date to date of creation for C). This is to do with how program flow can be made harder to maintain by use of a raw flow control pointer rather than a higher-level abstraction that serves the same purpose. – deworde Aug 29 '13 at 16:18
  • 4
    `// No need to use goto here.` Do it nonetheless. You never know if you might add another `if` statement. – Reactormonk Aug 29 '13 at 16:33
  • 5
    I voted this down because I largely prefer the return solution given by Mikhail. With return you see faster if the implementation is done correctly. And, important if working in a team, this is probably the more common sense solution that saves discussions. I see this as a C solution, not a C++ one as argued by utnapistim. – SebastianK Aug 29 '13 at 16:53
  • @SigTerm: Or you could also start with old-fashioned line-numbered BASIC, whose control-flow commands are barely removed from assembly language. – dan04 Aug 29 '13 at 16:54
  • 5
    I like this answer. It basically treats `goto` like a lightweight exception. I'm not sure this is the right answer for C++, but it's an excellent answer for C. – Steven Fisher Aug 29 '13 at 17:32
  • 5
    @SebastianK : don't know if voting down should be used because someone likes or dislikes something... "while voting down a post signals the opposite: that the post contains wrong information, is poorly researched, or fails to communicate information" – woliveirajr Aug 29 '13 at 18:22
  • 4
    @SebastianK I agree with woliveirajr, save the downvotes for the answers that are actually *incorrect*. You may or may not agree with using `goto`, but it does answer the question IMO ("what are other/better ways to do this?"). Of course, the "better" part of the question is inherently subjective. (I commonly find myself upvoting several different answers who answer a question using different techniques or from different angles; no need to downvote those where I wouldn't personally use the technique, as long as they aren't *wrong*.) – user Aug 29 '13 at 18:47
  • @Zack I totally agree with you: using braceless `if` statements would make this code so much easier to read. – AJMansfield Aug 29 '13 at 19:29
  • @Roy, that doesn't work very well if, for example, you have to save the data you have collected in the main part of the function into a file. Yes, we can perhaps split it into separate functions, but I've been in situations where that is clearly not the right solution either. – Mats Petersson Aug 29 '13 at 22:58
  • 3
    It's ok if we're speaking about theory. But the truth is that many people won't allow `goto`s, just because they are `goto`. Call it "instruction racism", if you want, but that's how it is. – Giulio Franco Aug 29 '13 at 22:58
  • 1
    I like @GiulioFranco's phrasing, "instruction racism." I bet this code generates precisely the same machine code as the `do{}while(0)` in the question, and yet it is derided simply for containing a `goto`. How much should programmers who really understand proper use of `goto` avoid it simply to prevent the less understanding from seeing it? – Kevin Aug 29 '13 at 23:31
  • @MatsPetersson Yes, I agree with you. I was saying that more for the benefit of those who don't – Kevin Aug 29 '13 at 23:48
  • 1
    @Joe: If the conditions aren't exceptional, then it would indeed be abuse to use exceptions. So don't. Instead, use RAII and early return. Destructors are a universal mechanism for automatically running code at the end of a scope, not limited to exception stack unwind. – Ben Voigt Aug 30 '13 at 03:52
  • 3
    I'm not going to downvote, but -- I think the `do { ... } while(false);` approach is better than the `goto`-based approach, because then you know that all your variables are in the scopes you expect, there's no risk you're accidentally bypassing a variable's initialization, and so on. – ruakh Aug 30 '13 at 03:55
  • 1
    Yeah `goto` doesn't really make it any clearer or more readable. `goto` is a hack. `return`s would be a much better solution in this case - but I personally don't have a problem with a bunch of `if` statements. – Oleksiy Aug 30 '13 at 04:24
  • 2
    @GiulioFranco : I asked a question (my unique question on stackoverflow, so far, about GOTO, and it got closed in less than one day... http://stackoverflow.com/questions/18518632/why-and-when-should-a-goto-be-used-in-c-c ... closed for using the G-word, I'd say :) – woliveirajr Aug 30 '13 at 16:24
  • I did not downvote, but in my own answer I give an example why this use of `goto` often does not work for C++. – Nemo Sep 05 '13 at 02:58
  • What is "break" but a goto with a label of "}"? What is "continue" but a "goto" that branches to the middle latter half of a for(;;) statement? Goto, in C++, has two problems: 1. ability to skip around across scopes, when scopes are the lifeblood of the language; 2. excessively primitive eliminating intent. 'goto', 'for (;;)', 'do while' - look how hard we're trying to find a way to solve a LANGUAGE level problem, and RAII only solves mundane cases where "sweep it under the rug" is good enough. – kfsone Sep 05 '13 at 06:53
  • My problem with goto is it can mess object instantiation after the goto "if (x) goto cleanup; classname obj;" will cause some compilers to emit a warning that obj's constructor might not be called. For some reason the do { break; } while(0); version doesn't cause that warning. – jmucchiello Sep 05 '13 at 20:29
82

You can use a simple continuation pattern with a bool variable:

bool goOn;
if ((goOn = check0())) {
    ...
}
if (goOn && (goOn = check1())) {
    ...
}
if (goOn && (goOn = check2())) {
    ...
}
if (goOn && (goOn = check3())) {
    ...
}

This chain of execution will stop as soon as checkN returns a false. No further check...() calls would be performed due to short-circuiting of the && operator. Moreover, optimizing compilers are smart enough to recognize that setting goOn to false is a one-way street, and insert the missing goto end for you. As the result, the performance of the code above would be identical to that of a do/while(0), only without a painful blow to its readability.

Sergey Kalinichenko
  • 714,442
  • 84
  • 1,110
  • 1,523
  • 30
    Assignments inside `if` conditions look _very_ suspicious. – Mikhail Aug 29 '13 at 10:56
  • 91
    @Mikhail Only to an untrained eye. – Sergey Kalinichenko Aug 29 '13 at 11:03
  • 20
    I've used this technique before and it always kind of bugged me that I figured the compiler would have to generate code to check at each `if` no matter what `goOn` was even if an early one failed (as opposed to jumping/breaking out)... but I just ran a test and VS2012 at least was smart enough to short-circuit everything after the first false anyway. I'll be using this more often. *note:* if you use `goOn &= checkN()` then `checkN()` will always run even if `goOn` was `false` at the start of the `if` (i.e., don't do that). – mark Aug 29 '13 at 12:15
  • 1
    An untrained *mind* might come and change all `=` into `==`, thinking `=` is a typo. :P (+1 anyway). – Nawaz Aug 29 '13 at 15:24
  • 1
    @mark this is the difference between ```aBool && func()``` (short-circuits) and ```aBool & func()``` (bitwise operator runs both). I can't think of any language that doesn't support this. – Deebster Aug 29 '13 at 15:30
  • 11
    @Nawaz: If you have an untrained mind making arbitrary changes all over a code base, you have a much bigger problem than just assignments inside `if`s. – Idelic Aug 29 '13 at 16:58
  • 1
    @Nawaz I'm sure he meant third-person "you" (or whatever it is called), i.e. no one in particular. You can read the sentence as "If one has an untrained mind making arbitrary changes all over a code base, one has a much bigger problem than just assignments inside `if`s." – R. Martinho Fernandes Aug 29 '13 at 17:06
  • 3
    By using this continuation pattern, EVERY if will always be hit. Depending on the number of ifs this may not be desirable especially if this is in some loop and the processor is guessing wrong with some ifs. I would be in favor of just using a goto to stop checking every if. Which turns into Mats answer – sisharp Aug 29 '13 at 20:50
  • 1
    Whenever I run across this construct, I assume the person who wrote it was not very experienced. They didn't know about `do` ... `while(0)` which I consider far more elegant. – wallyk Aug 29 '13 at 21:14
  • 6
    @sisharp Elegance is so in the eye of the beholder! I fail to understand how in the world a misuse of a looping construct could somehow be perceived as "elegant", but perhaps that's just me. – Sergey Kalinichenko Aug 29 '13 at 21:27
  • 3
    @sisharp That's plainly incorrect: modern optimizers will bypass every `if` once `goOn` is set to `false` (look at [mark's comment above](http://stackoverflow.com/questions/18507518/what-are-some-better-ways-to-avoid-the-do-while0-hack-in-c/18508168?noredirect=1#comment27217332_18508168)). – Sergey Kalinichenko Aug 29 '13 at 21:29
  • 5
    @wallyk: Whenever i see a `do` ... `while(0)` construct I assume the person who wrote it has an unfounded phobia of `goto`s. If you are going to use `goto`, please call it as such instead of dressing it up as a loop. – Bjarke Freund-Hansen Aug 30 '13 at 07:50
  • 1
    @sisharp why is it that so often when people worry about performance they assume a world where no optimisations take place? – R. Martinho Fernandes Aug 30 '13 at 10:30
  • 2
    @dasblinkenlight "Only to an untrained eye" No, possibly to compilers too. Your `if (goOn = check0())` may trigger a warning (something like "assignment in condition" or "did you mean '=='?"), needing to wrap the assignment into parentheses (`if ((goOn = check0()))`) to shut if off. – gx_ Aug 31 '13 at 08:37
  • @dasblinkenlight the problem in larger development groups is, that the untrained eye or the quick bug fixer will destroy the pattern and put another if between them without the `goOn` part. – Alexander Oh Sep 01 '13 at 10:57
  • 1
    @Alex The risk of a "quick bug fixer" ruining it remains the same in all approaches: that would be the same person who forgets to add a `break` in a `do`/`while(0)` pattern. – Sergey Kalinichenko Sep 01 '13 at 11:29
  • @dasblinkenlight I personally think it's least likely in the extract to function and use return version. but I agree. – Alexander Oh Sep 01 '13 at 15:46
  • 1
    I dislike this idiom. Why clutter the reader's brain with an extra variable `goOn`, even if the compiler is smart enough to insert the jump that's elided. – Andrew Lazarus Sep 03 '13 at 19:35
  • @AndrewLazarus There's nothing idiomatic about this approach: there's a Boolean variable with a name that matches its semantic, and a sequence of conditional statements with assignments of that variable - that's it! Stuff like that is known to anybody who read K&R, which is another way of saying "anyone who actively programs in C" :-) – Sergey Kalinichenko Sep 03 '13 at 19:43
  • @dasblinkenlight, well, 'paradigm' instead of 'idiom' if you prefer. And in the early K&R days, optimizers weren't smart enough to insert the jump, either. – Andrew Lazarus Sep 03 '13 at 19:58
38

  1. Try to extract the code into a separate function (or perhaps more than one). Then return from the function if the check fails.

  2. If it's too tightly coupled with the surrounding code to do that, and you can't find a way to reduce the coupling, look at the code after this block. Presumably, it cleans up some resources used by the function. Try to manage these resources using an RAII object; then replace each dodgy break with return (or throw, if that's more appropriate) and let the object's destructor clean up for you.

  3. If the program flow is (necessarily) so squiggly that you really need a goto, then use that rather than giving it a weird disguise.

  4. If you have coding rules that blindly forbid goto, and you really can't simplify the program flow, then you'll probably have to disguise it with your do hack.

Mike Seymour
  • 249,747
  • 28
  • 448
  • 644
  • 4
    I humbly submit that RAII, while useful, is not a silver bullet. When you find yourself about to write a convert-goto-to-RAII class that has no other use, I definitely think you'd be better served just using the "goto-end-of-the-world" idiom people mentioned already. – idoby Aug 29 '13 at 11:55
  • 1
    @busy_wait: Indeed, RAII can't solve everything; that's why my answer doesn't stop with the second point, but goes on to suggest `goto` if that's really a better option. – Mike Seymour Aug 29 '13 at 11:58
  • 3
    I agree, but I think it's a bad idea to write goto-RAII conversion classes and I think it should be stated explicitly. – idoby Aug 29 '13 at 12:07
  • @idoby what about writing *one* RAII template class, and instantiating it with whatever single use cleanup you need in a lambda – Caleth Feb 14 '20 at 12:30
  • @Caleth sounds to me like you're reinventing ctors/dtors? – idoby Feb 14 '20 at 12:34
  • @idoby I'm not reinventing them. Its *using* a destructor as a place to run code. Which is the *essence* of RAII – Caleth Feb 14 '20 at 12:36
37

TLDR: RAII, transactional code (only set results or return stuff when it is already computed) and exceptions.

Long answer:

In C, the best practice for this kind of code is to add an EXIT/CLEANUP/other label in the code, where cleanup of local resources happens and an error code (if any) is returned. This is best practice because it splits code naturally into initialization, computation, commit and return:

error_code_type c_to_refactor(result_type *r)
{
    error_code_type result = error_ok; //error_code_type/error_ok defd. elsewhere
    some_resource r1, r2; // , ...;
    if(error_ok != (result = computation1(&r1))) // Allocates local resources
        goto cleanup;
    if(error_ok != (result = computation2(&r2))) // Allocates local resources
        goto cleanup;
    // ...

    // Commit code: all operations succeeded
    *r = computed_value_n;
cleanup:
    free_resource1(r1);
    free_resource2(r2);
    return result;
}

In C, in most codebases, the if(error_ok != ... and goto code is usually hidden behind some convenience macros (RET(computation_result), ENSURE_SUCCESS(computation_result, return_code), etc.).

C++ offers extra tools over C:

  • The cleanup block functionality can be implemented as RAII, meaning you no longer need the entire cleanup block and enabling client code to add early return statements.

  • You throw whenever you cannot continue, transforming all the if(error_ok != ... into straight-forward calls.

Equivalent C++ code:

result_type cpp_code()
{
    raii_resource1 r1 = computation1();
    raii_resource2 r2 = computation2();
    // ...
    return computed_value_n;
}

This is best practice because:

  • It is explicit (that is, while error handling is not explicit, the main flow of the algorithm is)

  • It is straightforward to write client code

  • It is minimal

  • It is simple

  • It has no repetitive code constructs

  • It uses no macros

  • It doesn't use weird do { ... } while(0) constructs

  • It is reusable with minimal effort (that is, if I want to copy the call to computation2(); to a different function, I don't have to make sure I add a do { ... } while(0) in the new code, nor #define a goto wrapper macro and a cleanup label, nor anything else).

Peter Mortensen
  • 30,738
  • 21
  • 105
  • 131
utnapistim
  • 26,809
  • 3
  • 46
  • 82
  • +1. This is what I try to use, using RAII or something. `shared_ptr` with a custom deleter can do a lot of stuff. Even easier with lambdas in C++11. – Macke Aug 29 '13 at 10:57
  • True, but if you end up using shared_ptr for things that are not pointers, consider at least typedef-ing it: `namespace xyz { typedef shared_ptr shared_handle; shared_handle make_shared_handle(a, b, c); };` In this case (with `make_handle` setting the correct deleter type on construction), the name of the type no longer suggests it is a pointer. – utnapistim Aug 29 '13 at 11:10
22

I'm adding an answer for the sake of completeness. A number of other answers pointed out that the large condition block could be split out into a separate function. But as was also pointed out a number of times is that this approach separates the conditional code from the original context. This is one reason that lambdas were added to the language in C++11. Using lambdas was suggested by others but no explicit sample was provided. I've put one in this answer. What strikes me is that it feels very similar to the do { } while(0) approach in many ways - and maybe that means it's still a goto in disguise....

earlier operations
...
[&]()->void {

    if (!check()) return;
    ...
    ...
    if (!check()) return;
    ...
    ...
    if (!check()) return;
    ...
    ...
}();
later operations
Peter R
  • 2,865
  • 18
  • 19
19

Certainly not the answer, but an answer (for the sake of completeness)

Instead of :

do {
    if(!check()) break;
    ...
    ...
    if(!check()) break;
    ...
    ...
    if(!check()) break;
    ...
    ...
} while(0);

You could write:

switch (0) {
case 0:
    if(!check()) break;
    ...
    ...
    if(!check()) break;
    ...
    ...
    if(!check()) break;
    ...
    ...
}

This is still a goto in disguise, but at least it's not a loop any more. Which means you won't have to very carefully check there is not some continue hidden somewhere in the block.

The construct is also simple enough that you can hope the compiler will optimize it away.

As suggested by @jamesdlin, you can even hide that behind a macro like

#define BLOC switch(0) case 0:

And use it like

BLOC {
    if(!check()) break;
    ...
    ...
    if(!check()) break;
    ...
    ...
    if(!check()) break;
    ...
    ...
}

This is possible because the C language syntax expect a statement after a switch, not a bracketed block and you can put a case label before that statement. Until now I didn't see the point of allowing that, but in this particular case it is handy to hide the switch behind a nice macro.

kriss
  • 23,497
  • 17
  • 97
  • 116
  • 2
    Ooh, clever. You could even hide it behind a macro like `define BLOCK switch (0) case 0:` and use it like `BLOCK { ... break; }`. – jamesdlin Aug 30 '13 at 19:28
  • @jamesdlin: it never occurred to me before that it could be useful tu put a case before the opening bracket of a switch. But it is indeed allowed by C and in this case it is handy to write a nice macro. – kriss Sep 04 '13 at 07:00
15

I would recommend an approach similar to Mats answer minus the unnecessary goto. Only put the conditional logic in the function. Any code that always runs should go before or after the function is invoked in the caller:

void main()
{
    //do stuff always
    func();
    //do other stuff always
}

void func()
{
    if (!condition)
        return;
    ...
    if (!other condition)
        return;
    ...
    if (!another condition)
        return;
    ... 
    if (!yet another condition)
        return;
    ...
}
Dan Bechard
  • 5,104
  • 3
  • 34
  • 51
  • 3
    If you have to acquire another resource in the middle of `func`, you need to factor-out another function (per your pattern). If all these isolated functions need the same data, you'll just end up copying the same stack arguments over and over, or decide to allocate your arguments on the heap and pass a pointer around, not leveraging the most basic of the language's feature (function arguments). In short, I don't believe this solution scales for the worst case where every condition is checked after acquiring new resources that must be cleaned up. Note Nawaz comment about lambdas however. – tne Aug 29 '13 at 14:36
  • 2
    I don't remember the OP saying anything about acquiring resources in the middle his code block, but I accept that as a feasible requirement. In that case what is wrong with declaring the resource on the stack anywhere in `func()` and allowing its destructor to handle freeing resources? If anything outside of `func()` needs access to the same resource it should be declared on the heap prior to calling `func()` by a proper resource manager. – Dan Bechard Aug 29 '13 at 14:48
12

The code flow itself is already a code smell that to much is happening in the function. If there is not a direct solution to that (the function is a general check function), then using RAII so you can return instead of jumping to the an end-section of the function might be better.

stefaanv
  • 14,072
  • 2
  • 31
  • 53
11

If you don't need to introduce local variables during the execution then you can often flatten this:

if (check()) {
  doStuff();
}  
if (stillOk()) {
  doMoreStuff();
}
if (amIStillReallyOk()) {
  doEvenMore();
}

// edit 
doThingsAtEndAndReportErrorStatus()
the_mandrill
  • 29,792
  • 6
  • 64
  • 93
  • 2
    But then each condition would have to include the previous one, which is not only ugly but potentially bad for performance. Better to jump over those checks and cleanup immediately as soon as we know we're "not OK". – tne Aug 29 '13 at 14:29
  • That's true, however this approach does have advantages if there are things that have to be done at the end so you don't want to return early (see edit). If you combine with Denise's approach of using bools in the conditions, then the performance hit will be negligible unless this is in a very tight loop. – the_mandrill Aug 30 '13 at 08:28
10

Similar to dasblinkenlight's answer, but avoids the assignment inside the if that could get "fixed" by a code reviewer:

bool goOn = check0();
if (goOn) {
    ...
    goOn = check1();
}
if (goOn) {
    ...
    goOn = check2();
}
if (goOn) {
    ...
}

...

I use this pattern when the results of a step need to be checked before the next step, which differs from a situation where all the checks could be done up front with a big if( check1() && check2()... type pattern.

Denise Skidmore
  • 2,286
  • 22
  • 51
  • Note that check1() could really be PerformStep1() which returns a result code of the step. This would keep down the complexity of your process flow function. – Denise Skidmore Aug 29 '13 at 17:33
10

Use exceptions. Your code will look much more cleaner (and exceptions were created exactly for handling errors in the execution flow of a program). For cleaning up resources (file descriptors, database connections, etc,), read the article Why doesn't C++ provide a "finally" construct?.

#include <iostream>
#include <stdexcept>   // For exception, runtime_error, out_of_range

int main () {
    try {
        if (!condition)
            throw std::runtime_error("nope.");
        ...
        if (!other condition)
            throw std::runtime_error("nope again.");
        ...
        if (!another condition)
            throw std::runtime_error("told you.");
        ...
        if (!yet another condition)
            throw std::runtime_error("OK, just forget it...");
    }
    catch (std::runtime_error &e) {
        std::cout << e.what() << std::endl;
    }
    catch (...) {
        std::cout << "Caught an unknown exception\n";
    }
    return 0;
}
Peter Mortensen
  • 30,738
  • 21
  • 105
  • 131
Cartucho
  • 3,257
  • 2
  • 30
  • 55
  • 11
    Really? First, I honestly don't see any readability improvement, there. DO NOT USE EXCEPTIONS TO CONTROL PROGRAM FLOW. That IS NOT their proper purpose. Also, exceptions bring significant performance penalties. The proper use case for an exception is when some condition is present THAT YOU CAN NOT DO ANYTHING ABOUT, such as trying to open a file that is already exclusively locked by another process, or a network connection fails, or a call to a database fails, or a caller passes an invalid parameter to a procedure. That sort of thing. But DO NOT use exceptions to control program flow. – Craig Tullis Aug 30 '13 at 19:51
  • 3
    I mean, not to be a snot about it, but go to that Stroustrup article you referenced and look for the "What shouldn't I use exceptions for?" section. Among other things, he says: _"In particular, throw is not simply an alternative way of returning a value from a function (similar to return). Doing so will be slow and will confuse most C++ programmers used to seing exceptions used only for error handling. Similarly, throw is not a good way of getting out of a loop."_ – Craig Tullis Aug 30 '13 at 19:59
  • 3
    @Craig All that you have pointed is right, but you are assuming that the sample program can continue after a `check()` condition fail, and that's certantly YOUR assumption, there is no context in the sample. Assuming that the program cannot continue, using exceptions is the way to go. – Cartucho Aug 30 '13 at 20:34
  • 2
    Well, true enough if that is actually the context. But, funny thing about assumptions... ;-) – Craig Tullis Aug 30 '13 at 20:36
10

For me do{...}while(0) is fine. If you don't want to see the do{...}while(0), you can define alternative keywords for them.

Example:

SomeUtilities.hpp:

#define BEGIN_TEST do{
#define END_TEST }while(0);

SomeSourceFile.cpp:

BEGIN_TEST
   if(!condition1) break;
   if(!condition2) break;
   if(!condition3) break;
   if(!condition4) break;
   if(!condition5) break;
   
   //processing code here

END_TEST

I think the compiler will remove the unneccessary while(0) condition in do{...}while(0) in binary version and convert the breaks into unconditional jump. You can check it's assembly language version to be sure.

Using goto also produces cleaner code and it is straightforward with the condition-then-jump logic. You can do the following:

{
   if(!condition1) goto end_blahblah;
   if(!condition2) goto end_blahblah;
   if(!condition3) goto end_blahblah;
   if(!condition4) goto end_blahblah;
   if(!condition5) goto end_blahblah;
   
   //processing code here

 }end_blah_blah:;  //use appropriate label here to describe...
                   //  ...the whole code inside the block.

Note the label is placed after the closing }. This is the avoid one possible problem in goto that is accidentally placing a code in between because you didn't see the label. It is now like do{...}while(0) without condition code.

To make this code cleaner and more comprehensible, you can do this:

SomeUtilities.hpp:

#define BEGIN_TEST {
#define END_TEST(_test_label_) }_test_label_:;
#define FAILED(_test_label_) goto _test_label_

SomeSourceFile.cpp:

BEGIN_TEST
   if(!condition1) FAILED(NormalizeData);
   if(!condition2) FAILED(NormalizeData);
   if(!condition3) FAILED(NormalizeData);
   if(!condition4) FAILED(NormalizeData);
   if(!condition5) FAILED(NormalizeData);

END_TEST(NormalizeData)

With this, you can do nested blocks and specify where you want to exit/jump-out.

BEGIN_TEST
   if(!condition1) FAILED(NormalizeData);
   if(!condition2) FAILED(NormalizeData);

   BEGIN_TEST
      if(!conditionAA) FAILED(DecryptBlah);
      if(!conditionBB) FAILED(NormalizeData);   //Jump out to the outmost block
      if(!conditionCC) FAILED(DecryptBlah);
  
      // --We can now decrypt and do other stuffs.

   END_TEST(DecryptBlah)

   if(!condition3) FAILED(NormalizeData);
   if(!condition4) FAILED(NormalizeData);

   // --other code here

   BEGIN_TEST
      if(!conditionA) FAILED(TrimSpaces);
      if(!conditionB) FAILED(TrimSpaces);
      if(!conditionC) FAILED(NormalizeData);   //Jump out to the outmost block
      if(!conditionD) FAILED(TrimSpaces);

      // --We can now trim completely or do other stuffs.

   END_TEST(TrimSpaces)

   // --Other code here...

   if(!condition5) FAILED(NormalizeData);

   //Ok, we got here. We can now process what we need to process.

END_TEST(NormalizeData)

Spaghetti code is not the fault of goto, it's the fault of the programmer. You can still produce spaghetti code without using goto.

acegs
  • 2,621
  • 1
  • 22
  • 31
  • 10
    I'd choose `goto` over extending the language syntax using the preprocessor a million times over. – Christian Aug 30 '13 at 11:48
  • 2
    _"To make this code cleaner and more comprehensible, you can [use LOADS_OF_WEIRD_MACROS]"_: does not compute. – underscore_d May 12 '18 at 13:38
  • take note on the first sentence that the normal `do{...}while(0) ` is preferred. this answer is a suggestion only and a play on macro/goto/label for breaking out on specific blocks. – acegs Jun 29 '20 at 09:02
8

This is a well-known and well-solved problem from a functional programming perspective - the maybe monad.

In response to the comment I received below I have edited my introduction here: You can find full details about implementing C++ monads in various places which will allow you to achieve what Rotsor suggests. It takes a while to grok monads so instead I'm going to suggest here a quick "poor-mans" monad-like mechanism for which you need know about nothing more than boost::optional.

Set up your computation steps as follows:

boost::optional<EnabledContext> enabled(boost::optional<Context> context);
boost::optional<EnergisedContext> energised(boost::optional<EnabledContext> context);

Each computational step can obviously do something like return boost::none if the optional it was given is empty. So for example:

struct Context { std::string coordinates_filename; /* ... */ };

struct EnabledContext { int x; int y; int z; /* ... */ };

boost::optional<EnabledContext> enabled(boost::optional<Context> c) {
   if (!c) return boost::none; // this line becomes implicit if going the whole hog with monads
   if (!exists((*c).coordinates_filename)) return boost::none; // return none when any error is encountered.
   EnabledContext ec;
   std::ifstream file_in((*c).coordinates_filename.c_str());
   file_in >> ec.x >> ec.y >> ec.z;
   return boost::optional<EnabledContext>(ec); // All ok. Return non-empty value.
}

Then chain them together:

Context context("planet_surface.txt", ...); // Close over all needed bits and pieces

boost::optional<EnergisedContext> result(energised(enabled(context)));
if (result) { // A single level "if" statement
    // do work on *result
} else {
    // error
}

The nice thing about this is that you can write clearly defined unit tests for each computational step. Also the invocation reads like plain English (as is usually the case with functional style).

If you don't care about immutability and it is more convenient to return the same object each time you could come up with some variation using shared_ptr or the like.

Peter Mortensen
  • 30,738
  • 21
  • 105
  • 131
Benedict
  • 2,771
  • 20
  • 21
  • 3
    This code has an undesired property of forcing each of the individual functions to handle failure of the previous function, thus not properly utilising the Monad idiom (where monadic effects, in this case failure, are supposed to be handled implicitly). To do that you need to have `optional enabled(Context); optional energised(EnabledContext);` instead and use the monadic composition operation ('bind') rather than function application. – Rotsor Aug 30 '13 at 00:42
  • Thanks. You are correct - that is the way to do it properly. I didn't want to write too much in my answer to explain that (hence the term "poor-mans" which was meant to suggest I wasn't going the whole hog here). – Benedict Aug 30 '13 at 08:21
7

How about moving the if statements into an extra function yielding a numerical or enum result?

int ConditionCode (void) {
   if (condition1)
      return 1;
   if (condition2)
      return 2;
   ...
   return 0;
}


void MyFunc (void) {
   switch (ConditionCode ()) {
      case 1:
         ...
         break;

      case 2:
         ...
         break;

      ...

      default:
         ...
         break;
   }
}
Razzupaltuff
  • 2,250
  • 2
  • 21
  • 37
  • This is nice when possible, but much less general than the question asked here. Every condition could depends on the code executed after the last debranching test. – kriss Aug 29 '13 at 22:43
  • The problem here is you separate cause and consequence. I.e. you separate code that refers to the same condition number and this can be a source of further bugs. – Riga Aug 30 '13 at 09:34
  • @kriss: Well, the ConditionCode() function could be adjusted to take care of this. They key point of that function is that you can use return for a clean exit as soon as a final condition has been computed; And that is what is providing structural clarity here. – Razzupaltuff Aug 30 '13 at 17:16
  • @Riga: Imo these are completely academic objections. I find C++ to become more complex, cryptic and unreadable with every new version. I cannot see a problem with a small helper function evaluating complex conditions in a well structured way to make the target function right below more readable. – Razzupaltuff Aug 30 '13 at 17:19
  • @Riga: I am pointing that this is not exactly an answer to the OP question. (ie: answering to "How can I do this ?" by "You should do something else instead"). This does not mean you are not right. – kriss Sep 04 '13 at 08:28
  • 1
    @karx11erx my objections are practical and based on my experience. This pattern is bad unrelatedly to C++11 or whatever language. If you have difficulties with language constructs that allow you to write good architecture that is not a language problem. – Riga Sep 12 '13 at 19:19
5

I am not particularly into the way using break or return in such a case. Given that normally when we are facing such a situation, it is usually a comparatively long method.

If we are having multiple exit points, it may cause difficulties when we want to know what will cause certain logic to be executed: Normally we just keep on going up blocks enclosing that piece of logic, and the criteria of those enclosing block tell us the situation:

For example,

if (conditionA) {
    ....
    if (conditionB) {
        ....
        if (conditionC) {
            myLogic();
        }
    }
}

By looking at enclosing blocks, it is easy to find out that myLogic() only happen when conditionA and conditionB and conditionC is true.

It becomes a lot less visible when there are early returns:

if (conditionA) {
    ....
    if (!conditionB) {
        return;
    }
    if (!conditionD) {
        return;
    }
    if (conditionC) {
        myLogic();
    }
}

We can no longer navigate up from myLogic(), looking at enclosing block to figure out the condition.

There are different workarounds that I used. Here is one of them:

if (conditionA) {
    isA = true;
    ....
}

if (isA && conditionB) {
    isB = true;
    ...
}

if (isB && conditionC) {
    isC = true;
    myLogic();
}

(Of course it is welcomed to use the same variable to replace all isA isB isC.)

Such an approach will at least give the reader of code, that myLogic() is executed when isB && conditionC. The reader is given a hint that he need to further lookup what will cause isB to be true.

Peter Mortensen
  • 30,738
  • 21
  • 105
  • 131
Adrian Shum
  • 38,812
  • 10
  • 83
  • 131
5

Something like this perhaps

#define EVER ;;

for(EVER)
{
    if(!check()) break;
}

or use exceptions

try
{
    for(;;)
        if(!check()) throw 1;
}
catch()
{
}

Using exceptions you can also pass data.

liftarn
  • 429
  • 3
  • 21
  • 10
    Please don't do clever things like your definition of ever, they usually make code harder to read for fellow developers. I've seen somebody defining Case as break;case in a header file, and used it in a switch in a cpp file, making others wonder for hours why the switch breaks between Case statements. Grrr... – Michael Aug 30 '13 at 18:35
  • 5
    And when you name macros, you should make them look like macros (i.e., in all uppercase). Otherwise someone who happens to name a variable/function/type/etc. named `ever` will be very unhappy... – jamesdlin Aug 30 '13 at 19:18
3
typedef bool (*Checker)();

Checker * checkers[]={
 &checker0,&checker1,.....，&checkerN,NULL
};

bool checker1(){
  if(condition){
    .....
    .....
    return true;
  }
  return false;
}

bool checker2(){
  if(condition){
    .....
    .....
    return true;
  }
  return false;
}

......

void doCheck(){
  Checker ** checker = checkers;
  while( *checker && (*checker)())
    checker++;
}

How about that?

sniperbat
  • 231
  • 1
  • 6
2

I'm not a C++ programmer, so I won't write any code here, but so far nobody has mentioned an object oriented solution. So here is my guess on that:

Have a generic interface that provides a method to evaluate a single condition. Now you can use a list of implementations of those conditions in your object containing the method in question. You iterate over the list and evaluate each condition, possibly breaking out early if one fails.

The good thing is that such design sticks very well to the open/closed principle, because you can easily add new conditions during initialization of the object containing the method in question. You can even add a second method to the interface with the method for condition evaluation returning a description of the condition. This can be used for self-documenting systems.

The downside, however, is that there is slightly more overhead involved because of the usage of more objects and the iteration over the list.

Peter Mortensen
  • 30,738
  • 21
  • 105
  • 131
SpaceTrucker
  • 13,377
  • 6
  • 60
  • 99
  • Could you add an example in a different language? I think this question applies to many languages although it was asked specifically about C++. – Denise Skidmore Sep 23 '13 at 14:15
2

Another pattern useful if you need different cleanup steps depending on where the failure is:

    private ResultCode DoEverything()
    {
        ResultCode processResult = ResultCode.FAILURE;
        if (DoStep1() != ResultCode.SUCCESSFUL)
        {
            Step1FailureCleanup();
        }
        else if (DoStep2() != ResultCode.SUCCESSFUL)
        {
            Step2FailureCleanup();
            processResult = ResultCode.SPECIFIC_FAILURE;
        }
        else if (DoStep3() != ResultCode.SUCCESSFUL)
        {
            Step3FailureCleanup();
        }
        ...
        else
        {
            processResult = ResultCode.SUCCESSFUL;
        }
        return processResult;
    }
Denise Skidmore
  • 2,286
  • 22
  • 51
1

First, a short example to show why goto is not a good solution for C++:

struct Bar {
    Bar();
};

extern bool check();

void foo()
{
    if (!check())
       goto out;

    Bar x;

    out:
}

Try to compile this into an object file and see what happens. Then try the equivalent do+ break + while(0).

That was an aside. The main point follows.

Those little chunks of code often require some kind of cleanup should the whole function fail. Those cleanups usually want to happen in the opposite order from the chunks themselves, as you "unwind" the partially-finished computation.

One option to obtain these semantics is RAII; see @utnapistim's answer. C++ guarantees that automatic destructors run in the opposite order to constructors, which naturally supplies an "unwinding".

But that requires lots of RAII classes. Sometimes a simpler option is just to use the stack:

bool calc1()
{
    if (!check())
        return false;

    // ... Do stuff1 here ...

    if (!calc2()) {
        // ... Undo stuff1 here ...
        return false;
    }

    return true;
}

bool calc2()
{
    if (!check())
        return false;

    // ... Do stuff2 here ...

    if (!calc3()) {
        // ... Undo stuff2 here ...
        return false;
    }

    return true;
}

...and so on. This is easy to audit, since it puts the "undo" code next to the "do" code. Easy auditing is good. It also makes the control flow very clear. It is a useful pattern for C, too.

It can require the calc functions to take lots of arguments, but that is usually not a problem if your classes/structs have good cohesion. (That is, stuff that belongs together lives in a single object, so these functions can take pointers or references to a small number of objects and still do lots of useful work.)

Peter Mortensen
  • 30,738
  • 21
  • 105
  • 131
Nemo
  • 70,042
  • 10
  • 116
  • 153
  • Very easy to audit the cleanup path, but maybe not so easy to trace the golden path. But overall, I think something like this does encourage a consistent cleanup pattern. – Denise Skidmore Sep 23 '13 at 14:13
1

This is the way I do it.

void func() {
  if (!check()) return;
  ...
  ...

  if (!check()) return;
  ...
  ...

  if (!check()) return;
  ...
  ...
}
Vadoff
  • 9,219
  • 6
  • 43
  • 39
0

If you code has a long block of if..else if..else statements, you may try and rewrite the entire block with the help of Functors or function pointers. It may not be the right solution always but quite often is.

http://www.cprogramming.com/tutorial/functors-function-objects-in-c++.html

HAL
  • 3,888
  • 3
  • 19
  • 28
  • In principle this is possible (and not bad), but with explicit function objects or -pointers it just disrupts the code flow far too strongly. OTOH the, here equivalent, use of lambdas or ordinary named functions is good practise, efficient and reads well. – leftaroundabout Sep 02 '13 at 10:43
0

Consolidate it into one if statement:

if(
    condition
    && other_condition
    && another_condition
    && yet_another_condition
    && ...
) {
        if (final_cond){
            //Do stuff
        } else {
            //Do other stuff
        }
}

This is the pattern used in languages such as Java where the goto keyword was removed.

Peter Mortensen
  • 30,738
  • 21
  • 105
  • 131
Tyzoid
  • 1,072
  • 13
  • 31
  • 2
    That only works if you don't have to do any stuff between the condition-tests. (well, I suppose you could hide the doing-of-stuff inside some function calls made by the condition-tests, but that might get a bit obfuscated if you did it too much) – Jeremy Friesner Aug 29 '13 at 15:55
  • @JeremyFriesner Actually, you could actually do the in-between-stuff as separate boolean functions that always evaluate as `true`. Short circuit evaluation would guarantee that you never run in-between stuff for which all the prerequisite tests did not pass. – AJMansfield Aug 29 '13 at 19:34
  • @AJMansfield yes, that's what I was referring to in my second sentence... but I'm not sure doing that would be an improvement in code quality. – Jeremy Friesner Aug 29 '13 at 19:42
  • @JeremyFriesner Nothing stops you from writing the conditions as `(/*do other stuff*/, /*next condition*/)`, you could even format it nicely. Just don't expect people to like it. But honestly, this only goes to show that it was a mistake for Java to phase out that `goto` statement... – cmaster - reinstate monica Aug 29 '13 at 19:53
  • @JeremyFriesner I was assuming that these were booleans. If functions were to be run inside of each condition, than there is a better way to handle it. – Tyzoid Aug 29 '13 at 20:17
0

I am amazed by the number of different answers being presented here. But, finally in the code which I have to change (i.e. remove this do-while(0) hack or anything), I did something different from any of the answers being mentioned here and I am confused why no one thought this. Here's what I did:

Initial code:

do {

    if(!check()) break;
    ...
    ...
    if(!check()) break;
    ...
    ...
    if(!check()) break;
    ...
    ...
} while(0);

finishingUpStuff.

Now:

finish(params)
{
  ...
  ...
}

if(!check()){
    finish(params);    
    return;
}
...
...
if(!check()){
    finish(params);    
    return;
}
...
...
if(!check()){
    finish(params);    
    return;
}
...
...

So, what has been done here is that the finishing up stuff has been isolated in a function and things have suddenly become so simple and clean!

I thought this solution was worth mentioning, so provided it here.

Sankalp
  • 2,796
  • 3
  • 30
  • 43
0

If using the same error handler for all errors, and each step returns a bool indicating success:

if(
    DoSomething() &&
    DoSomethingElse() &&
    DoAThirdThing() )
{
    // do good condition action
}
else
{
    // handle error
}

(Similar to tyzoid's answer, but conditions are the actions, and the && prevents additional actions from occurring after the first failure.)

Denise Skidmore
  • 2,286
  • 22
  • 51
0

Why wasn't flagging method answered it is used since ages.

//you can use something like this (pseudocode)
long var = 0;
if(condition)  flag a bit in var
if(condition)  flag another bit in var
if(condition)  flag another bit in var
............
if(var == certain number) {
Do the required task
}
Fennekin
  • 216
  • 3
  • 12