Replacing Items in an NSMutableArray

Question

I have a function which is supposed to replace all instances of variables (represented as NSStrings) from an NSDictionary with their corresponding values before calling another procedure. I ended up doing this with a simple for loop, though I suspect that there is a better way. Do you have a suggestion how to do this differently?

+ (double)runProgram:(id)program usingVariables:(NSDictionary *)variables {        
    NSMutableArray *stack;
    if ([program isKindOfClass:[NSArray class]]) {
        stack = [program mutableCopy];
        for (int i=0; i<stack.count; i++) {
            /* replace variable values with NSNumber values */
            NSNumber *variableValue = [variables valueForKey:[stack objectAtIndex:i]];
            if ([variableValue isKindOfClass:[NSNumber class]]) {
                [stack replaceObjectAtIndex:i withObject:variableValue];
            }
            /* all variables should be replaced now */
        }
    }
    return [self popOperandOffProgramStack:stack];
}

Answer 1

+ (double)runProgram:(id)program usingVariables:(NSDictionary *)variables {        
    NSMutableArray *stack;
    if ([program isKindOfClass:[NSArray class]]) {

Checking the type of incoming parameters is a little suspicious. Do you really want to allow other parameters and just ignore them?

        stack = [program mutableCopy];

This process would actually be easier if you didn't create a copy, but instead create an empty NSMutableArray and add the filtered objects to it.

        for (int i=0; i<stack.count; i++) {

Presumably, you are using this because you need the indexes to replace the objects. If you stop making a copy, you should be able to replace this a foreach loop over the program.

            /* replace variable values with NSNumber values */
            NSNumber *variableValue = [variables valueForKey:[stack objectAtIndex:i]];
            if ([variableValue isKindOfClass:[NSNumber class]]) {

Again why are you checking types? Are there other things besides numbers in your variables? If there are, do you just want to ignore them?

                [stack replaceObjectAtIndex:i withObject:variableValue];
            }
            /* all variables should be replaced now */

This comment isn't true here, because its on the wrong side of the brace

        }
    }
    return [self popOperandOffProgramStack:stack];
}

Answer 2

I'm going to handle this answer in two parts, as they're two distinct issues I want to address here:

+ (double)runProgram:(id)program usingVariables:(NSDictionary *)variables {        
    NSMutableArray *stack;
    if ([program isKindOfClass:[NSArray class]]) {
        stack = [program mutableCopy];
        for (int i=0; i<stack.count; i++) {
            /* replace variable values with NSNumber values */
            NSNumber *variableValue = [variables valueForKey:[stack objectAtIndex:i]];
            if ([variableValue isKindOfClass:[NSNumber class]]) {
                [stack replaceObjectAtIndex:i withObject:variableValue];
            }
            /* all variables should be replaced now */
        }
    }
    return [self popOperandOffProgramStack:stack];
}

First, let's change the method signature to something that makes a whole lot more sense:

+ (double)runProgram:(NSArray *)program usingVariables:(NSDictionary *)variables;

The method only makes sense if program is an array. If it's not an array, then what does the method body actually look like? It looks like this:

return [self popOperandOffProgramStack:nil];

So let's change the argument to NSArray * rather than id. Now our method is more explicitly clear to callers that we except an NSArray, and if you don't have an array to send, send nil, because that's how we're going to use any non-array argument anyway.

Now that we've change the argument, we can get rid of the type check in the body.

---

Now to the second point I want to address in the body. First, in Objective-C, forin loops are faster then the other available loops, so use them where you can.

I think the method body works much better as such:

+ (double)runProgram:(NSArray *)program usingVariables:(NSDictionary *)variables { 
    NSMutableArray *stack = [NSMutableArray array];
    for (id obj in program) {
        if ([variables[obj] isKindOfClass:[NSNumber class]]) {
            [stack addObject:variables[obj]];
        } else {
            [stack addObject:obj];
        }
    }
    return [self popOperandOffProgramStack:stack];
}

As a note, the else block is in there simply in order to rewrite your existing code and produce identical results. If objects that are not NSNumbers should be added to the array, simply remove the else block.