is it good form to release self in an init method when that method allocates and returns something else?

Question

In my code, I have something that looks like this:

@implementation MyClass

- (id) initWithType:(NSInteger)type {
  [self release];
  if (type == 0) {
    self = [[MyClassSubclass1 alloc] init];
  } else {
    self = [[MyClassSubclass2 alloc] init];
  }
  return self;
}
//...

@end

which I think handles any potential memory leaks. However, I have seen code out there that does something similar, except it doesn't release self before reassigning it to another newly allocated instance. Is it not necessary to release self here or is the other code I've seen incorrect?

Answer 1

This looks like poor use of object-oriented design.

If you're creating a different instance depending on a type variable, then why don't you have subclasses for those types?

It would be much cleaner to define a base class with all the common functionality, and a subclass for each "type" variation.

What does the class do? We might be able to point you in the right direction.

Code-wise, your example code is correct, but it's generally bad practice to replace the instance with a different instance. Unless the init method is a factory method re-using instances or a singleton initializer, avoid releasing self en-lieu of another instance.

Answer 2

Your code looks technically correct, from a memory management perspective. Replacing self with a different alloc'd object loses the pointer to the original object, and nobody else will be able to release it, which would cause a leak. Try commenting out the release call and run it with Leaks in Instruments.

Just be cautious about opening this particular can of worms — Foundation.framework (part of Cocoa) uses class clusters for collections and strings, but doing so is a fairly advanced concept. A better approach might be to have a class method for each subclass, using the AbstractFactory pattern.

In any case, determining the subclass type based on an integer is a bad idea — any change in mapping from type to class will break dependent code. If you're going that way, why not just pass in the class object itself?