Confused about memory management in an array

[benoitr007]

Hello,

I am working on this game where I have this NSMutableArray that contains all the enemies objects on the screen. This array is created in my game scene init method like this:

enemiesArray = [[NSMutableArray alloc] init];

enemiesArray is also a property of the game scene object:

@property (nonatomic, assign) NSMutableArray *enemiesArray;

Everything looks OK so far? Now this is how I add enemies to this array (in a method from this same game scene class):

[enemiesArray addObject:[[enemy alloc] initAtX:-1 y:-1]];

(initAtX:y: is a method that I made for the enemy class)
Is that code okay for memory management? When I need to kill an enemy I simply remove it from the array and it seems to be deallocated correctly.

However I had a bug happen 2 times (out of about 100-200 tests) in which all the enemies on the screen suddenly disappeared, as if they were being automatically released for some reason. I'm not able to reproduce it, it happened really randomly both times. I was wondering if anything was wrong with the way I initialize this stuff. Thank you!

[BrianSlick]

See the properties link in my signature for more information. You aren't quite doing that correctly.

Edit for clarification: I suppose in an init method that's an ok way to initialize your array property. But after that, you want to use the accessor methods instead of the instance variable.

Apply the same concept to the way you add things to the array. You are leaking each time you add an enemy.

[benoitr007]

Wow, I had no idea. Thanks, that really helped. So I should write:

NSMutableArray *myArray = [[NSMutableArray alloc] init];
enemiesArray = myArray;
[myArray release];

Is that correct? I'm not sure why you said my first way was okay for an init method. According to your tutorial, won't that mess up the retain count and put it to 2 instead of 1 (doesn't it retain the new value, which is already to 1 because it's been alloc'd)?

EDIT: OK no, wait. Instance variable != instance property. So it IS okay to write enemiesArray = [[NSMutableArray alloc] init], but it's NOT okay to write [self setEnemiesArray:[[NSMutableArray alloc] init]], RIGHT? lol

And for the enemies part:

enemy *myEnemy = [[enemy alloc] initAtX:-1 y:-1];
[enemiesArray addObject:myEnemy];
[myEnemy release];

[BrianSlick]

Originally Posted by benoitr007

Wow, I had no idea. Thanks, that really helped. So I should write:

NSMutableArray *myArray = [[NSMutableArray alloc] init];
enemiesArray = myArray;
[myArray release];

Close:
[self setEnemiesArray: myArray];

Originally Posted by benoitr007

Is that correct? I'm not sure why you said my first way was okay for an init method. According to your tutorial, won't that mess up the retain count and put it to 2 instead of 1 (doesn't it retain the new value, which is already to 1 because it's been alloc'd)?

There is a growing school of thought that says NOT to use accessor methods in initializers or dealloc. So you don't have much choice but to use the instance variable directly. Thus, as previously written, you're fine. Just make sure to use the accessors any other time you access that array.

You have a couple options. If you go back to my properties page, down to the colors post, there is an example of a custom getter that auto-creates the array if necessary. Or, you could move the array creation someplace else like viewDidLoad, and you can create your array using the 3-line pattern.

Originally Posted by benoitr007

And for the enemies part:

enemy *myEnemy = [[enemy alloc] initAtX:-1 y:-1];
[enemiesArray addObject:myEnemy];
[myEnemy release];

General idea, yeah. It's probably harmless in this case, but I'm a real stickler for using the accessors. This would change your second line to:
[[self enemiesArray] addObject: myEnemy];

Also, by common convention, your class name should be capitalized: Enemy.

[BrianSlick]

Originally Posted by benoitr007

EDIT: OK no, wait. Instance variable != instance property. So it IS okay to write enemiesArray = [[NSMutableArray alloc] init], but it's NOT okay to write [self setEnemiesArray:[[NSMutableArray alloc] init]], RIGHT? lol

You got it. Just be careful with that first one because you risk a leak if not handled properly down the line. That's why it is ok in init, since you won't run that code again. But if you did the same thing in, say, viewWillAppear, you will leak the old array each time. The second is a leak from the get-go.

[benoitr007]

Thank you so much for the really nice replies BrianSlick!

It's probably harmless in this case, but I'm a real stickler for using the accessors. This would change your second line to:
[[self enemiesArray] addObject: myEnemy];

Hmm... If I may ask, what's the advantage of using the getter accessor? I can see for the setter one, but doesn't the getter do exactly the same as just writing enemiesArray? In what cases could it not be harmless to do so?

[BrianSlick]

Originally Posted by benoitr007

Hmm... If I may ask, what's the advantage of using the getter accessor? I can see for the setter one, but doesn't the getter do exactly the same as just writing enemiesArray? In what cases could it not be harmless to do so?

Yeah, that's what I mean by harmless. Since you're just reading, there probably isn't any difference between using the standard getter and reading the instance variable. From a consistency standpoint, I make it a point to use self everywhere.

However, if you would have a reason to write your own custom getter like the one I mentioned, then there would be a difference, and I imagine that would be a tough bug to track down.

Using [self ...] helps to reinforce that you are using a method. If the implementation of that method would change, that shouldn't affect the code you've already written.

[benoitr007]

Ok, I get it. I have another question:

I have this huge array that contains all the different layers of enemies and objects in the game. Each layer of enemies/objects is an array containing specific objects. Right now I'm initializing like that, is that fine?

objectsArrays = [[NSMutableArray alloc] init];

enemiesLayer1 = [[NSMutableArray alloc] init];
[objectsArrays addObject:enemiesLayer1];
[enemiesLayer1 release];

enemiesLayer2 = [[NSMutableArray alloc] init];
[objectsArrays addObject:enemiesLayer2];
[enemiesLayer2 release];

objectsLayer1 = [[NSMutableArray alloc] init];
[objectsArrays addObject:objectsLayer1];
[objectsLayer1 release];

objectsLayer2 = [[NSMutableArray alloc] init];
[objectsArrays addObject:objectsLayer2];
[objectsLayer2 release];

I'm doing it like that to then parse the objectsArrays array to draw every object in order, like this:

NSMutableArray *currentArray;
for (int i = 0; i < [objectsArrays count]; i ++) {
	currentArray = [objectsArrays objectAtIndex:i];
	for (int n = [currentArray count] - 1; n >= 0; n -= 1) {
		FallingObject *currentObject = [currentArray objectAtIndex:n];
		[currentObject render];
	}
}

[BrianSlick]

Originally Posted by benoitr007

Ok, I get it. I have another question:

I have this huge array that contains all the different layers of enemies and objects in the game. Each layer of enemies/objects is an array containing specific objects. Right now I'm initializing like that, is that fine?

objectsArrays = [[NSMutableArray alloc] init];

enemiesLayer1 = [[NSMutableArray alloc] init];
[objectsArrays addObject:enemiesLayer1];
[enemiesLayer1 release];

enemiesLayer2 = [[NSMutableArray alloc] init];
[objectsArrays addObject:enemiesLayer2];
[enemiesLayer2 release];

objectsLayer1 = [[NSMutableArray alloc] init];
[objectsArrays addObject:objectsLayer1];
[objectsLayer1 release];

objectsLayer2 = [[NSMutableArray alloc] init];
[objectsArrays addObject:objectsLayer2];
[objectsLayer2 release];

I would probably make a @property for objectsArrays and do the stuff previously mentioned, but otherwise it's fine.

Originally Posted by benoitr007

I'm doing it like that to then parse the objectsArrays array to draw every object in order, like this:

NSMutableArray *currentArray;
for (int i = 0; i < [objectsArrays count]; i ++) {
	currentArray = [objectsArrays objectAtIndex:i];
	for (int n = [currentArray count] - 1; n >= 0; n -= 1) {
		FallingObject *currentObject = [currentArray objectAtIndex:n];
		[currentObject render];
	}
}

Looks fine at quick glance. Nothing jumping out at me. There is a cool NSArray method that might simply that a bit: makeObjectsPerformSelector:

[benoitr007]

I would probably make a @property for objectsArrays and do the stuff previously mentioned, but otherwise it's fine.

Sorry, what do you mean? All these arrays are already properties, but this is in the init method, so we said it was okay that way, didn't we? And just by curiosity, why do you recommend making them properties for that short piece of code? Do you usually make all of your instance variables properties?

Looks fine at quick glance. Nothing jumping out at me. There is a cool NSArray method that might simply that a bit: makeObjectsPerformSelector:

Oh, thank you! Very useful indeed.

Finally, another quick question: using the objectAtIndex: method for an array doesn't copy it or do anything to the retain count? So it's perfectly fine to do currentArray = [objectsArrays objectAtIndex:i]; every step?

[BrianSlick]

Originally Posted by benoitr007

Sorry, what do you mean? All these arrays are already properties, but this is in the init method, so we said it was okay that way, didn't we? And just by curiosity, why do you recommend making them properties for that short piece of code? Do you usually make all of your instance variables properties?

Oh, I didn't realize we were still talking about the init method. Sorry.

Is there a reason that you make instance variables for the enemiesLayer1, 2, etc? If you are releasing them right away, then those instance variables don't seem to be necessary. They could be local variables. If you intend to access them again later (not via the main array, I mean), then don't release them.

And yes, I do make properties for all of my instance variables. Using instance variables directly is just begging for memory problems.

Originally Posted by benoitr007

Finally, another quick question: using the objectAtIndex: method for an array doesn't copy it or do anything to the retain count? So it's perfectly fine to do currentArray = [objectsArrays objectAtIndex:i]; every step?

Correct.

[benoitr007]

Is there a reason that you make instance variables for the enemiesLayer1, 2, etc? If you are releasing them right away, then those instance variables don't seem to be necessary. They could be local variables. If you intend to access them again later (not via the main array, I mean), then don't release them.

Well, [objectsArrays addObject:enemiesLayer1]; increases the retain count of enemiesLayer1, thus not deallocating it when it's released, right? Because yes, I do access these arrays layer, when I add enemies to the game, like this:

Enemy *myEnemy = [[Enemy alloc] initAtX:-1 y:-1];
[enemiesLayer1 addObject:myEnemy];
[myEnemy release];

Thanks again.

[BrianSlick]

Ok, I see what you're doing. Each of those releases should be in dealloc instead.

The issue with your approach is that the objectsArrays is the only thing keeping the various sub-arrays alive. That may not be a concern if objectsArrays lives for the duration of your program, but if for any reason it would go away later, it will decrement the retain count of each sub-array. At that point, they will be deallocated, but each of those instance variables is still pointing at them. That will be bad.

It may not be a problem in this case, but I try to make my practices such that they work across all of my projects. In this case, doing the same technique under different circumstances could easily result in problems.

My basic guideline is pretty simple: if you need to be able to talk to an object (or primitive) across multiple methods, then there should be a @property for doing so. If you need/want to stuff those into a parent collection like objectsArrays, that's fine, but let the @property take care of itself, and let the collection take care of itself. You're kinda crossing the streams a bit here with the instance variables being completely dependent on the objectsArrays.

[benoitr007]

Brian, I understand what you're saying. I've moved the sub-arrays' releases into the dealloc method, after the objectsArrays array is released.

[benoitr007]

And yes, I do make properties for all of my instance variables. Using instance variables directly is just begging for memory problems.

Even for non-object instance variables?

[BrianSlick]

Originally Posted by benoitr007

Even for non-object instance variables?

Memory problems, no. Do I use them, yes.

My goal is consistency. I don't want to be in a mode of "well, this IS an object, so I'll use the accessor method, but this ISN'T an object, so I won't." Too easy to screw up, one way or another. By using @properties for everything, you do the same thing each time regardless of type. And if your int suddenly becomes an NSNumber, the impact of that change is minimized.

It also helps to distinguish between local variables and instance variables. I don't use instance variables, so everything global will be a [self whatever]. If I see someVariableName, I know instantly that it is local.