4
\$\begingroup\$

I'd like to see how I can improve this code, as I know it's bad practice to have cell reuse identifiers like this, but I could not find any other way to keep the cells that contain images from calling the server again instead of reading the cached image.

- (UITableViewCell *)tableView:(UITableView *)tableView cellForRowAtIndexPath:(NSIndexPath *)indexPath
{
   NSString *cellIdentifier = [NSString stringWithFormat:@"cell%d%d",indexPath.section,indexPath.row];

    UITableViewCell *cell = [tableView dequeueReusableCellWithIdentifier:cellIdentifier];

    if (cell == nil)
    {
        UIImageView *profileImageView = [[UIImageView alloc]initWithFrame:CGRectMake(12, 4, 60, 60)];
        profileImageView.image = [UIImage imageNamed:@"Profile"];
        profileImageView.layer.cornerRadius = 30.0;
        profileImageView.layer.masksToBounds = YES;
        profileImageView.contentMode = UIViewContentModeScaleAspectFit;

        UILabel *userNameLabel = [[UILabel alloc]init];
        userNameLabel.frame = CGRectMake(82, 0, 228, 68);

        cell = [[UITableViewCell alloc]initWithStyle:UITableViewCellStyleDefault reuseIdentifier:cellIdentifier];

        [cell.contentView addSubview:profileImageView];
        [cell.contentView addSubview:userNameLabel];
        cell.selectionStyle = UITableViewCellSelectionStyleDefault;

        if (indexPath.section == 0)
        {
            cell.selectionStyle = UITableViewCellSelectionStyleNone;

            UIView *requestsView = [[UIView alloc]initWithFrame:CGRectMake(240, 0, 80, 68)];

            UIButton *approveButton = [UIButton buttonWithType:UIButtonTypeSystem];

            approveButton.frame = CGRectMake(0, 16, 36, 36);

            [approveButton addTarget:self action:@selector(approveButtonChanged:) forControlEvents:UIControlEventTouchUpInside];

            [approveButton setTintColor:[UIColor companyBlue]];

            [approveButton setImage:[UIImage imageNamed:@"Approve"] forState:UIControlStateNormal];

            [requestsView addSubview:approveButton];

            UIButton *denyButton = [UIButton buttonWithType:UIButtonTypeSystem];

            denyButton.frame = CGRectMake(42, 16, 36, 36);

            [denyButton addTarget:self action:@selector(denyButtonChanged:) forControlEvents:UIControlEventTouchUpInside];

            [denyButton setTintColor:[UIColor companyRed]];

            [denyButton setImage:[UIImage imageNamed:@"Deny"] forState:UIControlStateNormal];

            [requestsView addSubview:denyButton];

            userNameLabel.frame = CGRectMake(82, 0, 167, 68);

            NSInteger row = (NSInteger)[indexPath row];
            NSArray *keys = [self.tableDataSource objectForKey:@"Requests"];
            id aKey = [keys objectAtIndex:row];

            userNameLabel.text = [aKey objectForKey:@"User"];

            NSMutableURLRequest *request = [NSMutableURLRequest requestWithURL:[NSURL URLWithString:[aKey objectForKey:@"URL"]]];
            request.cachePolicy = NSURLRequestReloadIgnoringLocalAndRemoteCacheData;
            request.timeoutInterval = 5.0;

            [NSURLConnection sendAsynchronousRequest:request
                                           queue:[NSOperationQueue mainQueue]
                               completionHandler:
             ^(NSURLResponse *response, NSData *data, NSError *connectionError)
             {
                 if (data)
                 {
                     dispatch_async(dispatch_get_main_queue(), ^
                     {      
                       profileImageView.image = [UIImage imageWithData:data];
                     });
                 }
             }];

            [cell.contentView addSubview:requestsView];
        }
        if (indexPath.section == 1)
        {
            NSInteger row = (NSInteger)[indexPath row];
            NSArray *keys = [self.tableDataSource objectForKey:@"Friends"];
            id aKey = [keys objectAtIndex:row];

            NSString *firstName = [aKey objectForKey:@"First"];
            NSString *lastName = [aKey objectForKey:@"Last"];

            if (firstName.length < 1 && lastName.length < 1)
            {
                userNameLabel.text = [aKey objectForKey:@"User"];
            }
            else if (firstName.length > 0 && lastName.length > 0)
            {
                NSString *combined = [NSString stringWithFormat:@"%@ %@",firstName,lastName];
                userNameLabel.text = combined;
            }
            else if (firstName.length < 1 && lastName.length > 0)
            {
                userNameLabel.text = lastName;
            }
            else
            {
                userNameLabel.text = firstName;
            }

            NSMutableURLRequest *request = [NSMutableURLRequest requestWithURL:[NSURL URLWithString:[aKey objectForKey:@"URL"]]];
            request.cachePolicy = NSURLRequestReloadIgnoringLocalAndRemoteCacheData;
            request.timeoutInterval = 5.0;

            [NSURLConnection sendAsynchronousRequest:request
                                           queue:[NSOperationQueue mainQueue]
                               completionHandler:
             ^(NSURLResponse *response, NSData *data, NSError *connectionError)
             {
                 if (data)
                 {
                     dispatch_async(dispatch_get_main_queue(), ^
                     {
                         profileImageView.image = [UIImage imageWithData:data];
                     });
                 }
             }];
        }
        if (indexPath.section == 2)
        {
            NSInteger row = (NSInteger)[indexPath row];
            NSArray *keys = [self.tableDataSource objectForKey:@"Blocked"];
            id aKey = [keys objectAtIndex:row];
            userNameLabel.text = [aKey objectForKey:@"User"];
        }
    }

    return cell;
}

Update new code:

if (!self.pictureArray || !self.pictureArray.count)
{
    self.pictureArray = [NSMutableArray arrayWithCapacity:[[self.tableDataSource objectForKey:@"Friends"]count]+[[self.tableDataSource objectForKey:@"Requests"]count]];
}

if (![self.pictureArray objectAtIndex:indexPath.row])
{
    [NSURLConnection sendAsynchronousRequest:request
                                           queue:[NSOperationQueue mainQueue]
                               completionHandler:
     ^(NSURLResponse *response, NSData *data, NSError *connectionError)
     {
         if (data)
         {
             dispatch_async(dispatch_get_main_queue(), ^
            {
                [self.pictureArray insertObject:[UIImage imageWithData:data]
                                                            atIndex:indexPath.row];
                profileImageView.image = [self.pictureArray objectAtIndex:indexPath.row];
            });
         }
     }];
}
else
{
    profileImageView.image = [self.pictureArray objectAtIndex:indexPath.row];
}
\$\endgroup\$
5
\$\begingroup\$

So, first thing's first... we definitely need to break this down into groups of smaller methods which are all called from the cellForRowAtIndexPath:. The general rule of thumb is that you should be able to fit an entire method on your screen at the same time without the need to scroll in any direction... and I'm even stricter than that myself.

For example:

UIImageView *profileImageView = [[UIImageView alloc]initWithFrame:CGRectMake(12, 4, 60, 60)];
profileImageView.image = [UIImage imageNamed:@"Profile"];
profileImageView.layer.cornerRadius = 30.0;
profileImageView.layer.masksToBounds = YES;
profileImageView.contentMode = UIViewContentModeScaleAspectFit;

// ....

[cell.contentView addSubview:profileImageView];

We can improve this in many ways. First, add this method.

- (UIImageView *)profileImageView {
    profImgVw = [[UIImageView alloc] initWithFrame:
        CGRectMake(12.0f, 4.0f, 60.0f, 60.0f)];
    // we don't need to set the image if we're just going to change it later
    //profImgVw.image = [UIImage imageNamed:@"Profile"];
    profImgVw.layer.cornerRadius = 30.0f;
    profImgVw.layer.masksToBounds = YES;
    profImgVw.contentMode = UIViewContentModeScaleAspectFit;

    return profImgVw;
}

Now we can replace some lines in the cellForRowAtIndexPath: to look like this:

UIImageView *profileImageView = [self profileImageView];

// after this method call but before the following method call, you
// will need to set the image of the profileImageView.

[cell.contentView addSubview:profileImageView];

And we've already cut 5 lines out of your cellForRowAtIndexPath: in an attempt to get it down to a more manageable size in terms of readability and maintainability.

You should work through the rest of the method to make tweaks like this.


Now then, as to your specific question regarding how to keep around images instead of having to download them again every time the cell is displayed? My approach would be to store them in an NSMutableArray or perhaps an NSMutableDictionary. Whichever one you'd be more comfortable with in terms of pulling the data out. But here's some pseudo-code to get you started...

@property (nonatomic,strong) NSMutableArray *imgArray;

Create a @property in the .m file. Now, in our viewDidLoad or somewhere before we've started filling the table, we want to give the imgArray a capacity:

self.imgArray = [NSMutableArray arrayWithCapacity:someCapacity];

Where someCapacity is calculated in the same way you calculate the number of rows for the table view section 1.

Now then, in the cellForRowAtIndexPath: method when we're potentially downloading the images, we're going to do something a little similar to the approach I took in the example profileImageView method I showed you.

if (![self.imgArray objectAtIndex:indexPath.row]) {
    // we've never loaded the img before, so we need to download it
    // download it, then assign it to the array
    [self.imgArray insertObject:[UIImage imageWithData:data]
        atIndex:indexPath.row];
    profileImageView.image = [self.imgArray objectAtIndex:indexPath.row];
} else {
    // image is already in self.imgArray, so just grab it from there
    profileImageView.image = [self.imgArray objectAtIndex:indexPath.row];
}

Now we only need to keep the images in memory and we can let the table view recycle all the other elements of the array that don't need to be downloaded and can easily be redrawn on the fly.

\$\endgroup\$
  • \$\begingroup\$ I tried implementing the new image view method, but now no images are shown. Perhaps this is because my reuse identifiers are bad practice in the original question. \$\endgroup\$ – klcjr89 Feb 15 '14 at 5:31
  • \$\begingroup\$ Yes. Your approach to reuse identifiers means the cells aren't actually being reused at all. \$\endgroup\$ – nhgrif Feb 15 '14 at 12:47
  • \$\begingroup\$ As another note, you probably shouldn't actually add the profileImageView as a subview until you've set the final image. After looking at your code again, I think that the way I implemented the method won't work for your case because you're actually setting a different image to each one. \$\endgroup\$ – nhgrif Feb 15 '14 at 12:49
  • \$\begingroup\$ Please see my updated code above in original question; I'm still confused as to if I'm on the right path of reusing the cells correctly. \$\endgroup\$ – klcjr89 Feb 15 '14 at 21:01
  • \$\begingroup\$ Okay I feel that you've pointed my in the right direction. However, the imgArray is always empty when I try to set it's capacity to match the table data source on view did load \$\endgroup\$ – klcjr89 Feb 15 '14 at 22:44

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.