views:

61

answers:

3

Hi!

It might be that the problem is straight forward but I don't get my head around it.

I have the following iPhone code

for(int x = 0 ; x < [allFriends count] ; x++)   
{
    Friend *f = [[Friend alloc] init];          
    f = [allFriends objectAtIndex:x];

    if([uid isEqualToString:[f uid]])
    {                                           
        [f AddAlbum:album];
        [allFriends replaceObjectAtIndex:x withObject:f];
     }                              
}

It doesn't matter where I call [f release] the app always crashes. Why? (BTW the loop runs a few thousand times)

Is there a more efficient way to do it?

I think I provided enough code, let me know if not!

Thanks heaps for your help!

+4  A: 

I think your code has some serious problems:

  1. Why are you allocating a new Friend if you're going to immediately leak it?
  2. Why replace an object in an array with the same object?

I think you can replace your code with this loop:

for (Friend *f in allFriends)
{
    if([uid isEqualToString:[f uid]])
    {
        [f AddAlbum:album];
    }
}
Carl Norum
Took the answer right out of my mouth :P
Jasarien
+5  A: 

The object you create in this line (and are presumably trying to release):

Friend *f = [[Friend alloc] init];

is immediately leaked when you then assign f to the object you get out of your array:

f = [allFriends objectAtIndex:x];

So it's really that object you're releasing, meaning that the pointer in the array is no longer valid (it's pointing at a released instance).

Sixten Otto
+1  A: 

Seems like you're doing way more work than you need. Something like this (I think) does the same thing, but without the leaks or crashes:

for (Friend* f in allFriends) {
  if ([uid isEqualToString:[f uid]]) {
    [f addAlbum:album];
  }
}
Stephen Darlington
Assuming those UIDs are unique, also add a `break` after the `addAlbum` call.
St3fan
St3fan thank for the little hint, makes it a lot faster!
alex25