Objective-c - NSMutableString setString vs NSString

Question

Is it a better practice, in a setter, to retain and release NSString as follows :

-(void) setName:(NSString *)newName
{
    if(newName != nil)
    {
         [newName retain]:
         [m_Name release];
         m_Name = newName; //Where m_Name is a NSString *
    }
    //I'm not sure for this code, I have difficulties understanding memory-management in ObjC
}

Or to change the value via a NSMutableString :

-(void) setName:(NSString *)newName
{
    if(newName != nil)
        [m_Name setString:newName]; //Where m_Name is a NSMutableString *
}

If any or both of the methods are incorrect(s), let me know.

Answer 1

A couple of thoughts:

1. Best practice is to not write setters at all, to take advantage of the automatically synthesized accessor methods). Writing your own is only an opportunity to mess up your memory management or introduce a bug. You should have a compelling need for a custom setter before you write one.

2. The emerging convention for instance variable names is to use the property name preceded by a leading underscore (e.g., for a property called name, the ivar is _name). If you omit the @synthesize statement, the compiler included with recent versions of Xcode will do this automatically for you.

3. The question of what the setter should be makes no sense in the absence of your stating what memory qualifiers your property had. I'm going to assume you defined your property as retain.

4. Changing the property to a NSMutableString changes the behavior of your property and I would not advise that unless you really needed a mutable string for some reason.

5. Your first example does nothing if someone sets the name property to nil. But if someone wants to set it to nil, you should still (a) release the old name value; and (b) set your ivar to nil. (By the way, my code below takes advantage of the fact that sending a message to a nil object does nothing, so I don't need to check if it's nil or not, in this case.)

So, I assuming you have a property defined as follows:

@property (nonatomic, retain) NSString *name;

and a synthesize line that is either omitted or looks like:

@synthesize name = _name;

Then, I think the setter would look like:

-(void) setName:(NSString *)name
{
    // if you want to program defensively, you might want the following assert statement:
    //
    // NSAssert(name == nil || [name isKindOfClass:[NSString class]], @"%s: name is not string", __FUNCTION__);

    if (name != _name)
    {
        [_name release];
        _name = name;
        [_name retain];
    }
}

By the way, I assume your init method properly initializes _name and that dealloc releases it.

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

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

---

As bblum points out, it's prudent to use copy for your NSString properties:

@property (nonatomic, copy) NSString *name;

Then, I think the setter would look like:

-(void) setName:(NSString *)name
{
    // if you want to program defensively, you might want the following assert statement:
    //
    // NSAssert(name == nil || [name isKindOfClass:[NSString class]], @"%s: name is not string", __FUNCTION__);

    if (name != _name)
    {
        [_name release];
        _name = [name copy];
    }
}

But really, you simply shouldn't be writing setters unless you absolutely need to.

---

Finally, your code has a comment about finding memory management confusing. While you definitely need to understand it, I'd make two final suggestions:

1. Consider using Automatic Reference Counting (ARC). While this doesn't obviate the need to understand how memory management works (see the Advanced Memory Management Programming Guide), it does make it easier to write code that handles memory management properly. If you write manual reference counting (MRC) code, it's very easy to make simple memory management mistakes that ARC would otherwise take care of for you.

2. Especially if you're going to use MRC, avail yourself of Xcode's static analyzer ("Analyze" on the "Product" menu or press shift+command+B). This can facilitate finding many routine memory management issues that plague MRC code. The Finding Memory Leaks section of the Instruments User Guide also illustrates how to find leaks while debugging your code, but often the static analyzer can identify issues just by examining your code.

Answer 2

The first solution is better. This is how retain properties are handled. You retain the new value and then release the old value. Also if the nil case is not crucial to be handled, you can depend on the default implementation, generated by @synthesize.

For the second solution, it's really unnecessary, and it's a little bit against convention. Also, in this solution, you have to initialize m_name before ever assigning any string to it. You can do that in init.

- (void) init {
    if (self = [super init]) {
        m_name = [[NSMutableString alloc] init];
    }
}

Conclusion: First solution is definitely better.