What coding style for error checking is better and why?

Question

What coding style for error checking is better and why? (leave Exceptions away, imagine we do not have them)

int foo()
{
    int errCode = some_system_call();
    if (errCode != OK)
    {
        return myErrCode;
    }
    // continue doing foo stuff
    return myOK;
}

Or:

int foo()
{
    int returnCode = myOK;
    int errCode = some_system_call();
    if (errCode != sysOK)
    {
        returnCode = myErrCode;
    }
    else
    {
        // continue doing foo stuff
        returnCode = myOK;
    }
    return returnCode;
}

Answer 1

I would go with the first one. The second one contains a lot of unnecessary redundant code. That only confuses what is going on.

I'm personally a fan of guard clauses (the first example) as it reduces the indenting of the function. Some people don't like them because it results in multiple return points from the function, but I think it's clearer with them.

Answer 2

Given the choice, I always go for the clearest, most obvious code with the smallest amount of boilerplate content. Thus, of your choices, I would go for the obvious guard clause style:

int foo()
{
    int errCode = some_system_call();
    if (errCode != OK)
    {
        return myErrCode;
    }

    // continue doing foo stuff
    return myOK;
}

Answer 3

The first choice is better. In addition to improved clarity, it emphasizes the fact that if errCode != OK it may not be possible to // continue doing foo stuff. The prime directive of robustly using status codes is that you must handle them immediately, every single time.

Answer 4

The first. The second is a product of the "we must freak out if functions exit in more than one place, even if the guard clauses are enormously obvious" school of thought. I'm not a fan.

Answer 5

Normally, I would say that I prefer the second because it has only one return point, but if we're trying to indicate success or failure of the funciton (i.e. exceptions) then I would have to vote for the first (i.e. guard against failures). This is consistent with the way the .Net framework works.

For instance, let me switch back to the real .Net framework, the following is definitely bad practice in my opinion:

int foo()
{
    Exception ex = null;

    if(condition1)
        ex = new InvalidOperationException("condition1 occurred");

    if(condition2)
        ex = new InvalidOperationException("condition2 occurred");

    if(ex != null)
        throw ex;

    return 1 + 2; //or whatever
}

There's no need to produce guard code this way, but if we're doing something other than guarding, I would say the second example ... probably because that's the way I learned way back when in school.

Answer 6

Personally I would go with number 1, with one exception, unless I am going to be using whatever it returns elsewhere in the function I would just write the code as follows:

int foo() {
    if (some_system_call() != OK) {
        return myErrCode;
    }
    // continue doing foo stuff
    return myOK;
}

This makes it just as clear and has the exact same effect as the previous code. Yes, I understand the compiler will probably optimise the integer away, it still is extra code I am typing. I prefer my code to be clean and concise without making more work for myself as a programmer.

The whole only one return from a function paradigm just makes more work for me. If the function is written clearly, and it does not have too much code contained within, it is just as readable to have two different return statements out of the function. When the function becomes big enough that having multiple returns out of a function causes bugs the function should be re-factored and improved to be smaller and more readable.

Answer 7

int foo()
{
    int errCode = some_system_call();
    if (errCode != OK) { return myErrCode; }

    // continue doing foo stuff
    return myOK;
}

The point of foo is to make the system call and to continue doing more stuff.

Unfortunately, error checking is a fact of life. When the error checking takes up more space, it begins to look like it's part of foo's meat which is not the case.

The same thing applies in loops

for (int i=0; i<x; i++) {
  if (i%2 == 0) { continue; }

  // do some stuff.
}

This is better than:

for (int i=0; i<x; i++) {
  if (i%2 != 0) { 
    // do some stuff.
  }
}

Where 'do some stuff' starts to get lost.

Answer 8

I'd go for the first one. It is concise and much cleaner than the second option.

Answer 9

I'm a fan of the first approach, the resulting code is easier to read and understand and it is obvious where the function will return an error right off the bat. The second, not so much.

Answer 10

Even though you don't have exceptions, you can build error objects that contain useful information. Just as you can chain exceptions, you can chain error objects so that when this is reported, the developer can see the full range of events along the way that caused the problem (this is assuming that the primary audience for error information is the developer--it usually is).

Each error object should also have __FILE__ and __LINE__ recorded, which is pretty important to knowing exactly what's going on. Also, you can choose whether or not your error object used error codes; error codes are really only required if other code is trying to decide what to do in an error condition. More important to the developer is a text message describing what went wrong, and ideally with what information. Therefore a printf style constructor of the error object would go a long way.

So, assuming a simple error object implementation:

error_t* foo()
{
    int errCode = some_system_call();
    if (errCode != OK)
    {
        return error_object(__FILE__, __LINE__,
                            "some_system_call() returned %d",
                            errCode);
    }
    // continue doing foo stuff
    return NULL;
}

and the caller of foo() could return a "chained" error object that allows the developer to track the whole path of the error:

error_t* bar()
{
    error_t* errObj = foo();
    if (errObj != NULL)
    {
        return error_object_chain(errObj, __FILE__, __LINE__,
                                  "foo() failed with condition %s",
                                  implicitCondition);
    }
    return NULL;
}

Answer 11

The first one is preferrable for cases where the exit points are obvious. However, be careful exiting a function where things are allocated dynamically (using malloc or new). If you don't perform cleanup of such objects before leaving the function, you have a memory leak. The stl's auto_ptr class can be of enormous help here (also when using exceptions without a finally clause).

Answer 12

In case of a COM I would recommend to consider _com_error class (see MSDN for more information).

try 
{
 _com_util::CheckError(some_system_call());
 //here continue with more code
}
catch(const _com_error& e)
{
 //here handle your error and return
}

If it is not COM then I would probably implement a very similar class.

Answer 13

If you do not have exceptions, then you may be forced to use goto's to clearly handle errors.

Example: (Note that this is essentially your option 1)

int foo() {
  int ret = ERROR; // Default value.

  if (bar1() == ERROR) goto CLEANUP_1;
  if (bar2() == ERROR) goto CLEANUP_2;
  if (bar3() == ERROR) goto CLEANUP_3;

  ret = SUCCESS;

// Now, cleanup resources in reverse order.
CLEANUP_3:
  cleanup_bar3_resources();
CLEANUP_2:
  cleanup_bar2_resources();
CLEANUP_1:
  cleanup_bar1_resources();

return ret;
}

Answer 14

At least in C, I use:

int foo()
{
    if ((value = some_system_call()) != OK) {
        return myErrCode;
    }

    // continue doing foo stuff
    return myOK;
}

If the line'd be too long, or if the language doesn't support assignment within an if clause (say, Python):

int foo()
{
    value = some_system_call()

    if (value != OK) {
        return myErrCode;
    }

    // continue doing foo stuff
    return myOK;
}

The point being: avoid the superficial else; it's less indentation for the same result.

Answer 15

I would go with the second but flip the logic and change the name.

if (resultOfSystemCall == OK)