Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

I'm just beginning my foray into iOS development and need a code review. I have a custom NSManagedObject that needs some smart setters. New to obj-c, iOS, and core-data development, I don't trust myself yet. I come from a Java background, so smart setters are what I know, but might not be best when using Core Data. Here is the implementation.

+(void)initialize
{
    scoredKeys = @[@"fullSwings", @"putts", @"fairwayBunkers", @"greenBunkers", @"chipsOrPitches", @"rescue", @"wastedOrPenalty"];
}


-(id) initWithHoleNumber:(int)holeNumber
{
    self = [super init];
    if (self) {
        [self setHoleNumber:[NSNumber numberWithInt:holeNumber]];
    }
    return self;
}

-(void) incrementItem:(NSString *)key
{
    [self willAccessValueForKey:key];
    NSNumber *value = [self primitiveValueForKey:key];
    int intValue = value.intValue;
    [self didAccessValueForKey:key];

    [self willChangeValueForKey:key];
    [self setPrimitiveValue:[NSNumber numberWithInt: intValue++] forKey:key];
    [self didChangeValueForKey:key];

    [self recalculateScore];
}

-(void) recalculateScore
{
    NSString *key;
    NSNumber *current;
    int tally = 0;

    for (key in scoredKeys) {
        [self willAccessValueForKey:key];
        current = [self primitiveValueForKey:key];
        tally = tally + current.intValue;
        [self didAccessValueForKey:key];
    }

    [self willChangeValueForKey:@"score"];
    [self setPrimitiveValue:[NSNumber numberWithInt:tally] forKey:@"score"];
    [self didChangeValueForKey:@"score"];
}

I am also seeking general comments about my approach.

  1. In the core-data docs, it says the auto generated accessors are much faster than anything a developer can write, so is this a good idea?
  2. Should I not even have a score property, and just have a method that returns the tally?
share|improve this question
    
what do u mean with "Core Data class"? is this a subclass of NSManagedObject? –  vikingosegundo Jan 17 '13 at 16:41
    
@vikingosegundo yes -- edited question to reflect that. –  hvgotcodes Jan 17 '13 at 16:46
    
Than I'd like to know how you create new model objects, as it usually is done with initWithEntity:insertIntoManagedObjectContext: –  vikingosegundo Jan 17 '13 at 16:49
    
havent gotten into that, my question is specifically about the setters and the recalculation. However, I plan to create a service class as a factory that manages the entities and their interactions with the context. –  hvgotcodes Jan 17 '13 at 16:55

2 Answers 2

without referring to your code, I think there is one pattern, that makes core data more fun but sadly isnt supported by Xcode out-of-the-box:

Have two classes for each model: one for automatic code generation — it will be jsut changed by Xcode — and on that inherits from the first, that can be changed and will never touched again by the machine after creation.

// for the machine
@class _Event : NSManagedObject
@end

// for human beings
@class Event : _Event
// what ever you need: put it here
@end

a great tool to teach Xcode this pattern is mogenerator. mogenerator + Xcode4.x

Another similar approach would be to have categories on the Model class. But this is less powerful and needs more attention for importing.


Now to your code.

I dont see any reason to overwrite the setter. if apple says that you most likely will never be able to write as performant accessors as the provide, they probably are right. Instead you should create your own method, that will set up a object as need. you could overwrite save and similar methods, or access it via a wrapper class, that will set it up for you.

I plan to create a service class as a factory that manages the entities and their interactions with the context.

that sounds valid and could be the wrapper class mentioned above. But make sure, you create the model objects with initWithEntity:insertIntoManagedObjectContext:, as you want them in your context.


Core Data is a great and powerful framework. but beware: if you shoot yourself in the foot, most likely booth your legs will be gone.
2 Years ago I was working in a team where the Engineer responsible for persistency accidentally overwrote an internal method of NSManagedObject. It took him one week to find the cause and fix it by renaming his method. This line of code was probably the most expensive in the whole project. There-for another hint: prefix your custom methods.

share|improve this answer
    
ah, so basically proxy an underlying class that only xcode touches. hmmm. interesting. hey, where are the tutorials on patterns for ios? I see lots of basic info, but nothing on patterns and best practices.... also i realize my question makes no sense. I ask about smart setters, and have no actual settings in the code. I don't think I need the {did|will}AccessValueForKey and probably can just use the normal setters...is that correct? –  hvgotcodes Jan 17 '13 at 17:19
    
oh, and lol for the last sentence -- I'm gonna remember that. –  hvgotcodes Jan 17 '13 at 17:20
    
just use the setters provided by core data. everything else is for masochists. or are you masochistic? –  vikingosegundo Jan 17 '13 at 17:22
    
I added an eyewitness report –  vikingosegundo Jan 17 '13 at 17:23

I would refactor your code as follows:

@implementation CodeReviewClass

static NSArray *scoredKeys;

+ (void)initialize
{
    if (self == [CodeReviewClass class] ) {
        scoredKeys = @[@"fullSwings", @"putts", @"fairwayBunkers", @"greenBunkers", @"chipsOrPitches", @"rescue", @"wastedOrPenalty"];
    }
}

I asked a question about +[NSObject initialize] over on StackOverflow and got some great responses. The responses point out the canonical way to use initialize.

+ (id)codeReviewClassWithHoleNumber:(NSInteger)holeNumber insertInManagedObjectContext:(NSManagedObjectContext *)managedObjectContext
{
    CodeReviewClass *obj = [NSEntityDescription insertNewObjectForEntityForName:@"CodeReviewClass" inManagedObjectContext:managedObjectContext];
    if (obj) {
        [obj setHoleNumber:@(holeNumber)];
    }

    return obj;
}

I've seen this convenience constructor pattern more frequently with Core Data managed objects then the two-stage initialization used with NSObject. Particularly because the designated initializer (-[NSManagedObject initWithEntity: insertIntoManagedObjectContext:]) is pretty finicky. Definitely read NSManagedObject Class Reference - they give special treatment to overriding init. Obviously substitute your actual class name in for my placeholder text!

- (void)incrementItem:(NSString *)key
{
    NSInteger value = [[self valueForKey:key] integerValue];

    [self setValue:@(++value) forKey:key];

    [self recalculateScore];
}

Core Data properties are key-value coding compliant, and you don't have to use the will/did + Access/Change + ValueForKey: methods if you use the dynamically supplied accessor or mutator (KVC tries to use these methods).

-(void) recalculateScore
{
    NSNumber *current;
    int tally = 0;

    for (NSString *key in scoredKeys) {
        current = [self valueForKey:key];
        tally += current.integerValue;
    }

    [self setScore:@(tally)];
}

@end

Again, I opt to use the publicly exposed properties via KVC to calculate the tally. The rest of the changes are stylistic syntax.

Your score property could be marked as a transient property in the Core Data Managed Object Model, and calculated on-demand using the implementation of recalculateScore. This of course could get expensive if you calculate it frequently!

share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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