tags:

views:

72

answers:

4

Generally speaking goto is bad (we all know why) What would be a better way to implement a simple cleanup on an error (as in the example below) without having to duplicate the code. To my mind the code below is fine, I was just curious what others would do:

int main (int argc, const char * argv[]) {
    NSAutoreleasePool * pool = [[NSAutoreleasePool alloc] init]
    int returnCode = 0;

    // ... Other code

    // WRITE: To file
    if([dataStore writeToFile:savePathData atomically:YES] == NO) {
        NSLog(@"writeToFile ... Error");
        returnCode = 1;
        goto cleanUpCode;
    }

    // ... Other code

    // Clean up
    cleanUpCode:
    [archiver release];
    [newPlanet release];
    [pool drain];
    return(returnCode);
}

EDIT_001:

In general terms I agree that @try, @catch, @finally are more than certainly the way to go but I do have two small concerns.

(1) The three @try, @catch, @finally blocks must all be together, you don't have the flexibility to place code between the @catch and @finally that you might want to bypass.

(2) Apple's documentation for Obj-C 2.0 states the following: "Important: Exceptions are resource-intensive in Objective-C. You should not use exceptions for general flow-control, or simply to signify errors (such as a file not being accessible)".

much appcreciated

gary

+5  A: 

This is a common idiom in C code, and I don't think there is anything wrong with it in the absence of a language with more comprehensive creation/destruction semantics.

Goto is considered harmful in the general sense; it can make program execution difficult to follow. Here, it's very limited in scope, and the flow is unidirectional. It's also helpful in the sense that it allows you to localize your cleanup code, which is a good thing.

Terry Mahaffey
worse, he's using objective-c, which has try/catch and not using it.
KevinDTimm
+4  A: 

Well, first of all I would try to refactor it into some more methods, maybe then the code would not be so illegible?

Also, you can always use autoreleased objects and so you only need the [pool drain] at the end. And this can probably be done using @try/@finally, so that you can just return 1 instead of returnCode = 1.

E.g. take a look at the Exception Handling and Memory Management document (I know your question is not about exceptions, but this is basically all the same) - they recommend something like this:

 - (void)doSomething {
    NSMutableArray *anArray = nil;
    array = [[NSMutableArray alloc] initWithCapacity:0];
    @try {
       [self doSomethingElse:anArray];
    }
    @finally {
       [anArray release];
    }
}

Personally, this is what I would do.

Adam Woś
+1 for suggesting refactoring
Larry Watanabe
A: 

As you are describing something that looks like a small command line program I wouldn't worry about it. Because when your program ends all memory allocated by the program will be returned to the system (it will cease to exist). But, if you had a bigger program that allocates a lot and runs for a while you might look into cleaning more correctly as it can help tracking down memory leaks which could affect a program that runs a longer time. In that case I'd also suggest using some macros.

#define THROW( _name ) goto label_ ## _name
#define CATCH( _name ) label_ ## _name:

    :
    :

    if([dataStore writeToFile:savePathData atomically:YES] == NO) {
        NSLog(@"writeToFile ... Error");
        returnCode = 1;
        THROW( cleanUpCode );
    }

    :
    :

    CATCH( cleanUpCode )

    [archiver release];
    [newPlanet release];
    [pool drain];
epatel
And why introduce such macros, if you actually have language constructs such as `@try` and `@finally`, which result in clearer code? Also, I find the names throw/catch misleading for anyone who might read the code after e.g. a month.
Adam Woś
@adam.wos Sure, but I'd say it depends. What kind of program is it? As he/she has written it I'd say *Keep It Simple* (don't worry too much about it which is my first point)
epatel
wow, that's awful.
KevinDTimm
A: 

There are many ways to do this. Here's a couple:

  1. Write a procedure corresponding to your main code, e.g. RunApplication. Put the call to it in the "else" branch.

  2. Put it in a try/catch, do the error handling in the "catch" section, normal processing otherwise.

Code

 e.g. try {
  .. check for error (any function/procedure that checks for an error can throw an exception
 .. do normal processing (still a good idea to factor this out into a procedure, for clarity
 }
 catch  {
     .. handle errors
 }
 finally
 {
     .. do cleanup
 }

Anytime you think you need a goto, for an error case, you will probably find that a try/catch and exception works better.

In languages without try/catch, just use refactoring.

Larry Watanabe