NSTask with Git

This is my first real Cocoa app that I am writing. The first version of this app was just one long applescript. So in my app auto git, I have an NSTask instead of Tell terminal. My code looks like this:

-(void) runScript:(NSString *) path:(NSString* )cmd1:(NSString *) cmd2:(NSString *) cmd3{

    NSTask *aTask = [[NSTask alloc] init];

    NSPipe *pipe;
    pipe = [NSPipe pipe];
    [aTask setStandardOutput: pipe];

    NSFileHandle *file;
    file = [pipe fileHandleForReading];

    NSArray* args = [NSArray arrayWithObjects:cmd1,cmd2,cmd3, nil];
    [aTask setArguments:args];
    [aTask setCurrentDirectoryPath:path];
    [aTask setLaunchPath:@"/usr/local/git/bin/git"];
    [aTask setArguments:args];
    [aTask launch];

    [finished setStringValue:@"finished"];

}

Example usage:

[self runScript:dirPath:@"add" :@"*" :nil];
[self runScript:dirPath:@"commit" :@"-m" :commitText];
[self runScript:dirPath:@"push" :@"origin" :@"HEAD"];

Is there a better way to do this? And what about errors? How to I let the user take care of custom errors?

All help appreciated.

First of all, I don't care for this style of method naming in the slightest. This is Objective-C. Our method names should be verbose and descriptive. If the method name is too long, code completion will help us out. But if it's too short, and we have other methods that perform similar tasks, we may easily get confused. This is particularly true when the person who is implementing this code or maintaining code that uses your code ends up being someone other than yourself.

- (void)runScript:(NSString*)path 
         command1:(NSString*)command1
         command2:(NSString*)command2
         command3:(NSString*)command3;

Already this method name is better, but it still needs work because the argument following "runScript" is labeled as (and internally used as) a path. This isn't the Objective-C way.

And if I'm honest, the command1, command2, command3 is a little confusing. The values sent here are arguments, not values. You know this, because as soon as you dealt with them, you moved them into an array called args, and then put them into the task using setArguments:.

Here's how I'd change method signature:

- (void)runScript:(NSArray*)arguments inDirectoryPath:(NSString*)path;

Now instead of taking multiple arguments, we take the arguments in an array.

But this alone probably isn't good enough. We do need to verify that they're actually sending us arguments, as NSTask will raise an exception if the arguments are nil.

So, let's modify the method call one more time:

- (void)runScript:(NSArray*)arguments 
  inDirectoryPath:(NSString*)path
            error:(NSError**)error;

This is the way Apple handles errors with many of the built-in Objective-C classes.

The user can pass nil for the error argument if they don't care about it, but if they do, we can give them some useful information about the error back using it.

Now let's verify the array will be okay to pass as the arguments array:

@autoreleasepool {
    NSString *errorString;
    NSInteger *errorCode;
    BOOL errorExists = NO;
    if ([arguments count] < 2) {
        errorString = @"Invalid Arguments: too few arguments";
        errorCode = 100;
        errorExists = YES;
    } else {
        for (NSObject *arg in arguments) {
            if (![arg isKindOfClass:[NSString class]]) {
                errorString = @"Invalid Arguments: arguments must be strings";
                errorCode = 101;
                errorExists = YES;
            } else if (!arg) {
                errorString = @"Invalid Arguments: arguments cannot be nil";
                errorCode = 102;
                errorExists = YES;
            }
            if (errorExists) {
                break;
            }
        }
    }
    if (errorExists) {
        if(error) {
            *error = [NSError errorWithDomain:errorString
                                         code:errorCode
                                     userInfo:nil];
        } 
        return;
    }
}

Now, before NSTask has an opportunity to raise an exception, we've stopped executing our method and given some feedback.

This can be even more helpful if you change the return type from void to BOOL. Then in the above snippet, we'd change return; to return NO;.

And then your code could be used as such:

if(![self runScript:aScript inDirectoryPath:aPath error:&error]) {
    // some code to make user aware of the error
}

StackExchange Code Review Q#12494, answer score: 5