NSMutable Array - objectAtIndex: index beyond bounds

Question

I'm trying to look through a NSMutableDictionary loaded with an NSMutableArray and I'm messing it up, and I don't know how. I'm trying to load a larger plist of game questions, then delete them if they aren't the right level. I don't get an error until I try and delete. Can anyone see the flaw in this code? I'd super appreciate it!

Thanks,

~G

{
NSUserDefaults *settings = [NSUserDefaults standardUserDefaults];
NSString *GameLevel = [[NSString alloc] initWithFormat: [settings objectForKey:kLevelKey]];

NSBundle *Bundle = [NSBundle mainBundle];
NSString *PListPath = [Bundle pathForResource:@"questions" ofType:@"plist"];

NSMutableDictionary *Dictionary = [[NSMutableDictionary alloc] initWithContentsOfFile:PListPath]; 

self.QuestionDetailsByLevel = Dictionary;
[Dictionary release];

NSMutableArray *Components = [[NSMutableArray alloc] initWithArray:[QuestionDetailsByLevel allKeys]];
self.QuestionsByLevel = Components;

int QuestionCount = [self.QuestionsByLevel count] - 1;

for (int j = 0; j < QuestionCount - 1; j++)
{

    NSString *SelectedQuestion = [self.QuestionsByLevel objectAtIndex:j];
    NSMutableArray *Array = [QuestionDetailsByLevel objectForKey:SelectedQuestion];
    self.QDetailsByLevel = Array;

    NSString *level = [[NSString alloc] initWithFormat:[self.QDetailsByLevel objectAtIndex:Level]];

    if (level != GameLevel)
        [QuestionsByLevel removeObjectAtIndex:j];   
}
}

Solution 1

Barring the other issues mentioned by everyone else, let's focus on why you're getting an out-of-bounds error.

Here's the code that's relevant.

for (int j = 0; j < QuestionCount - 1; j++)
{

    NSString *SelectedQuestion = [self.QuestionsByLevel objectAtIndex:j];
    // ... snip ...
    if (level != GameLevel) //Always happening in your current code
        [QuestionsByLevel removeObjectAtIndex:j];       
}

Let's see what happens after a few iterations of this code.

First iteration:

j == 0
self.QuestionsByLevel == [Q1, Q2, Q3, Q4, Q5]

SelectedQuestion = QuestionsByLevel[0] // Q1

// The following happens because you call removeObjectAtIndex:0
QuestionsByLevel = [Q2, Q3, Q4, Q5]

Second Iteration:

j == 1
self.QuestionsByLevel == [Q2, Q3, Q4, Q5]
SelectedQuestion = QuestionsByLevel[1] // Q3

// The following happens because you call removeObjectAtIndex:1
QuestionsByLevel = [Q2, Q4, Q5]

Third Iteration:

j == 2
self.QuestionsByLevel == [Q2, Q4, Q5]
SelectedQuestion = QuestionsByLevel[2] // Q5

// The following happens because you call removeObjectAtIndex:2
QuestionsByLevel = [Q2, Q4]

Fourth Iteration:

j == 3
self.QuestionsByLevel == [Q2, Q4]
SelectedQuestion = QuestionsByLevel[3] // CRASH!!!! 

Can you see the problem? Your for loop assumes you'll access the objects by their index, but then after each iteration, you're deleting something out of the array, which shifts all the indexes after that point. You shouldn't be calling removeObjectAtIndex:, because you're trying to walk through the array at the same time.

If you're just trying to skip a specific object, you could just call "continue" when you reach that object. If you're actually wanting to remove it out of the array, just call [QuestionsByLevel removeObject:GameLevel]. Or whatever makes sense for your situation. But do that before you iterate through the array.

Solution 2

This is not an answer. This is a critique of your code.

1. Holy memory leaks, Batman! You alloc/init: GameLevel, Components, and level, but never release any of them.
2. GameLevel doesn't need to be alloc/init'd at all. You can just pull the value out of [settings objectForKey:kLevelKey];, assign it into your GameLevel string, and use that. Then you don't even have to release it.
3. Your loop is... odd. You're iterating through the loop, but each time you iterate, you set the self.QDetailsByLevel property to a new value. Are you sure that's what you want?
4. This: if (level != GameLevel) does not do what you think it does. That's comparing pointers (ie, the ADDRESSES of two objects in memory). In your current state, both level and GameLevel have been alloc/init'd, which means they will never be the same object. You're probably wanting if ([level isEqualToString:GameLevel] == NO) instead.
5. You subtract one from [self.QuestionsByLevel count] to get your QuestionCount int, which would appear to be an upper bound on a for() loop. Yet the conditional on the for loop (which @Michael showed to be your problem) subtracts another 1 from QuestionCount, which means that your for() loop will never reach the last element in the array. Are you sure that's what you want?
6. Memorize this: http://www.cocoadevcentral.com/articles/000082.php (or this)

Solution 3

The problem is occurring, if I'm not mistaken, due to the fact that you are modifying the object while you are iterating over its contents. When you remove objects from the list, your termination condition becomes invalid. Try implementing this as a while loop, and be careful to update your termination condition whenever you remove elements from the list. You can also use the debugger to find out where you were out of bounds.

Comments

[Nico]

Also, don't let the user provide format strings. initWithFormat:[settings objectForKey…] is always wrong. The user will write a format string that is valid, but expects arguments you're not giving it. Then your app will crash. Always pass either a constant string (@"…") or a localized version of one (NSLocalizedString(@"…", /*comment*/ @"Explanation of the string").

[Nico]

I also question the presence of two properties, QuestionDetailsByLevel and QDetailsByLevel, of different types. asUwish, you should either pick one or the other, or give one or both of them better names.

[Dave DeLong]

If this is running on a Mac under garbage collection, then point 1 is moot. However, I never assume garbage collection, because knowing how to manually manage memory is an extremely valuable skill.

[Nico]

Indeed. It's worth knowing how to manually manage memory because you must be able to do that if you ever write code for a framework, library, plug-in (of any kind, including color pickers and image units), or iPhone application.