Do you consider this technique "BAD"?

Question

Sometimes you need to skip execution of part of a method under certain non-critical error conditions. You can use exceptions for that, but exceptions generally are not recommended in normal application logic, only for abnormal situations.

So I do a trick like this:

do {    bool isGood = true;     .... some code     if(!isGood)        break;     .... some more code     if(!isGood)        break;     .... some more code   } while(false);   ..... some other code, which has to be executed. 

I use a "fake" loop which will run once, and I can abort it by break or continue.

Some of my colleagues did not like that, and they called it "bad practice". I personally find that approach pretty slick. But what do you think?

Answer 1

Bad practice, it depends.

What I see in this code is a very creative way to write "goto" with less sulphur-smelling keywords.

There are multiple alternatives to this code, which can or can not be better, depending on the situation.

Your do/while solution

Your solution is interesting if you have a lot of code, but will evaluate the "exit" of this processing at some limited points:

do {    bool isError = false ;     /* some code, perhaps setting isError to true */    if(isError) break ;    /* some code, perhaps setting isError to true */    if(isError) break ;    /* some code, perhaps setting isError to true */ } while(false) ;     // some other code    

The problem is that you can't easily use your "if(isError) break ;" is a loop, because it will only exit the inner loop, not your do/while block.

And of course, if the failure is inside another function, the function must return some kind of error code, and your code must not forget to interpret the error code correctly.

I won't discuss alternatives using ifs or even nested ifs because, after some thinking, I find them inferior solutions than your own for your problem.

Calling a goto a... goto

Perhaps you should put clearly on the table the fact you're using a goto, and document the reasons you choose this solution over another.

At least, it will show something could be wrong with the code, and prompt reviewers to validate or invalidate your solution.

You must still open a block, and instead of breaking, use a goto.

{    // etc.    if(/*some failure condition*/) goto MY_EXIT ;    // etc.     while(/* etc.*/)    {       // etc.       for(/* etc.*/)       {          // etc.          if(/*some failure condition*/) goto MY_EXIT ;          // etc.       }       // etc.       if(/*some failure condition*/) goto MY_EXIT ;       // etc.    }     // etc. }  MY_EXIT:     // some other code    

This way, as you exit the block through the goto, there is no way for you to bypass some object constructor with the goto (which is forbidden by C++).

This problem solves the process exiting from nested loops problem (and using goto to exit nested loops is an example given by B. Stroustrup as a valid use of goto), but it won't solve the fact some functions calls could fail and be ignored (because someone failed to test correctly their return code, if any).

Of course, now, you can exit your process from multiple points, from multiple loop nesting depth, so if it is a problem...

try/catch

If the code is not supposed to fail (so, failure is exceptional), or even if the code structure can fail, but is overly complex to exit, then the following approach could be clearer:

try {    // All your code    // You can throw the moment something fails    // Note that you can call functions, use reccursion,    // have multiple loops, etc. it won't change    // anything: If you want to exit the process,    // then throw a MyExitProcessException exception.     if(/* etc. */)    {       // etc.       while(/* etc.*/)       {          // etc.          for(/* etc.*/)          {             // etc.             if(/*some failure condition*/) throw MyExitProcessException() ;             // etc.          }          // etc.           callSomeFunction() ;          // the function will throw if the condition is met          // so no need to test a return code           // etc.       }       // etc.    }     // etc. } catch(const MyExitProcessException & e) {    // To avoid catching other exceptions, you should    // define a "MyExitProcessException" exception }  // some other code 

If some condition in the code above, or inside some functions called by the code above, is not met, then throw an exception.

This is somewhat weightier than your do/while solution, but has the same advantages, and can even abort the processing from inside loops or from inside called functions.

Discussion

Your need seems to come from the fact you can have a complex process to execute (code, functions calls, loops, etc.), but you want to interrupt it over some condition (probably either failure, or because it succeeded sooner than excepted). If you can rewrite it in a different way, you should do it. But perhaps, there is no other way.

Let's assume that.

If you can code it with a try/catch, do it: To interrupt a complex piece of code, throwing an exception is the right solution (the fact you can add failure/success info inside your exception object should not be underestimated). You will have a clearer code after that.

Now, if you're in a speed bottleneck, resolving your problem with thrown exceptions as an exit is not the fastest way to do it.

No one can deny your solution is a glorified goto. There won't be a goto-spaghetti code, because the do/while won't let you do that, but it is still a semantic goto. This can be the reasons some could find this code "bad": They smell the goto without finding its keyword clearly.

In this case (and in this performance, profiled-verified) case only, your solution seems Ok, and better than the alternative using if), but of lesser quality (IMHO) than the goto solution which at least, doesn't hide itself behind a false loop.

Conclusion

As far as I am concerned, I find your solution creative, but I would stick to the thrown exception solution.

So, in order of preference:

1. Use try/catch
2. Use goto
3. Use your do/while loop
4. Use ifs/nested ifs