Use C switch statement for error handling

Question

Consider this C construct that checks for errors before doing actual work:

int function(struct Context *context,struct Connection *conn)
{
    int retval;

    switch(0)
    {   
        case 0:
            retval = BUFFER_INACTIVE;
            if(conn->mSocket == -1) 
                break;
            retval = BUFFER_FULL;
            /* Is there enough room to add ? */
            if((context->mMaxBufferSize - conn->mSendPacketLength) < aPacketLength)
                break;

            /* Is the send packet buffer half sent? */
            if(conn->mSendPacketLength > 0 && conn->mSendPacketPos != conn->mSendPacket)
                break;

            /* Do some work here */
            retval = BUFFER_DONE;
    }
    /* Do some things before returning */
    printf("%d",retval);
    return retval;
}

Would you consider this a being readable? Would the alternatives, using goto or stacked if() be better?

Answer 1

I've never seen the switch solution, but I've done stuff like this:

do {
    err = func();
    if( err ) break;
    err = func2();
    if( err ) break;
    ...
} while( 0 );
if( err ) {
   // handle errors
}

But what's the real difference between that and this:

err = func();
if( err ) goto done;
err = func2();
if( err ) goto done;
...
done:
if( err ) {
   //handle errors;
}

The first is just the second one rewritten to avoid using the keyword goto, and I'd argue that the goto solution is more readable. It took me a while, but I've managed to convince myself that gotos are not always evil.

In the end, I prefer just using if statements if possible since it makes the code more readable, but gotos if necessary.

Answer 2

I would say that is less readable. I think using a if statements or even a goto would be a much more suitable approach. Using goto's is not the end of the world and is perfectly acceptable and appropriate for error handling.

http://kerneltrap.org/node/553/2131

Answer 3

I would recommend you to use while(true) instead of switch:

while(true)
{   
            retval = BUFFER_INACTIVE;
            if(conn->mSocket == -1) 
                    break;
            retval = BUFFER_FULL;
            /* Is there enough room to add ? */
            if((context->mMaxBufferSize - conn->mSendPacketLength) < aPacketLength)
                    break;

            /* Is the send packet buffer half sent? */
            if(conn->mSendPacketLength > 0 && conn->mSendPacketPos != conn->mSendPacket)
                    break;

            /* Do some work here */
            retval = BUFFER_DONE;
            break;
}

Answer 4

Another alternative is wrapping it in a function and returning instead of breaking. This is often a bad idea since it ends up adding an unnecessary layer of abstraction. However, in some cases it can make things simpler.

Answer 5

An alternative is to use cascaded ifs:

u8 u8IsOk;

u8IsOk = Func1();

if(u8IsOk)
{
    /* Do some stuff...*/
    u8IsOk = Func2();
} /* if */

if(u8IsOk)
{
    /* Do some stuff...*/
    u8IsOk = Func3();
} /* if */

...and so on. Not as efficient as some other methods, but avoids excessive nesting, goto, break, while(0) and multiple returns.