mikeash.com pyblog/friday-qa-2010-08-27-defensive-programming-in-cocoa.html comments

mikeash - 2010-10-24 20:21:35

Sun, 24 Oct 2010 20:21:35 GMT

Thanks for the typo notice, I've fixed it now.

Regarding copyWithZone:, while you need to support the zone APIs when overriding in case someone decides to call them directly, I don't think it's important to actually respect the zone parameter. Zones are pretty much unused and useless these days. Certainly it would do no harm to use allocWithZone: in your implementation, but I don't think it's worth any bother.

DavidPhillipOster - 2010-10-24 04:26:08

Sun, 24 Oct 2010 04:26:08 GMT

Setters:

typo:
s/The superclass my call the setter/The superclass may call the setter/

Copy:

copyWithZone: shouldn't call alloc, as you've written, it should call allocWithZone: That's what allocWithZone: is for.

Steve Wart - 2010-09-12 13:36:18

Sun, 12 Sep 2010 13:36:18 GMT

There is an alternative solution to the NSCopyObject() problem that doesn't involve tricks with C pointers. The Smalltalk idiom for Object copy is to implement it as follows:



copy

   ^self shallowCopy postCopy

shallowCopy is a primitive that behaves essentially the same way as NSCopyObject(). Apple's documentation states that it will be deprecated at some point, but I think the alternatives are worse.

in Objective-C syntax the most abstract class might contain something like:



-(id)copyWithZone:(NSZone *)zone {

    // shallow copy

    id copy = NSCopyObject(self, 0, nil);

    [copy postCopy];

    return copy;

}

As a rule, subclasses should not overrride copy, but they could implement postCopy to maintain reference counting semantics:



-(void)postCopy {

    [super postCopy];

    [someString retain];

}

Sean McB - 2010-09-12 01:10:06

Sun, 12 Sep 2010 01:10:06 GMT

Mike: well, it wouldn't be the first time Apple's docs were wrong. :)

Personally, I trust Greg Parker's advice (in the link I posted above) more than the docs. Of course, it doesn't contradict your advice to 'always write your dealloc implementation to tolerate an uninitialized object', one should do that _also_.

Anyway, this is another nice thing about GC... no release/dealloc anyway. :)

mikeash - 2010-09-04 02:52:16

Sat, 04 Sep 2010 02:52:16 GMT

Sean McB: While the idea that you should call [super dealloc] directly has been in vogue lately, it's contradicted by the documentation:

Subclass implementations of this method should initialize and return the new object. If it can’t be initialized, they should release the object and return nil.

(From the docs for -[NSObject init].)

I mention that this is why you should always write your dealloc implementation to tolerate an uninitialized object. I would go so far as to say that not doing so would be a bug in the subclass.

Sean McB - 2010-09-04 02:16:06

Sat, 04 Sep 2010 02:16:06 GMT

Patrick Stein -> when something fails in init, you should in fact call [super dealloc].

"[self dealloc] or [self release] are bad because they might call some subclass's -dealloc method even though the subclass's -init hasn't done anything yet."[1] Remember method dispatch is always dynamic in Obj-C.

[1] http://lists.apple.com/archives/objc-language/2008/Sep/msg00133.html

Jim Rodovich - 2010-08-28 19:41:44

Sat, 28 Aug 2010 19:41:44 GMT

Anonymous: Good point that I hadn't considered. The lack of a relevant stack is probably on its own a good enough reason not to use autorelease as I proposed, unless you're specifically concerned about waiting too long for release to return or about the stack exploding due to an extremely long release-dealloc-set:nil chain reaction. (I still have linked lists on my mind.)

mikeash - 2010-08-28 17:45:16

Sat, 28 Aug 2010 17:45:16 GMT

Jim Rodovich: Thread safety is a binary property: either something is thread safe, or it's not. None of the setters I showed, nor either of the ones you show, are thread safe. Yours narrow the window for the race condition, but there is still the possibility for a second thread to grab a reference to the object, and then for the first thread to destroy it before the second thread can retain it. When it comes to threaded programming, narrowing the window for a race condition is usually a bad idea, because it makes problems much harder to track down and debug. Better to crash early and often if the code is busted.

It is possible to do a lockless thread-safe setter but it is an enormous amount of work, something like 100-200 lines of code last time I tried it. Basically, if you need thread-safe setters (and that's usually the wrong place for thread safety, but not always) then use a lock.

Anonymous - 2010-08-28 15:57:53

Sat, 28 Aug 2010 15:57:53 GMT

Jim Rodovich: I've always done the latter, using autorelease. However, using autorelease technically can make programs harder to debug since if there is a crash during the actual release, your stack trace with the autorelease will be nowhere to be found.

Jim Rodovich - 2010-08-28 14:48:14

Sat, 28 Aug 2010 14:48:14 GMT

Regarding memory management in setters, obj can be freed even if it's unequal to the released ivar, if the ivar holds the only reference to obj and releases it during dealloc. (For example, suppose you're implementing a linked list.) The second pattern you propose is safer: always retain before releasing.

With both patterns, the ivar is temporarily assigned to a value which could have been freed, so I'm not sure that either is thread-safe. (I may be wrong -- I'll admit I was releasing first before I saw this post.) Wouldn't the following be safer?

NSObject * temp = myObjIvar;

myObjIvar = [obj retain];

[temp release];

Or, more elegantly,

[myObjIvar autorelease];

myObjIvar = [obj retain];

Anonymous - 2010-08-28 12:32:03

Sat, 28 Aug 2010 12:32:03 GMT

Mike Shields: Even better would be if Apple implemented Mike Ash's suggestion that Apple implements -hash on NSObject returning 0 at the very least...

Granted it would be more likely if Mike Ash submitted this as an actual bug report to Apple (I would but it's just too early in the morning).

Anonymous - 2010-08-28 12:24:23

Sat, 28 Aug 2010 12:24:23 GMT

Scott Gould: I think what Mike means is that Apple can (and does) add private methods without using the underscore prefix. Thus they are still just as tricky to catch as ones with underscores.

Anyone: Related: I've always wondered if it would be fine to use two or maybe even three underscores as a method prefix rather than initials or something. Granted this would only work if by convention it was only used in application code and never framework code, so that nobody had the monopoly on multi-underscores thus disabling anyone else from using them when adding a framework to their app.

Dave DeLong: Oh that's even uglier (IMHO). It comes right before the argument! And what happens when your method has multiple arguments?

Karan: Do you mean checking if an ivar of the newly copied object is nil, or checking if the newly copied object itself is nil? If the latter, I agree that it would be safer to do that, but I can't think of a single time where -copyWithZone: has failed or would ever fail or was documented to fail, nor have I ever seen an implementation where it would fail and return nil. But yes, technically it should be safer.

Patrick Stein: As a style, I personally find that one too repetitive to use, because you return several times when you really only need to return twice (self at the end of the method, or nil after an error).

But on the note of different coding styles for -init, if only you could use "return self" or "break" as the last clause of a for() loop, you could do something like this:

- (id) init {

    for (self = [super init]; self; break) {

        /* ... */

        if (something_goes_wrong) {

            [self release], self = nil;

            break;

        }

        /* ... */

    }

    return self;

}

Mike Shields - 2010-08-28 04:07:46

Sat, 28 Aug 2010 04:07:46 GMT

One other thing I'd add to this list is being dang sure that you think about implementing -(void)hash whenever you override -(BOOL)isEqual:. I've been bitten enough times when I implement the second without the first and then stuff an object into a container like NSSet, only to have things not work correctly.

In fact, I'd say that you should ALWAYS implement -hash, if only to make sure that you know you've thought of the implications.

Karan - 2010-08-27 19:57:52

Fri, 27 Aug 2010 19:57:52 GMT

I would also recommend adding a nil check for the result from [super copyWithZone:...] since you're doing a C-style pointer dereference on the result and there's no guarantee the result won't be nil, thus causing a crash.

Scott Gould - 2010-08-27 19:14:36

Fri, 27 Aug 2010 19:14:36 GMT

mikeash: Using an underscore prefix is at the very least no more dangerous than not using a prefix at all.

I would think colliding with a public Apple method would be preferable to colliding with a private one. In the case of colliding with an underscore prefix method, you're guaranteed to hit a private method which will be trickier to catch (I realize I might be missing something here).

Suffixes would seem to solve this problem as Dave DeLong mentioned above. Although using suffixes when a method takes an argument (or multiple arguments in particular) looks very ugly in Objective-C to my eyes.

mikeash - 2010-08-27 18:39:31

Fri, 27 Aug 2010 18:39:31 GMT

Patrick Stein: Thanks for the note about the broken link, that was just a silly typo. Fixed now.

Rob Wills: I've always found that warning to be a bit strange. Using an underscore prefix is at the very least no more dangerous than not using a prefix at all. Apple can and does add un-prefixed methods to their classes, so unprefixed names can clash too. Adding an underscore at the very least does not increase the probability of breaking something in the future.

Rob Wills - 2010-08-27 17:02:40

Fri, 27 Aug 2010 17:02:40 GMT

Another defensive tip for Cocoa is avoiding underscore prefixes.. Apple's documentation claims that it has reserved the use of the leading underscore:

Avoid the use of the underscore character as a prefix meaning private, especially in methods. Apple reserves the use of this convention. Use by third parties could result in name-space collisions; they might unwittingly override an existing private method with one of their own, with disastrous consequences.

(http://developer.apple.com/iphone/library/documentation/Cocoa/Conceptual/CodingGuidelines/CodingGuidelines.pdf page 11)

Patrick Stein - 2010-08-27 16:40:48

Fri, 27 Aug 2010 16:40:48 GMT

Two things:
1st:

I use the

- (id)init
{
    if(! (self=[super init]) )
    {
        return nil;
    }

    // put your code herebut remember

    if( somethingfails )
    {
        [self release];
        return nil;
    }

    return self;
}

this makes it easier to understand in my opinion as you don't have a mile long open/close bracket if your init routine is long. You don't have to keep in mind that open bracket the whole time.

Furthermore remind people to use [self release] in the init method once they have self filled with something but can't initialize the object anyways.

2nd: the link to http://www.mikeash.com/pyblog/the-how-and-why-of-cocoa-initializers.html is broken as it contains a URI which is too long.

Dave DeLong - 2010-08-27 16:20:19

Fri, 27 Aug 2010 16:20:19 GMT

Regarding category name collision:

I personally prefer to use a method suffix instead of a method prefix. That way I can still name the method something logical and find it in the autocompletion list, without having to remember that it's a category method. So instead of doing:

- (void) dd_map:(id (^)(id obj))block;

I do:

- (void) map_dd:(id(^)(id obj))block;

Scott Gould - 2010-08-27 16:20:15

Fri, 27 Aug 2010 16:20:15 GMT

Thanks so much for the post!

There is a lot of practical wisdom here. I know the future me really appreciates all of the debugging time and anguish you have likely saved him. Enjoy your time off.