Use of goto for cleanly exiting a loop

Question

I have a question about use of the goto statement in C++. I understand that this topic is controversial, and am not interested in any sweeping advice or arguments (I usually stray from using goto). Rather, I have a specific situation and want to understand whether my solution, which makes use of the goto statement, is a good one or not. I would not call myself new to C++, but would not classify myself as a professional-level programmer either. The part of the code which has generated my question spins in an infinite loop once started. The general flow of the thread in pseudocode is as follows:

void ControlLoop::main_loop()
{
    InitializeAndCheckHardware(pHardware) //pHardware is a pointer given from outside
    //The main loop
    while (m_bIsRunning)
    {
        simulated_time += time_increment; //this will probably be += 0.001 seconds
        ReadSensorData();
        if (data_is_bad) {
            m_bIsRunning = false;
            goto loop_end;
        }    
        ApplyFilterToData();
        ComputeControllerOutput();
        SendOutputToHardware();
        ProcessPendingEvents();

        while ( GetWallClockTime() < simulated_time ) {}
        if ( end_condition_is_satisified ) m_bIsRunning = false;
    }
    loop_end:
    DeInitializeHardware(pHardware);
}

The pHardware pointer is passed in from outside the ControlLoop object and has a polymorphic type, so it doesn't make much sense for me to make use of RAII and to create and destruct the hardware interface itself inside main_loop. I suppose I could have pHardware create a temporary object representing a sort of "session" or "use" of the hardware which could be automatically cleaned up at exit of main_loop, but I'm not sure whether that idea would make it clearer to somebody else what my intent is. There will only ever be three ways out of the loop: the first is if bad data is read from the external hardware; the second is if ProcessPendingEvents() indicates a user-initiated abort, which simply causes m_bIsRunning to become false; and the last is if the end-condition is satisfied at the bottom of the loop. I should maybe also note that main_loop could be started and finished multiple times over the life of the ControlLoop object, so it should exit cleanly with m_bIsRunning = false afterwards.

Also, I realize that I could use the break keyword here, but most of these pseudocode function calls inside main_loop are not really encapsulated as functions, simply because they would need to either have many arguments or they would all need access to member variables. Both of these cases would be more confusing, in my opinion, than simply leaving main_loop as a longer function, and because of the length of the big while loop, a statement like goto loop_end seems to read clearer to me.

Now for the question: Would this solution make you uncomfortable if you were to write it in your own code? It does feel a little wrong to me, but then I've never made use of the goto statement before in C++ code -- hence my request for help from experts. Are there any other basic ideas which I am missing that would make this code clearer?

Thanks.

Answer 1

Avoiding the use of goto is a pretty solid thing to do in object oriented development in general.

In your case, why not just use break to exit the loop?

while (true)
{
    if (condition_is_met)
    {
        // cleanup
        break;
    }
}

As for your question: your use of goto would make me uncomfortable. The only reason that break is less readable is your admittance to not being a strong C++ developer. To any seasoned developer of a C-like language, break will both read better, as well as provide a cleaner solution than goto.

In particular, I simply do not agree that

if (something)
{
    goto loop_end;
}

is more readable than

if (something)
{
    break;
}

which literally says the same thing with built-in syntax.

Answer 2

With your one, singular condition which causes the loop to break early I would simply use a break. No need for a goto that's what break is for.

However, if any of those function calls can throw an exception or if you end up needing multiple breaks I would prefer an RAII style container, this is the exact sort of thing destructors are for. You always perform the call to DeInitializeHardware, so...

// todo: add error checking if needed
class HardwareWrapper {
public:
    HardwareWrapper(Hardware *pH) 
      : _pHardware(pH) { 
        InitializeAndCheckHardware(_pHardware);
    }

    ~HardwareWrapper() {
        DeInitializeHardware(_pHardware);
    }

    const Hardware *getHardware() const {
        return _pHardware;
    }

    const Hardware *operator->() const {
        return _pHardware;
    }

    const Hardware& operator*() const {
        return *_pHardware;
    }

private:
    Hardware *_pHardware;
    // if you don't want to allow copies...
    HardwareWrapper(const HardwareWrapper &other);
    HardwareWrapper& operator=(const HardwareWrapper &other);
}

// ...

void ControlLoop::main_loop()
{
    HardwareWrapper hw(pHardware);
    // code
}

Now, no matter what happens, you will always call DeInitializeHardware when that function returns.