How to rewrite this without the use of goto

Question

I'm new to C++. I have the following form of code. I was wondering how I could rewrite this without the use of goto. If I use break in place of it, then I still check the if statement following the loop on every pass through this code even though if I did break I know this condition will fail, so why check it? The only solution I was able to come up with is to make this code a function and replace the goto with a return and remove the if conditional following the loop, leaving just the statements. Would this be a place where goto IS acceptable?

Edit: I should also mention the reason I leave the loop when we find an equal member, is because I don't need to check the remainder of the members in my object cause I already know the one we found is unique to the objects we are iterating, therefore there will never be a match if we continued through the rest of the iterators. So I just exit the loop then.

while (begIt != endIt)
    if ((*begIt).member == someObject.member){
        // Do these statements
        goto someLabel; // then goto someLabel
    }
    ++begIt;
}
if (begIt == endIt){ // We must have not found an equal member
    // So do these statements
}
someLabel: // ...

Answer 1

Use algorithms and lambdas.

auto it = std::find_if(begin, end, [&](const A& a) {
    return a.member == other.member;
});

if(it != end) {
    // found
}
else {
    // not found
}

Answer 2

while (true)
    if (begIt == endIt){ // We must have not found an equal member
        // So do these statements
        break;
    }
    if ((*begIt).member == someObject.member){
        // Do these statements
        break;
    }
    ++begIt;
}

// someLabel: ...

The only solution I would accept in my code. (or seperate function. directly executing a lambda for this is ugly...)

But it is very likely that you are worrying about the wrong things. Inserting a goto will probably not increase performance because the thing that you are intending to "optimize away" is a single conditional (pointer == pointer) which is negligible in 99.9999999% of all programs out there. So even if you are writing high-performance numerical code I would advice you to simple break and check with an if after the loop (I dare you to show me some profiler output to prove me wrong ;) )

Answer 3

This looks like a perfectly reasonable use of goto to me.

There's no spaghetti logic in your code, and don't let the naysayers brainwash you into their lies.

Your alternative is to move the loop and the ensuing conditional into their own function, and return from that function. You could use a lambda to keep all the logic inline:

[&]() {
    while (begIt != endIt)
        if ((*begIt).member == someObject.member){
            // Do these statements
            return;
        }
        ++begIt;
    }
    if (begIt == endIt){ // We must have not found an equal member
        // So do these statements
    }
}();

// ...

You can likely shorten your loop logic using C++'s standard algorithms, anyway.

Answer 4

See Steve McConnell's article for a discussion of ways to refactor gotos. I agree you have one of a few cases that (without C stdlib idioms) are harder to refactor than most intro programming textbooks would suggest, but it's not that hard to refactor and still better to do so.

I agree the above solution that better uses the standard library is best. A standard pattern that works in all languages (that is pure structured programming), is to create a bool foundMatch = false, set it when you find it, and check in your code between the while loop and the goto's label.

Answer 5

AFAIK the idea of using goto has been dead for a while apart from maybe in Windows batch scripts and certainly not in C++ where we have all kinds of flow control magic. This is done primarily to improve code readability and avoid ending up with spaghetti code which is code with so many gotos and labels flying around that it's utter hell to mke head nor tail of, let alone maintain. If you want to jump out of the loop I the break command is what you want. Like so:

bool not_broken;
not_broken = true;
while (begIt != endIt)
    if ((*begIt).member == someObject.member){
        // Do these statements
        not_broken=false;
        break; // quit out of while loop
    }
    ++begIt;
}
if (not_broken && begIt == endIt){ // We must have not found an equal member
    // So do these statements
}
// code continues...

I wouldn't be overly concerned about knowing that begIt wont equal endIt but if this really bothers you then you could set a bool just before you break and use the logical and operator && to check whether or not the while loop was broken. It is important that the bool is first in the if as this will result in the begIt and endIt not being compared when the bool is false.