CODEWITHSUNDEEP
c++refactoringgoto
Ron
Ron

Reputation: 147

Can you rewrite this snippet without goto

Guys, I have the following code that is inside a big while loop that iterates over a tree. This is as fast as I can get this routine but I have to use a goto. I am not fundamentally against goto but if I can avoid them I would like to. (I am not trying to start a flame war, please.)

The constraints:

  1. The current=current->child() is expensive (it's a shared_ptr) so I'd like to minimize the use of that operation at all cost.
  2. After the operation current should be the last child it found.
  3. cnt must count each child it encounters.
  4. cnt++ will be replaced by some other operation (or several operations) and should only appear once :)

the code:

insideloopy:
cnt++;
if ( current->hasChild() )
{
    current = current->child();
    goto insideloopy;
}

Edit: Sorry guys, originally forgot to mention cnt++ should only appear once. It will be some kind of operation on the node, and should thus only be there one time. I'm also trying to avoid making that another function call.

Upvotes: 4

Views: 718

Answers (8)

Greg Miller
Greg Miller

Reputation: 479

No break statements:

notDone=true;
while(notDone){
   cnt++;
   if ( current->hasChild() ){
       current = current->child();
   } else {
       notDone=false;
   }
}

Upvotes: 0

Thorarin
Thorarin

Reputation: 48476

Original answer

Assuming this is C or C++:

while (cnt++, current->hasChild())
{
    current = current->child();
}

I'm not a big fan of the comma operator usually, but I don't like repeating myself either :)

Updated 'fun' answer

After learning that cnt++ is actually some multiline operation, this particular syntax would be less than ideal. Something more along the lines of your accepted answer would be better.

If you want to be really funky, this would also work:

do 
{
    cnt++;
} while (current->hasChild() && (current = current->child()));

Now I feel really dirty though, with my abusing the short circuiting on the && operator :)

Sane answer

Exercises in compactness aside and striving for readable code, I'm forced to conclude that one of the existing answers is best suited (I'm just including this for completeness' sake):

while (true)
{
   cnt++;
   if (!current->hasChild()) break;
   current = current->child();
}

The while (true) will be optimized by the compiler into a regular infinite loop, so there is only one conditional statement (if you care about that).

The only thing going against this solution is if your node operation was a long piece of code. I don't mind infinite loops so much, as long as I can see where they terminate at a glance. Then again, if it were really long, it should be a function anyway.

Upvotes: 26

AProgrammer
AProgrammer

Reputation: 52284

I'd investigate the possibility of making current->child() return NULL when it has no child if it doesn't already -- that seems the best possible result and leaving it undefined in this case seems error prone -- and then use:

for (; current; current = current->child())
{
    cnt++;
}

Upvotes: 0

nos
nos

Reputation: 229098

for(cnt++ ; current->hasChild() ; cnt++) {
   current = current->child();
}

Upvotes: 0

Guffa
Guffa

Reputation: 700322

You can use break to get out of the loop in the middle of the code:

while (true) {
   cnt++;
   if (!current->hasChild()) break;
   current = current->child();
}

Upvotes: 3

Stefano Borini
Stefano Borini

Reputation: 143795

insideloopy:
cnt++;
if ( current->hasChild() )
{
    current = current->child();
    goto insideloopy;
}

I love infinite loops.

while (true) {
   cnt++;
   if (!current->hasChild()) break;
   current = current->child();
}

Of course you can do it in many other ways (see other answers). do while, put the check in the while, etc. In my solution, I wanted to map nearly to what you are doing (an infinite goto, unless break)

Upvotes: 11

Achim
Achim

Reputation: 15702

while (current->hasChild())
{
    cnt++;
    current = current->child();
}

Or am I missing something?

Upvotes: 0

LorenVS
LorenVS

Reputation: 12857

cnt++;
while(current->hasChild())
{
   cnt++;
   current = current->child();
}

EDIT:

If you only want cnt++ to be in your code once:

while(true)
{
    cnt++;
    if(current->hasChild())
       current = current->child();
    else
       break;
}

Upvotes: 18

Related Questions