Is it bad form to use lots of break(s) in C++?

[Chaotic42]

I'm writing an object constructor which will load a file and set several properties of the object by performing a series of if statements on the file contents to check various equalities. Certain equalities will result in setting a boolean IsValid to false. Once it has been set to false, there is no need to test the other conditions, because clearly something got messed up.

I realize that I could stack this all into some monster if by using a lot of or statements, but that seems like a mess. I was thinking about something like this (and please understand that this is just an pseudocode example, these aren't the actual tests I'd be doing):

while (1){
[INDENT]if (a==4){
[INDENT]IsValid=false;
break;[/INDENT]
}[/INDENT]
[INDENT]if (b==7){
[INDENT]IsValid=false;
break;[/INDENT]
}[/INDENT]
...
}

I've always thought that was bad form, but since constructors don't return anything, I can't use returns. On a similar note, is it bad form to even be doing these kinds of things in the constructor to begin with?

Thanks

Edit: The CODE tag is being weird, sorry about the strange whitespace.

 

[Markbnj]

Breaking out of a loop (or continuing) based on a conditional is very standard in my experience. It's the only elegant means of short-circuiting loop execution when conditions require it.

 

[mv2devnull]

Even if constructors don't return anything, you can still return from them.

6.6.3 The return statement
2 A return statement without an expression can be used only in functions that do not return a value, that is, a function with the return type void, a constructor (12.1), or a destructor (12.4). A return statement with an expression of non-void type can be used only in functions returning a value; the value of the expression is returned to the caller of the function.

Therefore:

Foo::Foo() {
  if ( condA ) {
    ...
    return;
  }
  if ( condB ) {
    ...
    return;
  }
  ...
}

 

[iCyborg]

I've run into many cases like this, and I'm not sure what to think. In a very concrete way, it is really a "goto in disguise", and given the whole "goto is evil" mantra, I try to avoid it. But sometimes avoiding results in a much messier code.
The OP can use return, but sometimes this is not elegant. I've seen a lot of functions in production code that look like this:

void function()
{
	while (true)
	{
            ... some code
            if (errorCondition1)
                break;
            ... more code
            if (errorCondition2)
                break;
            ... more code
            if (errorCondition3)
                break;
            ... etc,
            break; // not really a loop, loop executed only once, just using break to bail out early
        }

    ... do some cleanup
}

This is really using while-break concepts to replace goto. And, as I said, it can be messy to avoid this. If there are more than 2-3 conditions, you will have very deeply nested if's. An alternative is to duplicate cleanup code before returning for every if-test, but if cleanup code is non-trivial it will be ugly to duplicate it every time, and you might forget it too. And using goto generally seems to be no-no.

I try to avoid as much as I can using this while(1) and breaks, but not at all costs.

 

[Merad]

IMO, constructors really shouldn't be used for complex initialization. In your case it sounds like the ctor should put the object into some basic default state, and then have a method which reads settings from a file/stream/whatever. If for some reason the ctor can't setup the object, that should be an exceptional circumstance that warrants throwing an exception.

In a very concrete way, it is really a "goto in disguise", and given the whole "goto is evil" mantra, I try to avoid it.

I disagree. The problem with goto is that execution may jump to any point in the program - you literally have no idea where it will go. With a break it's very clear that control will move to: the end of the containing loop's scope. There's no confusion about what's going to happen (if you know your language, at least).

Ironically, breaking out of nested loops is one of the only places where you'll see a significant number of programmers argue that goto should be used.

 

[dighn]

The arguments against GOTO etc. is usually about readability. If it makes your code more readable, there's no reason to not use it.

 

[iCyborg]

I disagree. The problem with goto is that execution may jump to any point in the program - you literally have no idea where it will go. With a break it's very clear that control will move to: the end of the containing loop's scope. There's no confusion about what's going to happen (if you know your language, at least).

You are here talking generally about breaking out of the loop - I agree that it's a perfectly acceptable way to use it for this, and I'm not saying that every case of breaking out of the loop is a "goto in disguise".
I'm only talking about this particular case. Look at how it's being used here: what you want is 'execute the function and if something goes wrong, go to the end of it, clean up stuff and return'. If you used goto, this would be simple. To avoid using it, we introduce a while loop completely artificially, and its corresponding scope that pretty much wraps up all the code in the function apart from the cleanup. It is completely unnecessary if you use goto. And it is only ever executed exactly once. The only reason is so that you can *go to* the end of the function whenever you want via break instead of goto. I admit that it's still a little better, but it's really exploiting another language idiom just to avoid an "evil goto". I mean, why would you have a while loop if you know that this code will be executed once and once only.

And if you know your language, you know exactly where goto command will go - to the label you specified (I jest, I know this is not what you meant).

 

[Elixer]

The arguments against GOTO etc. is usually about readability. If it makes your code more readable, there's no reason to not use it.

Yeah, I don't see why people get so uptight over using goto. Sure, it can be abused, but, so can most everything else.

 

[Merad]

The arguments against GOTO etc. is usually about readability. If it makes your code more readable, there's no reason to not use it.

Yeah, I don't see why people get so uptight over using goto. Sure, it can be abused, but, so can most everything else.

Use of goto pretty quickly leads to spaghetti code. Trying to maintain spaghetti code leads to suicidal impulses.

P.S. I've don't think I've ever really seen a situation where goto improved code. The nested loop thing is debatable, but it's about the only thing I can think of.

 

[dighn]

Use of goto pretty quickly leads to spaghetti code. Trying to maintain spaghetti code leads to suicidal impulses.

P.S. I've don't think I've ever really seen a situation where goto improved code. The nested loop thing is debatable, but it's about the only thing I can think of.

goto can be useful for error handling in C (where you don't have exceptions) as part of a construct similar to try/finally e.g.

...
if(error) goto cleanup;
...
if(other error) go to cleanup:
...
..

cleanup:
free_resources

the alternative would be heavily nested if statements, which imo don't improve readability

 

[Apathetic]

void function()
{
	while (true)
	{
            ... some code
            if (errorCondition1)
                break;
            ... more code
            if (errorCondition2)
                break;
            ... more code
            if (errorCondition3)
                break;
            ... etc,
            break; // not really a loop, loop executed only once, just using break to bail out early
        }

    ... do some cleanup
}

Having a bunch of break conditions can lead to some un-readable code. If you have to have a bunch of break conditions, throw them all into a seperate function that returns a boolean and then just have a single if/break statement that checks the results.

Dave

 

[Jeraden]

I'd much rather see a lot of breaks/gotos than deeply embedded if statements.

 

[iCyborg]

void function()
{
	while (true)
	{
            ... some code
            if (errorCondition1)
                break;
            ... more code
            if (errorCondition2)
                break;
            ... more code
            if (errorCondition3)
                break;
            ... etc,
            break; // not really a loop, loop executed only once, just using break to bail out early
        }

    ... do some cleanup
}

Having a bunch of break conditions can lead to some un-readable code. If you have to have a bunch of break conditions, throw them all into a seperate function that returns a boolean and then just have a single if/break statement that checks the results.

Dave

I don't understand what you mean. Could you clarify on my example?
I only see 3 alternatives:
1) Deeply nested ifs
2) Artifical while loop and breaks
3) goto like in dighn's post

Neither is elegant, and I tend to go with 2, mainly because others I work with do the same.

 

[Crusty]

I don't understand what you mean. Could you clarify on my example?
I only see 3 alternatives:
1) Deeply nested ifs
2) Artifical while loop and breaks
3) goto like in dighn's post

Neither is elegant, and I tend to go with 2, mainly because others I work with do the same.

I just don't see a single reason to put a while(true) there... ever, what's the point when there's even a break at the end of the code block?

That code should be in it's own function and should use return to short circuit the rest of the logic in the block. You shouldn't need to add a placebo loop to get the control flow you need, that just reeks of bad design.

I guess I might be missing the context of when you need this kind of logic, but short of a few niche cases I just don't see the reason you need to have a while(true) loop ever.

 

[iCyborg]

I just don't see a single reason to put a while(true) there... ever, what's the point when there's even a break at the end of the code block?

It's while (true) - the only way out is with a break.

That code should be in it's own function and should use return to short circuit the rest of the logic in the block. You shouldn't need to add a placebo loop to get the control flow you need, that just reeks of bad design.

Not quite sure what you mean, so can you rewrite the above? And I will probably give you a host of problems with your solution that will most likely "reek with bad design" even more.

I guess I might be missing the context of when you need this kind of logic, but short of a few niche cases I just don't see the reason you need to have a while(true) loop ever.

It's pretty much for error handling only, when you don't want to use exceptions (or can't, like with C, or e.g. the coding standard where I work forbids them), which I've never really liked myself. The most common case I ran into is allocating stuff, checking it's not NULL and continuing. Whenever something is NULL, there's an error, and then you need to delete everything you allocated before and return.

 

[Crusty]

It's while (true) - the only way out is with a break.

Not quite sure what you mean, so can you rewrite the above? And I will probably give you a host of problems with your solution that will most likely "reek with bad design" even more.

It's pretty much for error handling only, when you don't want to use exceptions (or can't, like with C, or e.g. the coding standard where I work forbids them), which I've never really liked myself. The most common case I ran into is allocating stuff, checking it's not NULL and continuing. Whenever something is NULL, there's an error, and then you need to delete everything you allocated before and return.

You're effectively hacking an anonymous method into the language with that construct. The same exact logic and control flow can be achieved with a method and return statements which makes the code arguably more readable and maintainable.

If you are dead set on using a loop you are much better off using a do {...} while(false); instead of while(true) {...break;}. Sure, in their short form like that you can easily see that they are equivalent and the loops will only execute once, but this is psuedocode and on a forum. Throw that construct into a production code and you've got a maintenance nightmare waiting to happen, all it takes is a misplaced brace and/or removing the last break; by accident and you've got an infinite loop. Using either a method or the do/while construct you eliminate that possibility permanently.

 

[iCyborg]

You're effectively hacking an anonymous method into the language with that construct. The same exact logic and control flow can be achieved with a method and return statements which makes the code arguably more readable and maintainable.

Once again, try and rewrite the example I gave, as I don't see how it helps.

If you are dead set on using a loop you are much better off using a do {...} while(false); instead of while(true) {...break;}. Sure, in their short form like that you can easily see that they are equivalent and the loops will only execute once, but this is psuedocode and on a forum. Throw that construct into a production code and you've got a maintenance nightmare waiting to happen, all it takes is a misplaced brace and/or removing the last break; by accident and you've got an infinite loop. Using either a method or the do/while construct you eliminate that possibility permanently.

Actually do {} while(false) is what I mostly see, but OP sait it this way, and it's not that much of a difference. E.g. if the loop body is not small, at least you know up front that you need break to exit.

 

[Crusty]

bool  check_and_run_conditions() {
 if (con1) {
   run_con1();
   return true;
 }
 if (con2) {
   run_con2();
   return true;
 }
 if (con3) {
   run_con3();
   return true;
 }
 //nothing worked
 return false;
}

void method() {
  some code;
  bool status = check_and_run_conditions();
  if (status) {
    everything_worked();
  }
  else {
    nothing_worked();
  }
}

 

[douglasb]

bool  check_and_run_conditions() {
 if (con1) {
   run_con1();
   return true;
 }
 if (con2) {
   run_con2();
   return true;
 }
 if (con3) {
   run_con3();
   return true;
 }
 //nothing worked
 return false;
}

void method() {
  some code;
  bool status = check_and_run_conditions();
  if (status) {
    [b]we_only_know_for_sure_that_at_least_one_thing_worked();[/b]
  }
  else {
    [b]nothing_worked();[/b]
  }
}

Fixed it for you.

I think there are quite a few ways to approach this problem. I like the idea of calling a separate method or function from the constructor, though.

I think this is a better solution below:

bool  check_and_run_conditions() {
 bool result =  con1 && con2 && con3;
 return result;
}

void method() {
  some code;
  bool status = check_and_run_conditions();
  if (status) {
    [b]everything_worked();[/b]
  }
  else {
    [b]at_least_one_thing_didnt_work();[/b]
  }
}

 

[iCyborg]

bool  check_and_run_conditions() {
 if (con1) {
   run_con1();
   return true;
 }
 if (con2) {
   run_con2();
   return true;
 }
 if (con3) {
   run_con3();
   return true;
 }
 //nothing worked
 return false;
}

void method() {
  some code;
  bool status = check_and_run_conditions();
  if (status) {
    everything_worked();
  }
  else {
    nothing_worked();
  }
}

This will not work at all: basically your run_con1() must include if (con2) and if (con3) and all that code, duplicating would be very ugly. You could rewrite it to be if (!con1) return false etc.
But I still find it more messy and less maintainable:
1) You're breaking apart a single purpose function into two functions, the only reason being to avoid goto/break. And creating more code and complexity.
2) You will be also creating a bunch of functions
check_and_run_conditions_for_methodA()
check_and_run_conditions_for_methodB()
check_and_run_conditions_for_methodC()
and it'll be hard to give them any kind of non-generic name.
3) You are allocating stuff in check_and_run_conditions() and deallocating in another method (otherwise you just moved the problem to that function). In a producer-consumer scenario that's OK, but it's always a maintainability risk, I'm not counting constructor/destructor case here.

Anyway, you might like yours better, I'm not saying while(true)/break is a great solution. That *is* the problem: there are shortcomings for pretty much every approach, so it comes to choosing what one finds the "least bad" way. But that's engineering, sometimes solutions aren't academically perfect, but they work...

 

[iCyborg]

Fixed it for you.

I think there are quite a few ways to approach this problem. I like the idea of calling a separate method or function from the constructor, though.

I think this is a better solution below:

No, this is not good, if you could do that, you wouldn't need a new function at all, just a compound if.
The problem is you can't separate code from conditions, there's code0 in the beginning, then codeA that should only be executed if condA is true, then codeB etc.
Say if you have something like

doStuff1();
a = Factory::CreateA();
if (a == NULL) break;
a->init()

You can't just move everything to:

doStuff1();
a = Factory::CreateA();
a->init()

and then check at the end if a is not null, you'll segfault.

 

[Apathetic]

I don't understand what you mean. Could you clarify on my example?
I only see 3 alternatives:
1) Deeply nested ifs
2) Artifical while loop and breaks
3) goto like in dighn's post

Neither is elegant, and I tend to go with 2, mainly because others I work with do the same.

Sorry I wasn't clear, I was meant doing something simple like this:

bool DetectedErrorConditions()
{
   bool results = false;

   if (errorCondition1)
      results = true;
   else if (errorCondition2 && errorCondition3)
      results = true;
   else if (errorCondition4 && (errorCondition5 || errorCondition6))
      results = true;

   return results
}

void SomeOtherFunction
{
...
      if (DetectedErrorConditions())
          break;
...
}

Or something similar. That way, you only have one "if" and one "break".

Dave

 

[mv2devnull]

I only see 3 alternatives:
1) Deeply nested ifs
2) Artifical while loop and breaks
3) goto like in dighn's post

Neither is elegant, and I tend to go with 2, mainly because others I work with do the same.

Shouldn't return and exceptions be on the list?

 

[iCyborg]

Or something similar. That way, you only have one "if" and one "break".

Dave

I think this also assumes code and checks can be neatly separated in which case you don't need extra function. Let me try to come up with something realistic enough, and without breaking my company's NDA:

bool function(int ID)
{
    MyFile fp;
    bool ret = false;

    fp.open("filename");

    do
    {
        int index = fp.findIndex(ID);
        if (index == IndexNotFound)
            break; // the code after may crash or behave unpredictably if index not valid
        
        int x = fp.readX(index);
        if (x<0 || x>100)
            break; // maybe x is percentage, and you want to verify the value is not out of logical bounds

        int y = fp.readY(index);
        if (y == 0)
            break;  // divide by 0 == bad :(

        fp.writeDiv(x/y);
        ret = true;
    } while (false)

    fp.close(); // must always close file (one would do that in destructor, but take it hypothetically)
    return ret;
}

Can you try organizing this into your DetectedErrorConditions() and SomeOtherFunction() with just one if and one break, while preserving exact behaviour?

 

[iCyborg]

Shouldn't return and exceptions be on the list?

Yeah.
returns can be messy since you need to duplicate cleanup code for every break. And I guess I forgot about exceptions, I've never worked on a project that used them, and they never sat well with me. But you're right, you could include those two as well.