Custom tableview cell for displaying different foods

Question

I have a custom tableview cell for displaying different foods, with a star next to each name. If the food has been favorited (i.e., it exists in the database), the star is filled in, otherwise it is empty. I'm not sure if the logic that determines the color of the star should go in the view controller or the tableviewcell.

Currently, I have this in the view controller under didSelectRowForIndexPath:

cell.foodLabel.text= food.name;
if ([cell isStarredForName:food.name])
        [cell changeStarToState:StateFull];
else
        [cell changeStarToState:StateEmpty];

Also, I use this cell in two different view controllers, so I have to use the code above twice.

In the cell, I have:

- (BOOL) isStarredForName:(NSString*)foodName {

    PFInstallation *installation = [PFInstallation currentInstallation];
    NSArray *foodList = (NSArray*)[installation objectForKey:@"favorites"];
    return ([foodList containsObject:foodName]);
}

- (void)changeStarToState:(StarState)state{
    NSString *imgName;
    if (state == StateEmpty){
        imgName = @"star_none.png";
        _currentState = StateEmpty;
    }
    else {
        imgName = @"star_full.png";
        _currentState = StateFull;
    }
    [self.favButton setImage:[UIImage imageNamed:imgName] forState:UIControlStateNormal];

}

I'm wondering if it's better to just have a [cell handleStarColor] function in the viewcontroller and have the cell decide whether it needs to fill in the star.

Answer 1

For what you're doing, both options are wrong actually.

Currently, we're asking both the UITableViewDelegate and the UITableViewCell to do too much work. Your proposed change only shifts more work onto the UITableViewCell, which is already doing too much.

Instead, we need a custom class for carrying around our data. One way or another, the UITableViewDelegate gets a reference to an array of these objects. This class should have methods and properties that can tell the cell how to appear.

In this specific case, one of those properties is:

@property BOOL isFavorited;

And one of those methods might be:

- (void)toggleFavorite {
    self.isFavorited = !self.isFavorited;
}

In the table view's data source, you grab the object out of the array the corresponds to the section/row you're creating the cell for and you ask these custom objects how the cell should look.

The cell could have a property as such:

@property (nonatomic,assign) BOOL isFavorited;

And we implement the setter for it as such:

- (void)setIsFavorited:(BOOL)isFavorited {
    _isFavorited = isFavorited
    NSString *imgName = isFavorited ? @"star_full.png" : @"star_none.png";
    [self.favButton setImage:[UIImage imageNamed:imgName] forState:UIControlStateNormal];
}

And in didSelectRowAtIndexPath, we shouldn't be calling cellForRowAtIndexPath: to completely reconstruct the cell. This wastes a lot of time setting up the cell. Instead, we use the index path to reach into our original array again and grab out the data object that we used to create the cell in the first place.

MyDataObject *dataObject = self.myArrayOfDataObject[indexPath.row];

Then from here, we modify the data object:

[dataObject toggleFavorite];

Now, since we set up cellForRowAtIndexPath: to set up the cell based on our data object, we tell the table to refresh itself (or at least this row) using either:

[self.tableView reloadData];

Or reload just the individual cell:

[self.tableview reloadRowsAtIndexPaths:@[indexPath] 
                      withRowAnimation:UITableViewRowAnimationNone];

Answer 2

I can't talk much about the big picture issue that you are asking about, but I see plenty of improvements that can be made to the code.

When working with Objective-C, it is considered best practice to follow Apple's guidelines and conventions for the language. One of these is to use the full name of the enum when naming its contents. I see you have the enum StarState and then you have StateEmpty and StateFull. These should be changed to StarStateEmpty and StarStateFull. Sometimes this can lead to really long names, but it is still Apple's recommendation to do things this way. The biggest reason why is that if two names conflict, the compiler will not be able to determine which one is which. This is a real problem here, because if you ever want to have fill states for another type of object, you will have problems trying to use just StateFull. Another option would be to change the name of the enum to ObjectFillState and then you can reuse it for other objects.

If I remember correctly, in a previous question you were advised to fix your irregular white spacing, and to also always use brackets around if-else statements. It is better to always have brackets so that the code is more readable.

if ([cell isStarredForName:food.name])
    [cell changeStarToState:StateFull];
else
    [cell changeStarToState:StateEmpty];

There are lots of ways that a construct like the above could go wrong in the future, so it is better to just put in the brackets. Also this cell.foodLabel.text= food.name; is missing a space after text. A minor nitpick but things like that are still important for readability.

I have a feeling that there is a much better way to go about retrieving the information from the dictionary here (such as an object that returns the proper value from a properly named method):

PFInstallation *installation = [PFInstallation currentInstallation];
NSArray *foodList = (NSArray*)[installation objectForKey:@"favorites"];
return ([foodList containsObject:foodName]);

But I will specifically mention this part: (NSArray*)[installation objectForKey:@"favorites"]. Someone here on Code Review once said this to me, and I think it is very good advice: "The most common purpose of a cast is to turn a compile-time error into a run-time error." Please click the link and read his expanded explanation of this quote. I think it is very good advice.

I do not know completely if it is best practice in this case or not, but I am not a fan of uninitialized values such as this: NSString *imgName;. There is a bigger problem with the method - (void)changeStarToState:(StarState)state however. The name says that it changes the state of the star, but then it also includes this line:

[self.favButton setImage:[UIImage imageNamed:imgName] forState:UIControlStateNormal];

At the very least the method name should be changed to reflect that the image is being set also. However, I'm thinking that this is actually an indication of a bigger design problem. It makes me think that you may want to change your approach. Since that is the reason you posted this question in the first place, I think you are on the right track. Hopefully someone else will be able to elaborate on the design part of your question.