Is this a good (Cocoa-like, Apple-approved) model class?

Question

I've been using Objective-C for a while, but I've not been following Apple's guidelines very well. Recently I read Cocoa Design Patterns and the Model Object Implementation Guide, and I'm trying to do some very simple things, but do them very well.

Have I missed any major concepts? Please don't mention self = [super init]; that's been covered many times on SO already. Feel free to critique my #pragma marks though!

#import "IRTileset.h"
#import "IRTileTemplate.h"

@interface IRTileset () //No longer lists protocols because of Felixyz

@property (retain) NSMutableArray* tileTemplates; //Added because of TechZen

@end

#pragma mark -
@implementation IRTileset

#pragma mark -
#pragma mark Initialization

- (IRTileset*)init
{
    if (![super init])
    {
        return nil;
    }

    tileTemplates = [NSMutableArray new];

    return self;
}

- (void)dealloc
{
    [tileTemplates release];
    [uniqueID release]; //Added because of Felixyz (and because OOPS. Gosh.)
    [super dealloc]; //Moved from beginning to end because of Abizern
}

#pragma mark -
#pragma mark Copying/Archiving

- (IRTileset*)copyWithZone:(NSZone*)zone
{
    IRTileset* copy = [IRTileset new];
    [copy setTileTemplates:tileTemplates]; //No longer insertTileTemplates: because of Peter Hosey
    [copy setUniqueID:uniqueID];

    return copy; //No longer [copy autorelease] because of Jared P
}

- (void)encodeWithCoder:(NSCoder*)encoder
{
    [encoder encodeObject:uniqueID forKey:@"uniqueID"];
    [encoder encodeObject:tileTemplates forKey:@"tileTemplates"];
}

- (IRTileset*)initWithCoder:(NSCoder*)decoder
{
    [self init];

    [self setUniqueID:[decoder decodeObjectForKey:@"uniqueID"]];
    [self setTileTemplates:[decoder decodeObjectForKey:@"tileTemplates"]]; //No longer insertTileTemplates: because of Peter Hosey

    return self;
}

#pragma mark -
#pragma mark Public Accessors

@synthesize uniqueID;
@synthesize tileTemplates;

- (NSUInteger)countOfTileTemplates
{
    return [tileTemplates count];
}

- (void)insertTileTemplates:(NSArray*)someTileTemplates atIndexes:(NSIndexSet*)indexes
{
    [tileTemplates insertObjects:someTileTemplates atIndexes:indexes];
}

- (void)removeTileTemplatesAtIndexes:(NSIndexSet*)indexes
{
    [tileTemplates removeObjectsAtIndexes:indexes];
}

//These are for later.
#pragma mark -
#pragma mark Private Accessors

#pragma mark -
#pragma mark Other

@end

(Edit: I've made the changes suggested so far and commented which answers discuss them, in case anyone needs to know why.)

Answer 1

two minor nitpicks: One is the init method, (where stylistically I'm against having 2 different return points but thats just me), however there's nothing stopping super's init from returning a different object than itself or nil, eg a different object of its class or even just another object altogether. For this reason, self = [super init] is generally a good idea, even if it probably won't do much in practice. Second is in the copyWithZone method, you don't copy the tileTemplates, which could be intentional but is generally a bad idea (unless they're immutable). Copying an object is supposed to have the same effect as allocing a fresh one, eg. retain count of 1, so don't autorelease it. Also, it doesn't look like you do anything with the zone, so you should probably replace it with something like

- (IRTileTemplate*)copyWithZone:(NSZone*)zone {
    IRTileset* copy = [[IRTileset allocWithZone:zone] init];
    [copy insertTileTemplates:[tileTemplates copyWithZone:zone]
                    atIndexes:[NSIndexSet indexSetWithIndex:0]];
    [copy setUniqueID:uniqueID];
    return copy;
}

thats everything I found; with the exception of the retain count of copy (which WILL lead to bugs later on) mostly just stuff that I prefer, you can do it your way if you like it better. Good work

Answer 2

It looks pretty good except you've left your properties open to arbitrary manipulation by external objects. Ideally, the data should be manipulated directly only by the model class itself and external objects should have access only via dedicated methods.

For example what if some external code calls this:

myIRTileset.tileTemplates=someArray;

Boom, you've lost all your data.

You should define both the data properties as readonly. Then write accessors internal to the class that will managed their retention within the class implementation. This way the only way for an external object to change the tileTemplates is by calling the - insertTileTemplates:atIndexes: and removeTileTemplatesAtIndexes: methods.

Edit01:

I think I mangled it the first go, so let me try again. You should setup you data model class in the following pattern:

Interface

@interface PrivateTest : NSObject {
@private 
    //iVar is invisible outside the class, even its subclasses
    NSString *privateString; 
@public
    //iVar is visible and settable to every object. 
    NSString *publicString; 
}
@property(nonatomic, retain)  NSString *publicString; //property accessors are visible, settable and getable. 
//These methods control logical operations on the private iVar.
- (void) setPrivateToPublic;  
- (NSString *) returnPrivateString;
@end

So in use it would look like:

Implementation

#import "PrivateTest.h"

//private class extension category defines 
// the propert setters and getters 
// internal to the class
@interface PrivateTest ()
@property(nonatomic, retain)  NSString *privateString;
@end

@implementation PrivateTest
//normal synthesize directives
@synthesize privateString; 
@synthesize publicString;

// Methods that control access to private
- (void) setPrivateToPublic{
    //Here we do a contrived validation test 
    if (self.privateString != nil) {
        self.privateString=self.publicString;
    }
}

- (NSString *) returnPrivateString{
    return self.privateString;
}

@end

You would use it like so:

PrivateTest *pt=[[PrivateTest alloc] init];
    // If you try to set private directly as in the next line
    // the complier throws and error
//pt.privateString=@"Bob"; ==> "object cannot be set - either readonly property or no setter found"
pt.publicString=@"Steve";
[pt setPrivateToPublic];
NSLog(@"private=%@",[pt returnPrivateString]); //==> "Steve"

Now the class has bullet proof data integrity. Any object in your app can set and get the publicString string property but no external object can set or get the private.

This means you can safely allow access to the instance by any object in your app without worrying that a careless line of code in some minor object or method will trash everything.

Answer 3

Please don't mention self = [super init]…

So, why aren't you doing that?

The same goes for initWithCoder:: You should be using the object returned by [self init], not assuming that it initialized the initial object.

- (void)dealloc
{
    [super dealloc];
    [tileTemplates release];
}

As Abizern said in his comment, [super dealloc] should come last. Otherwise, you're accessing an instance variable of a deallocated object.

- (IRTileTemplate*)copyWithZone:(NSZone*)zone

The return type here should be id, matching the return type declared by the NSCopying protocol.

{
    IRTileset* copy = [IRTileset new];
    [copy insertTileTemplates:tileTemplates atIndexes:[NSIndexSet indexSetWithIndex:0]];
    [copy setUniqueID:uniqueID];

You're inserting zero or more objects at one index. Create your index set with a range: location = 0, length = the count of the tileTemplates array. Better yet, just assign to the whole property value:

copy.tileTemplates = self.tileTemplates;

Or access the instance variables directly:

copy->tileTemplates = [tileTemplates copy];

(Note that you must do the copy yourself when bypassing property accessors, and that you are copying the array on behalf of the copy.)

    return [copy autorelease];
}

copyWithZone: should not return an autoreleased object. According to the memory management rules, the caller of copy or copyWithZone: owns the copy, which means it is the caller's job to release it, not copyWithZone:'s.

@synthesize tileTemplates;
[et al]

You may want to implement the single-object array accessors as well:

- (void) insertObjectInTileTemplates:(IRTileTemplate *)template atIndex:(NSUInteger)idx;
- (void) removeObjectFromTileTemplatesAtIndex:(NSUInteger)idx;

This is optional, of course.

Answer 4

//However, should I list protocols here, even though they're already listed in IRTileset.h?

No, you shouldn't. The class extension declared in the implementation file is an extension, so you don't have to care about which protocols the class has been declared to follow.

I would recommend to mark your instance variables' names with an underscore: _tileTemplates. (Purists will tell you to affix rather than prefix the underscore; do that if you're afraid of them.)

Don't use new to instantiate classes. It's not recommended ever, as far as I understand.

[NSMutableArray new];                     //  :(
[NSMutableArray arrayWithCapacity:20];    //  :)

Don't call [super dealloc] before doing your own deallocation! This can cause a crash in certain circumstances.

- (void)dealloc
{
    [tileTemplates release];
    [super dealloc];          // Do this last
}

I'm not sure what type uniqueID has, but shouldn't it also be released in dealloc?

I would never put my @synthesize directives in the middle of a file (place them immediately below ´@implementation´).

Also, having no clear idea about the role of this class, countOfTileTemplates doesn't sound good to me. Maybe just ´count´ will do, if it's unambiguous that what this class does it to hold tile templates?