1
\$\begingroup\$

I have an NSArray called program and I need to scan it and then produce an NSSet of objects that fit the test isOperation:. If none are found I should return nil. I wrote the following code. How would you make this better? (FYI - the fact the program is of type (id) comes directly from the assignment so I can't change that one thing)

Revised code:

+ (NSSet *) variablesUsedInProgram:(id)program {
    NSSet *programSet, *variablesUsed;
    if([program isKindOfClass:[NSArray class]]) {
        programSet = [NSSet setWithArray:program];
        variablesUsed = [programSet objectsPassingTest:^BOOL(id term, BOOL *stop) {
            return (! ([term isKindOfClass:[NSNumber class]] || [CalculatorBrain isOperation:term]));  
        }];
    }
    return ([variablesUsed count] > 0) ? variablesUsed : nil;
}
\$\endgroup\$
2
\$\begingroup\$

There is no need to return a copy of the variablesUsed set. It's actually wrong to do so, because naming conventions for your method imply an autoreleased object to be returned. Thus I would rewrite your return as:

return [variablesUsed autorelease];
// or in case of ARC just
return variablesUsed;

See the Advanced memory management programming guide

Also set contains a useful method to filter using a block. In my opinion this results in slightly nicer code. You can convert your incoming array to a NSSet using + setWithArray:.

The if statement to return nil can be combined in your return.

Combining this results in the following implementation

+ (NSSet *) variablesUsedInProgram:(id)program {
    NSSet *programSet = [NSSet setWithArray:program];
    NSSet *variablesUsed = [NSSet objectsPassingTest:^(id object, BOOL *stop) {
        return !([object isKindOfClass:[NSNumber class]] || [self isOperation:term])
    }];
    return ([variablesUsed count] > 0) ? variablesUsed : nil;
}
\$\endgroup\$
  • \$\begingroup\$ Thank you - this is a very nice solution. By the way, I have two questions: \$\endgroup\$ – jaresty Feb 7 '12 at 9:20
  • \$\begingroup\$ 1. What is "BOOL *stop" used for? and \$\endgroup\$ – jaresty Feb 7 '12 at 9:21
  • \$\begingroup\$ 2. Why is it wrong to return a copy of variablesUsed? \$\endgroup\$ – jaresty Feb 7 '12 at 9:22
  • \$\begingroup\$ Incidentally, I am using ARC - should I be calling release in any situation? I was under the impression that it is reserved for the compiler under ARC. \$\endgroup\$ – jaresty Feb 7 '12 at 9:23
  • \$\begingroup\$ 1. "The block can set the value to YES to stop further processing of the set." - see the NSSet documentation. Not needed in your case. \$\endgroup\$ – Joris Kluivers Feb 7 '12 at 10:17
2
\$\begingroup\$

I want to propose something similar to the answer Joris gave.

Inspired by List Comprehensions

python: l = [i**2 for i in l if i%2] #filtes a list for all even numbers and squares them

I wrote a number of category methods on NSArray to be able to do something similar using Blocks.

here one that fits to your needs:

interface

#import <Foundation/Foundation.h>

@interface NSArray (FunctionalTools)

-(NSSet *)setByPerformingBlock:(id  (^)(id element))performBlock
           ifElementPassesTest:(BOOL (^)(id element))testBlock;

@end

implementation

@implementation NSArray (FunctionalTools)

-(NSSet *)setByPerformingBlock:(id   (^)(id))performBlock 
           ifElementPassesTest:(BOOL (^)(id))testBlock
{
    NSMutableSet *set = [NSMutableSet setWithArray:self];
    NSMutableSet *newSet = [NSMutableSet set];
    for (id element in set){
        if (testBlock(element)) {
            [newSet addObject:performBlock(element)];
        }
    }
    if ([newSet count]<1)
        return nil;
    return [NSSet setWithSet:newSet];
}
@end

usage

+ (NSSet *) variablesUsedInProgram:(id)program {
     return [program setByPerformingBlock:^id(id term) {
        return term;
     } ifElementPassesTest:^BOOL(id term) {
        return !([term isKindOfClass:[NSNumber class]] || [self isOperation:term]);
     }];
\$\endgroup\$
  • \$\begingroup\$ Cool! :) I like this technique... any downsides? \$\endgroup\$ – jaresty Feb 7 '12 at 12:03
  • \$\begingroup\$ I don't like return [NSSet setWithSet:newSet];, but I think, it is better, to not surprise the programmer by returning a mutable set. \$\endgroup\$ – vikingosegundo Feb 7 '12 at 13:43
  • \$\begingroup\$ In the implementation why are you creating an NSMutableSet out of the array (self) just to turn it back into an array in the for statement? \$\endgroup\$ – Nathan Kinsinger Feb 9 '12 at 19:00
  • \$\begingroup\$ I am creating a set to avoid that possibly expensive blocks are executed more often than needed. [set allObjects]was a copy and past error. \$\endgroup\$ – vikingosegundo Feb 9 '12 at 19:52
  • \$\begingroup\$ I disagree that it's at all important to care whether your set is mutable or not. Your return type is NSSet*, so if the programmer even finds out that your set is mutable, they already messed up bigtime. \$\endgroup\$ – andyvn22 Apr 26 '12 at 2:31
0
\$\begingroup\$

The only real issue I see is that you're leaking memory. You are allocating a mutable set that you never release and then copying it to presumably make it immutable thus leaking more memory.

+ (NSSet *) variablesUsedInProgram:(id)program {
    NSMutableSet *variablesUsed = [[NSMutableSet alloc] init];
    for (id term in program) {
        if(! ([term isKindOfClass:[NSNumber class]] || 
              [self isOperation:term])) {
            /* term is a variable, because it's neither an operation or a number */
            [variablesUsed addObject:(NSString *)term];

        }
    }
    NSSet returnSet = nil;
    if (variablesUsed.count > 0) {
        returnSet = [NSSet setWithSetVariableSet];
    }
    [variablesUsed release];
    return returnSet;
}
\$\endgroup\$
0
\$\begingroup\$

In your newer version, if program is not an NSArray then variablesUsed is uninitialized. Set it to nil where you declare it.

Remember to always check failure paths in conditional statements.

Also you should start using Xcode's Build and Analyze option to get the LLVM compiler to point out these kinds of errors.

\$\endgroup\$

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.