UITableViewCell's Data is being mixed up when using GCD

Question

I'm using GCD to load my UITableView data on the background thread, however doing so mixes up the data in my custom UITableViewCell. The titleLabel and imageView on the cell are fine, but the textLabel (the subtitle) is wrong on every cell. This doesn't happen when the data is loaded on the main thread, and the data doesn't come from multiple arrays, so I can only guess it's because of my use of GCD, which I am new to.

Firstly, I set up the NSOperationQueue like so...

- (void)setUpTableForAlbums
{
   dispatch_async(dispatch_get_global_queue(0, 0), ^
               {
                   [self setUpTableForAlbumsFD];
                   dispatch_async(dispatch_get_main_queue(), ^
                                  {
                                      [albumTable reloadData];
                                  });
               });
}

The setUpTableForAlbumsFD selector is as so...

- (void)setUpTableForAlbumsFD
{
//    __block CLProgressIndeterminateView *clP = [[CLProgressIndeterminateView alloc] initWithFrame:CGRectMake(325, tableScrollView.frame.size.height/2, 310, 20)];
//    [tableScrollView addSubview:clP];
//    [clP startAnimating];
type = @"Albums";
queryAlbums = [MPMediaQuery albumsQuery];
[queryAlbums setGroupingType:MPMediaGroupingAlbum];

mainArrayAlbum = [[NSMutableArray alloc] init];
otherArrayAlbum = [[NSMutableArray alloc] init];
theOtherArrayAlbum = [[NSMutableArray alloc] init];

NSArray *paths = NSSearchPathForDirectoriesInDomains(NSDocumentDirectory, NSUserDomainMask, YES);
NSString *documentsPath = [paths objectAtIndex:0];

NSArray *fullArray = [queryAlbums collections];
for (MPMediaItemCollection *collection in fullArray)
{
    item = [collection representativeItem];
    NSString *albumName = [item valueForProperty:MPMediaItemPropertyAlbumTitle];
    NSString *albumArtist = [item valueForProperty:MPMediaItemPropertyArtist];

    NSString *filePath = [documentsPath stringByAppendingPathComponent:[NSString stringWithFormat:@"%@.png", albumName]];
    Album *album = [[Album alloc] init];
    album.albumTitle = albumName;
    album.albumArtwork = [UIImage imageImmediateLoadWithContentsOfFile:filePath];
    if (album.albumTitle.length > 4)
    {
        if ([album.albumTitle hasPrefix:@"The "])
        {
            album.albumOrderTitle = [album.albumTitle substringFromIndex:4];
        }
        else
        {
            album.albumOrderTitle = album.albumTitle;
        }
    }
    else
    {
        album.albumOrderTitle = album.albumTitle;
    }
    album.albumArtist = albumArtist;
    if (![mainArrayAlbum containsObject:album])
    {
        [mainArrayAlbum addObject:album];
    }

}
}

The Album custom class is just a container for the data.

The cellForRowAtIndex path method is as so...

MasterCellAlbum *albumCell = [tableView dequeueReusableCellWithIdentifier:@"Cell"];
    if (!albumCell)
    {
        albumCell = [[MasterCellAlbum alloc] initWithStyle:nil reuseIdentifier:@"Cell"];
    }
    alphabet = [self alphabet:@"album" withIndex:YES];
    [albumCell setSelectionStyle:UITableViewCellEditingStyleNone];
    NSString *alpha = [alphabet objectAtIndex:indexPath.section];
    NSPredicate *predicate = [NSPredicate predicateWithFormat:@"SELF.albumOrderTitle beginswith[c] %@", alpha];
    NSArray *predict = [mainArrayAlbum filteredArrayUsingPredicate:predicate];
    [cell setSelectionStyle:UITableViewCellSelectionStyleNone];


    Album *album1 = [predict objectAtIndex:indexPath.row];
    albumCell.titleLabel.text = album1.albumTitle;
    albumCell.textLabel.text = album1.albumArtist;
    albumCell.avatarImageView.image = album1.albumArtwork;

    longPress = [[UILongPressGestureRecognizer alloc] initWithTarget:self action:@selector(albumLittleMenu:)];
    [albumCell addGestureRecognizer:longPress];
    return albumCell;

Am I using GCD correctly, or is there another way I should be doing it?

Answer 1

Yikes. There are lots of things that are, shall we say, interesting about this code. Let's start with the first method:

NSOperationQueue *operationQueue = [[NSOperationQueue alloc] init];
NSInvocationOperation *operation = [NSInvocationOperation alloc];

operation = [operation initWithTarget:self selector:@selector(setUpTableForAlbumsFD) object:nil];
[operation setCompletionBlock:^
{
  [albumTable reloadData];
}];
[operationQueue addOperation:operation];
operation = nil;

What I think you're tying to do is execute the -setUpTableForAlbumsFD method in the background, and then when it's done, reload the tableView.

First, the completionBlock doesn't execute on the main thread (which is where you MUST call -reloadData from). The docs say:

The exact execution context for your completion block is not guaranteed but is typically a secondary thread. Therefore, you should not use this block to do any work that requires a very specific execution context.

The simpler way to do this method would be:

dispatch_async(dispatch_get_global_queue(0,0), ^{
  [self setUpTableForAlbumsFD];
  dispatch_async(dispatch_get_main_queue(), ^{
    [albumTable reloadData];
  }
});

---

Now for the setUpTableForAlbumsFD method...

- (void)setUpTableForAlbumsFD {
    type = @"Albums";
    queryAlbums = [MPMediaQuery albumsQuery];
    [queryAlbums setGroupingType:MPMediaGroupingAlbum];

    mainArrayAlbum = [[NSMutableArray alloc] init];

    NSArray *fullArray = [queryAlbums collections];
    for (MPMediaItemCollection *collection in fullArray) {
        item = [collection representativeItem];
        NSString *albumName = [item valueForProperty:MPMediaItemPropertyAlbumTitle];
        NSString *albumArtist = [item valueForProperty:MPMediaItemPropertyArtist];

        NSArray *paths = NSSearchPathForDirectoriesInDomains(NSDocumentDirectory, NSUserDomainMask, YES);
        NSString *documentsPath = [paths objectAtIndex:0];

You should do these two lines of finding the NSDocumentDirectory outside of the for loop, for efficiency.

        NSString *filePath = [documentsPath stringByAppendingPathComponent:[NSString stringWithFormat:@"%@.png", albumName]];

        UIImage *artwork = [UIImage imageImmediateLoadWithContentsOfFile:filePath];

I'm assuming this is a UIImage category method?

        Album *album = [[Album alloc] init];
        album.albumTitle = albumName;
        if (album.albumTitle.length > 4) {
            if ([[NSString stringWithFormat:@"%c%c%c%c", [album.albumTitle characterAtIndex:0], [album.albumTitle characterAtIndex:1], [album.albumTitle characterAtIndex:2], [album.albumTitle characterAtIndex:3]] isEqual: @"The "]) {

Yikes! Just do: if ([album.albumTitle hasPrefix:@"The "]) {

                album.albumOrderTitle = [album.albumTitle substringWithRange:NSMakeRange(4, album.albumTitle.length-4)];

And here do: album.albumOrderTitle = [album.albumTitle substringFromIndex:4];

            } else {
                album.albumOrderTitle = album.albumTitle;
            }
        } else {
            album.albumOrderTitle = album.albumTitle;

When you see multiple lines that are doing the same thing like this, it's a sign you can pull it out and do it differently. For example, you could always set the album.albumOrderTitle to the albumTitle, and then only do something different if the albumTitle length is more than 4 and it has a prefix of @"The ".

        }
        album.albumArtist = albumArtist;
        album.albumArtwork = artwork;
        if (![mainArrayAlbum containsObject:album]) {
            [mainArrayAlbum addObject:album];
        }
    }
}

---

Your cellForRowAtIndexPath: is similarly convoluted:

MasterCellAlbum *albumCell = [[MasterCellAlbum alloc] init];

You should be using UITableView's cell-reuse mechanism.

alphabet = [self alphabet:@"album" withIndex:YES];
[albumCell setSelectionStyle:UITableViewCellEditingStyleNone];
NSString *alpha = [alphabet objectAtIndex:indexPath.section];
NSPredicate *predicate = [NSPredicate predicateWithFormat:@"SELF.albumOrderTitle beginswith[c] %@", alpha];
[cell setSelectionStyle:UITableViewCellSelectionStyleNone];

NSArray *predict = [mainArrayAlbum filteredArrayUsingPredicate:predicate];

Why are you re-filtering the mainArrayAlbum every time you need a cell? It looks like you're always going to be grabbing the same alphabet, which means you're always going to be defining the same predicate, which means you're always going to be ending up with the same predict array.

Album *album1 = [predict objectAtIndex:indexPath.row];
albumCell.titleLabel.text = album1.albumTitle;
albumCell.textLabel.text = album1.albumArtist;
if (album1.albumArtwork) {
    albumCell.avatarImageView.image = album1.albumArtwork;
} else {
    albumCell.avatarImageView.image = [UIImage imageNamed:@"albumArtInvertedLight1.png"];
}

longPress = [[UILongPressGestureRecognizer alloc] initWithTarget:self action:@selector(albumLittleMenu:)];
[albumCell addGestureRecognizer:longPress];
return albumCell;

---

So, there are some obvious places where your code can use some improvement. Honestly, I think the answer to the problem you're having is because you're trying to reload the tableview on a background thread, which is a Bad Idea™.