views:

88

answers:

2

I have an object that I alloc/init like normal just to get a instance. Later in my application I want to load state from disk for that object. I figure I could unarchive my class (which conforms to NSCoding) and just swap where my instance points to. To this end I use this code...

NSString* pathForDataFile = [self pathForDataFile];
if([[NSFileManager defaultManager] fileExistsAtPath:pathForDataFile] == YES)
{
    NSLog(@"Save file exists");
    NSData *data = [[NSMutableData alloc] initWithContentsOfFile:pathForDataFile];
    NSKeyedUnarchiver *unarchiver = [[NSKeyedUnarchiver alloc] initForReadingWithData:data];
    [data release];

    Person *tempPerson = [unarchiver decodeObjectForKey:@"Person"];
    [unarchiver finishDecoding];
    [unarchiver release];   

    if (tempPerson)
    {
        [self release];
        self = [tempPerson retain];
    }
}

Now when I sprinkled some NSLogs throughout my application I noticed

self.person: <Person: 0x3d01a10> (After I create the object with alloc/init)
self: <Person: 0x3d01a10> (At the start of this method)
tempPerson: <Person: 0x3b1b880> (When I create the tempPerson)
self: <Person: 0x3b1b880> (after i point self to the location of the tempPerson)
self.person: <Person: 0x3d01a10> (After the method back in the main program)

What am I missing?

+6  A: 

Don't do this. Besides that it breaks identity rules, you can't change the pointer values other parts of a program hold.

A better approach would be to use the PIMPL idiom: your class holds a pointer to an implementation object and you only swap that.

E.g. something along the lines of this:

@class FooImpl;
@interface Foo {
    FooImpl* impl;
}
// ...
- (void)load;
@end

@implementation Foo
- (void)load {
    FooImpl* tmp = loadFromDisk();
    if (tmp) {
        FooImpl* old = impl;
        impl = tmp;
        [old release];
    }
}
@end
Georg Fritzsche
Thanks! I'll give this a shot.
rob5408
+5  A: 

self is a function argument to instance methods. Assigning to self is perfectly reasonable, just like assigning values to other function arguments is perfectly reasonable. Since the scope of self is the current function, your code leaks one object and releases another in a way that will most likely cause a crash.

The only time it is meaningful to assign to self is in an init method. Although it is almost never used, init methods are allowed to release self and allocate a new object to return or just return nil. The only reason this works is because the return value is self and callers of init expect to use the return value.

As gf pointed out, the correct approach is a load function that assigns new values to the members of your instance, not that tries to replace the instance.

drawnonward
Both excellent answers. This one gives me a bit more background as to why my approach was so wrong, thanks!
rob5408