objective-c - 这是一个好的（类似可可，苹果认可的）模型类吗？

Question

我使用 Objective-C 已经有一段时间了，但我并没有很好地遵循 Apple 的指导方针。最近我阅读了Cocoa Design Patterns和Model Object Implementation Guide，我正在尝试做一些非常简单的事情，但做得很好。

我错过了任何主要概念吗？请不要提及self = [super init]；这已经在 SO 上讨论过很多次了。随意批评我#pragma mark的虽然！

#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

（编辑：到目前为止，我已经做出了建议的更改并评论了哪些答案讨论了它们，以防有人需要知道原因。）

score 5 · Accepted Answer

Please don't mention self = [super init]…</p>

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.

score 2 · Accepted Answer

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.

score 2 · Accepted Answer

//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´).

另外，对这个类的角色没有明确的概念，countOfTileTemplates对我来说听起来不太好。也许只是“计数”就可以了，如果这个类用来保存图块模板是明确的吗？

score 0 · Accepted Answer

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

objective-c - 这是一个好的（类似可可，苹果认可的）模型类吗？

4 回答 4

Related

Reference