Programming style: Reoccuring error checks

Question

Hey, I have a question about programming style, because in my current code I am using a bigger function which calls some smaller functions and all of these need to be error-checked. So something like this:

 void bigFunction() {
      /* some computations */
      if(smallFunction1() == -1) {
           free(mem1);
           free(mem2);
           fclose(file);
           unlink(filename);
           return -1;
      }
      if(smallFunction2() == -1) {
           free(mem1);
           free(mem2);
           fclose(file);
           unlink(filename);
           return -1;
      }
        if(smallFunction3() == -1) {
           free(mem1);
           free(mem2);
           fclose(file);
           unlink(filename);
           return -1;
      }
      /* more computations and stuff in biggerFunction */
 }

I think you can clearly see my problem: The code after one of these functions fails is always the same, and I feel like repeating this coder again and again will make my code more and more unreadable.

How to deal with this problem? gotos came into my mind, but in my programming courses in university I was told never use gotos (though I forget the reason why...)

Answer 1

How about

 void bigFunction() {
      /* some computations */
      if(smallFunction1() == -1 || smallFunction2() == -1 || smallFunction3() == -1) {
           free(mem1);
           free(mem2);
           fclose(file);
           unlink(filename);
           return -1;
      }
      /* more computations and stuff in biggerFunction */
 }

Or am I missing something here?

Answer 2

One option is to put the repeated code into its own method:

void bigFunction() {
    /* some computations */
    if(smallFunction1() == -1) {
        CleanUpAfterError();
        return -1;
    }
    if(smallFunction2() == -1) {
        CleanUpAfterError();
        return -1;
    }
    if(smallFunction3() == -1) {
        CleanUpAfterError();
        return -1;
    }
    /* more computations and stuff in biggerFunction */
}

void CleanUpAfterError() {
    free(mem1);
    free(mem2);
    fclose(file);
    unlink(filename);
}

Answer 3

This is probably the only circumstance where a goto would be acceptable, but you can do it without.

void bigFunction() {
    /* some computations */
    bool has_error = smallFunction1() == -1;
    if (!has_error) {
        has_error |= smallFunction2() == -1;
    }
    if (!has_error) {
        has_error |= smallFunction3() == -1;
    }
    if(has_error) {
        free(mem1);
        free(mem2);
        fclose(file);
        unlink(filename);
        return -1;
    }
    /* more computations and stuff in biggerFunction */
}

Answer 4

In C this is one of those cases where it can be argued that judicious use of goto is justifiable, e.g.

void bigFunction()
{
    // NB: important to initialise these so that we can clean up properly in all cases
    void * mem1 = NULL;
    void * mem2 = NULL;
    FILE * file = NULL;

    /* some computations */

    if (smallFunction1() == -1)
        goto CleanUpAndExit;

    if (smallFunction2() == -1)
        goto CleanUpAndExit;

    if (smallFunction3() == -1)
        goto CleanUpAndExit;

    /* more computations and stuff in biggerFunction */

cleanUpAndExit:
    free(mem1); // NB: OK to call free on NULL pointer
    free(mem2);
    if (file != NULL) fclose(file); // NB: NOT OK to call fclose on NULL FILE *
    unlink(filename);
    return;
}

Of course in other languages there are better ways of doing this kind of thing, e.g. exceptions in C++.

Answer 5

goto is generally frowned upon, and in 25 years I think I've had to use it about once.

However, these kind of thing take on religious dogma overtones sometimes and rational thought gets pushed to one side.

If a goto is the simplest, cleanest way to go, then use it. In the end clarity is good. Torturing yourself to avoid using a goto is wasting your valuable time, and causing confusion to the maintainer who follows.

If in doubt, explain WHY you did it this way with a comment.

My coding standard says that goto is forbidden. But it also has a section in it saying that any of the rules can be broken for good reason, so long as the reason is explained in the source code with a comment. This allows anything at all that is pragmatic to be done if its the best thing for the task. Coding standards should be about best practices, with an "out" clause if it makes sense.

Answer 6

Your intuition is quite right. How to break up the function depends on exactly what the different bits do (feel free to post some more details). But off the top of my head, promising possibilities would be:

• If it's possible to use some C++ code in this project, don't use malloc/free, instead use new and assign it to an auto_ptr, and then when you "return" and the variable goes out of scope, the memory will automatically be deleted. (A similar approach can be used to automatically do the other clean-up whenever the variable goes out of scope. The awkwardly-named keyword is "RAII: Resource Acquisition Is Initialisation"

• If it's possible to use some C++ code in this project, it may or may not be appropriate to turn part or all of this function into a class, which takes some of the arguments as constructor arguments, and performs the clean-up on destruction, and the guts of the function in one or more member functions. (You may be able do a similar thing in C, by putting the variables in a struct, and having all the relevant functions take a pointer to an instance of it.)

• I assume the clean-up code relies on member variables in bigFunction? (if not, it would be reasonable to break that out into a separate function.) But you may be able to break out other parts. For instance, how about:

bool /* or int */ smallFunctionsOK( /* args */ ) {
    if (smallFunction1( /* args */ ) == -1) return false;
    if (smallFunction2( /* args */ ) == -1) return false;
    if (smallFunction3( /* args */ ) == -1) return false;
    return true;
}

void bigFunction() {
        someComputations( /* args */ );
     if(smallFunctionsOK()) {
          moreComputations( /* args */ );
     } else {
          free(mem1);
          free(mem2);
          fclose(file);
          unlink(filename);
          return -1;
     }
}

You probably can't do all of that because some of the parts need to stay in the main function, but see if you can break out some parts of it. Don't be afraid to put the important bits in another function and the boring cleanup in this one -- that's equally likely to read easily as the reverse.

Answer 7

There is (at least) one context in which you can clean up the code quite a bit by placing the burden of checking the status on the small functions instead of the big one. This should only be used in a situation where the functions are tightly related and consistently used in similar succession.

Each small function takes a status flag as part of its parameters and checks it first before proceeding to its respective computation.

void smallFunction1(..., bool* status)
{
    if(*status != True)
        return;

    // Computation...
}

The big function is then coded as follows:

bool status = True;
smallFunction1(..., &status);
smallFunction2(..., &status);
smallFunction3(..., &status);
if(!status)
{
     // Clean up...
     return -1;
}

Admittedly, there is an opportunity here for some wasteful processing, where several function calls are made for no reason. So this is best used in cases where the expected error rate is low.

One example where this sort of thing may be fitting would be where several fields of a data packet are encoded separately. E.g., encodeHeader(...), encodePayload(...), encodeCRC(...), etc.

Answer 8

Different languages will handle that in different ways.

In C++, your best bet is to use objects and have the cleanup in the destructors, using RAII. One advantage is that this keeps all the code for something in one place, rather than having everything in an initialization and a cleanup section.

In some languages, you can put a try { ... } finally { ... } construct in for cleanup.

C itself has neither of these, so you're going to have to find a way to fake it. One technique is goto (gotos aren't so bad when used in limited ways; guidelines are never use one unless to jump forwards to a significant section of code).