Good practice - lot of to do at the function termination - goto alternative

Question

Possible Duplicate:
Valid use of goto for error management in C?

Recently I've encountered C code like this:

if( !condition1){
    goto failure;
}

some_stuff_that_needs_manual_undoing();

if( !condition2){
    goto failure;
}

// And many more lines such as the ones above
return 0;

failure:
// Do a lot of stuff here, free, file closing and so on
return -1;

To sum up the situation:

I have a long function that do several things in a row (let's say open a file, then allocate memory based on file contents, then connect do database and so so on). Of course I want to free resources correctly, but there are lot of places which can cause function to end prematurely (and all of them need clean up).

The Question: How to do this correctly?

Although goto doesn't seem to be that bad practice it also doesn't seem to be good solution.

I though of following:

Use a macro which would do the job, e.g.:

#define CLEANUP if( memory != NULL)free(memory); \
                if( fp != NULL) fclose(fp);\
                // ...

if( !condition1){
    CLEANUP
    return -1;
}

if( !condition2){
    CLEANUP
    return -2;
}

// ...

This will result in duplicate assembly, but the cleanup code would be in one place.

Encapsulate function into another function

int _some_stuff_do_work(void **memory, FILE **file, ...){
    // Would just return on error
}

int some_stuff() {
    void *memory = NULL;
    FILE *file = NULL;

    _some_stuff_do_work( &memory, &file, ...);
    if( fp) fclose(fp);
}

This could get really ugly if there will be more than 3-5 things that will need cleaning up (the function would then take many arguments and that always calls for a problems).

OOP - Destructor

typedef struct {
    void *memory;
    FILE *fp;
} LOCAL_DATA;

// Destructor
void local_data_destroy( LOCAL_DATA *data)
{
    if( data->fp){
        free(data->fp);
        data->fp = NULL;
    }
}

But this could lead to many functions (and structures) that would be used only once in whole application and it seems like it could create a hell of a mass.

Loop and break statements

while(1){
    if( !condition1){
       break;
    }

    // ...

    break;
}

if( fp) fclose(fp);

I found this on may places but using one iteration loop? I don't know, it seems to be completely un-intuitive.

There's another possibility: `do{ /*...*/ }while(false);` and `break` statements. — dyp, Oct 10 '12 at 16:31
Actually, I think that from these solutions, the original code with `goto` is the best. :) — Gabor Csardi, Oct 10 '12 at 16:34
Vyktor, there is no need to check for NULL before passing pointer to `free`. It guarantees to ignore `NULL` pointers. This wasn't true like 10 years ago, but now those checks don't make sense. — , Oct 10 '12 at 16:35
@DyP thanks, though about this one, but forgot to put it down, added. — Vyktor, Oct 10 '12 at 16:35
Cleaning up in a function and having a single exit point is one of the situations where a goto is perfect. This is still structured programming, not spaghetti code. — Zane, Oct 10 '12 at 16:36
@BoPersson I'm asking what's the best practice for this (thus the scope of my question seems wider than the question you posted to me). — Vyktor, Oct 10 '12 at 16:39
@Vyktor - Some of us believe that there is no best practice for using goto. :-) In my experience, a *reasonable* use might appear once every 10 years or so. That's about how often I have used it. — Bo Persson, Oct 10 '12 at 16:44
@BoPersson so how would you write a code for mentioned situation (that's what I'm asking in my question, what's effective and readable alternative for it). — Vyktor, Oct 10 '12 at 16:45
@Vyktor - I would try to put all the different actions in their own separate functions, and let them each clean up their own mess. The base problem might be trying to do too many things in one large function. — Bo Persson, Oct 10 '12 at 16:48
@BoPersson the things are mixed together, in last step you need resources allocated in first step and so on... So basically you would go class/object/struct. :) — Vyktor, Oct 10 '12 at 16:54

score 6 · Accepted Answer · answered Oct 10 '12 at 16:48

6

Goto is the way to go. First understand why exactly "goto is bad" and then you will see that in situation you're describing goto is not really bad. Avoiding goto at all costs is just wrong and comes from shallow understanding of good programming principles.

This is way to go (Linux kernel source): http://goo.gl/uSgp5

answered Oct 10 '12 at 16:48

dpc.pw

3,462
1
19
24

+1. Gotos are awesome, actually. Gotos, gotos everywhere! – Oct 10 '12 at 16:50
1

+1 for great source ;) `goto` = `jmp`, you can rewrite every language control construct in C into goto :) – Vyktor Oct 10 '12 at 16:52

Good practice - lot of to do at the function termination - goto alternative

Use a macro which would do the job, e.g.:

Encapsulate function into another function

OOP - Destructor

Loop and break statements

1 Answers1