Using goto as a error handling strategy

Question

I had a code review recently and a debate began. Much of my code looks like this:

for (i = 1; i <= 3; i++)
        {
            DoubleValue = tODBCX->getDouble(KeyFieldCount + i, IsNULL, IsSuccess);
            if (IsNULL)
            {
                LoggerDrillHole::LogToDB(LOG_ERROR, L"Survey depth, dip and azimuth values can not be NULL.", __FUNCTIONW__);
                IsSuccess = false;
                goto EXIT;
            }
            else
            {
                if (i == 1)
                    Depth = DoubleValue;
                else if(i == 2)
                    DipDegrees = DoubleValue;
                else if (i == 3)
                    AzimuthDegrees = DoubleValue;
            }
        }

The contentious goto statement sparked debate. This code is contained within a function that begins life by initializing a local boolean variable IsSuccess = true and the function finally returns IsSuccess. The EXIT strategy looks after essential tidy up code;

EXIT:

    tODBCX->Close();
    if (Key != 0) free(Key);
    Key = 0;
    if (PriorKey != 0) free(PriorKey);
    PriorKey = 0;
    return IsSuccess;

There were several such goto EXIT statements adjacent to setting IsSuccess = false and logging to database and so on. The comment was made that this is disruptive to the flow of the code. One should instead use a do loop (infinite loop) and break out of that loop and then process all of the required instead of using the goto statement.

I intensely disliked the infinite loop strategy, but I could get used to it if it truly improves the readability. Is there a better way?

Try to never use any `goto`'s, especially if it's on an important project. — bitcell, Nov 28 '14 at 13:36
This is primarily opinion based. You should try asking at http://codereview.stackexchange.com/ — 2501, Nov 28 '14 at 13:36
yep, wrap up the ending code in a function [is possible] and call it. mostly, that's what we do to avoid `goto`. — Sourav Ghosh, Nov 28 '14 at 13:37
Wrap up the ending code in a function makes sense, but that will not eliminate the goto which prevents further execution of the code. — Peter, Nov 28 '14 at 13:42
@Peter And this has been answered many times: http://stackoverflow.com/search?q=[c]+goto Please use search next time. — 2501, Nov 28 '14 at 13:44
@VladfromMoscow: way to go Vlad... you tell the world who's good enough to code! — Tony Delroy, Nov 28 '14 at 13:51
We already have at least 20 goto discussions on SO. Pick your favourite and read it. — Lundin, Nov 28 '14 at 13:57
http://stackoverflow.com/questions/24451/is-it-ever-advantageous-to-use-goto-in-a-language-that-supports-loops-and-func — Lundin, Nov 28 '14 at 13:58
http://stackoverflow.com/questions/1073397/while1-break-instead-of-goto — Lundin, Nov 28 '14 at 13:58
http://stackoverflow.com/questions/245742/examples-of-good-gotos-in-c-or-c — Lundin, Nov 28 '14 at 13:58
The issue here is the cleanup code; the `goto` is a red herring. Wrap your cleanup code in RAII objects and you can simply `return false;` where you now have the `goto`. That'll probably allow you to get rid of the `IsSuccess` variable too. — D Drmmr, Nov 28 '14 at 14:22

score 4 · Answer 1 · edited May 23 '17 at 12:21

4

I wanted to mark this as a duplicate of this question. Still, it is not exactly the same, even though the solution is the same:

In C++, the best solution is to use RAII and transactional code. In C, the best solution is to use goto, with following a few rules (only use for return/cleanup, do not use goto to simulate loops, etc).

See my answer in the question mentioned above (basically, the solution is to use RAII and transactional code); this will eliminate completely the need for a goto cleanup/error handling block.

edited May 23 '17 at 12:21

Community

1
1

answered Nov 28 '14 at 13:45

utnapistim

26,809
3
46
82

I should not have added the c++ tag to my question. But I did hope for something positive from that side. I will stay with using goto but only for cleanup and never to simulate loops, etc. – Peter Nov 28 '14 at 13:59

score 4 · Answer 2 · answered Nov 28 '14 at 13:53

4

There is nothing wrong with using goto here. This is one of the few situations where it is the cleanest solution. (Another example would be breaking out of an inner loop.)

Using a do { ... } while (false) loop is an artificial solution that actually reduces the readability of the code.

answered Nov 28 '14 at 13:53

TonyK

16,761
4
37
72

Breaking out of a `do` loop is arguably equivalent to using a `goto` anyway... – sjdowling Nov 28 '14 at 15:38
@sjdowling: Yes, exactly! Except it's a `goto` in disguise, which makes it harder to follow the code. – TonyK Nov 28 '14 at 15:40

score 0 · Answer 3 · answered Nov 28 '14 at 13:49

In C, consider breaking your code into two functions... an outer function that does common intialisation and passes down the variables the inner function needs, such that the inner function can simply return the success status knowing the outer function will clean up.

In C++ it's usually a good idea to use scope guards so destructors ensure proper clean up. Consider your:

 tODBCX->Close();

If tODBCX needs to live longer than the function call - so a Close() in the destructor doesn't help - then create a helper:

struct Odbc_Access_Guard 
{
    Odbc_Access_Guard(ODBC& o) : odbc_(o) { }
    ~Odbc_Access_Guard() { odbc_.close(); }
    operator ODBC&() { return odbc_; }
    operator const ODBC&() const { return odbc_; }
    ODBC& odbc_;
};

Then inside your function:

Odbc_Access_Guard odbc(tODBC);
odbc.xyz();
if (whatever)
   return ...success expression...;

The same thing goes for your pointers: they should probably be shared pointers or guards using the logic above. Then you can return any time without having to even think about where to go for the clean up code, and wondering if it's up to date with the current variable use.

Using goto as a error handling strategy

3 Answers3