Take the 2-minute tour ×
Programmers Stack Exchange is a question and answer site for professional programmers interested in conceptual questions about software development. It's 100% free, no registration required.

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...)

share|improve this question
1  
This might be better asked on Stack Overflow. Also, pedantically, recurring is the word you're looking for. –  Matt Ellen Apr 4 '11 at 7:49
    
@Matt: perhaps nitpickingly would be better than pedantically :-P –  Newtopian Apr 8 '11 at 3:00
    
@Chris: I have added the C tag since such clean-up logic is much easier to get right in languages with higher-level constructs (RAII, using, with, ...) –  Matthieu M. Apr 8 '11 at 18:13
    
I did consider migrating this to Stack Overflow, but it's about coding style rather than a specific problem with the code. –  ChrisF Apr 8 '11 at 18:46
    
@ChrisF: this question may be suitable for codereview.stackexchange.com –  rwong Apr 9 '11 at 4:50

8 Answers 8

up vote 11 down vote accepted

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?

share|improve this answer
    
yes, you are missing something. In the OP's code smallFunction2 isn't called if smallFunction1 fails and smallFunction3 isn't called if smallFunction2 or smallFunction1 fails. –  dan_waterworth Apr 4 '11 at 8:12
8  
@dan: with most implementation of C and C-like languages, evaluation of || clauses stop at first true, so smallFunction2() isn't called if smallFunction1() returns -1. –  jv42 Apr 4 '11 at 8:22
1  
@stijn, I've written a test and you are indeed right. –  dan_waterworth Apr 4 '11 at 8:31
4  
well I surely hope so because a lot of code I wrote relies on this (it's called 'short-circuit evaluation'; that's why I'm asking jv42 why his statement seems to be that not all implementations of C/C++ support this: that sounds scary, becuase it would mean my code would break very, very hard on those implementations –  stijn Apr 4 '11 at 8:34
5  
clarification here: stackoverflow.com/questions/628526/… So it's madated for C and C++, but the caveat is that in C++ it is not for overloaded operator || and &&. So I'm safe, but learnt a lesson: I had no idea there were languages that doe not support this. –  stijn Apr 4 '11 at 8:39

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.

share|improve this answer

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).

share|improve this answer

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.

share|improve this answer

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.

share|improve this answer
1  
Those who downvote this answer: It would be nice for you to explain why. Downvoting without an explanation is puzzling, and makes me think you have a problem with being pragmatic. –  quickly_now Apr 29 '11 at 10:57

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++.

share|improve this answer
    
Although I disagree with religious stance on any rules in programming and I do agree with you this works and is a justifiable use of goto; I still feel uncomfortable using it here. Main reason is that if bigFunction's extra stuff gets very large the goto falls out of view. In this case the risk for the goto ending in the wrong place following a refactor of bigFunction is very real. This would most likely still result in a working system but the side effects could produce very difficult to diagnose bugs and side effects. It would at least warrant a big comment warning. my 2 cent. –  Newtopian Apr 8 '11 at 3:15
    
@Newtopian: yes, fair comments - it's not an ideal solution, but I don't believe that there is an ideal solution for this kind of thing in C. I do firmly believe in having a single exit point for each function though, and ditto for having a single block of code which is responsible for clean up etc, so this addresses those two concerns at least. –  Paul R Apr 8 '11 at 6:59
2  
@Newtopian: if bigFunction ends up larger than a page, it's time to refactor it anyway. –  Matthieu M. Apr 8 '11 at 18:11
    
@Matthieu : True. Though I sometimes find refactoring very simple but boringly large functions makes the code harder to read. –  Newtopian Apr 9 '11 at 11:07

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 */
}
share|improve this answer
    
Although the short circuit answer was the accepted one, after learning from the comments I think I would have answered something along these lines. –  Newtopian Apr 8 '11 at 3:16

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);
}
share|improve this answer
3  
The problem with this approach is that I either need to pass CleanUpAfterError all variables that need to be cleaned or make them global variables that it can access it, which is no good option –  Chris Apr 4 '11 at 7:59
    
+1 This solution is the most readable when possible. In Chris' case, it seems it's not a very good option. –  jv42 Apr 4 '11 at 8:23
1  
@Chris: if your vars are related, you could have them in a struct and have just a single struct to pass around, which is quite future-proof (no need to rewrite all the calls and change method signature if you have to add/remove vars). –  jv42 Apr 4 '11 at 8:25
    
@Chris: it wasn't clear from your question where the variables were declared. I assumed they were class level variables. –  Kristof Claes Apr 4 '11 at 9:13
    
@Kristof, Looking at the code and it's use of free, unlink and fclose, I'd say it was C, therefore there are no classes. –  dan_waterworth Apr 4 '11 at 13:56

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.