8
\$\begingroup\$

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];
}
\$\endgroup\$
5
\$\begingroup\$
+ (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];
}
\$\endgroup\$
11
  • \$\begingroup\$ Also, if 'program' is not an NSArray then 'stack' is uninitialized in the return statement. \$\endgroup\$ – Nathan Kinsinger Feb 4 '12 at 17:37
  • \$\begingroup\$ Thank you for your feedback. I agree on most points. In the case of the NSNumber *variableValue test, I imagine that there will be some objects which will not be found in the variables NSDictionary. Rather than testing if there is nothing in the dictionary for that key, I opted to test that the object returned was in fact a kind of NSNumber before doing anything. I don't want to replace anything if there is nothing in variables for that key. \$\endgroup\$ – jaresty Feb 5 '12 at 9:00
  • \$\begingroup\$ Do you think it's better to explicitly test for null instead? \$\endgroup\$ – jaresty Feb 5 '12 at 9:00
  • \$\begingroup\$ The technique of testing classes and ignoring input if it is not what you expect is something I learned from watching the iPhone dev videos from Stanford (stanford.edu/class/cs193p/cgi-bin/drupal), although I'm not sure if I'm doing it correctly... The runProgram:usingVariables is API so I can't be certain that the input is as I would expect. How would you handle erroneous values? \$\endgroup\$ – jaresty Feb 5 '12 at 9:05
  • \$\begingroup\$ By the way, I really liked your suggestion to create an empty NSMutableArray and add each step (replacing variables on the way) rather than replacing variables later. It's much easier to read - thank you! \$\endgroup\$ – jaresty Feb 5 '12 at 9:18
5
\$\begingroup\$

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.

\$\endgroup\$
0

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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