Custom tableview cell for displaying different foods

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.

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.

StackExchange Code Review Q#61189, answer score: 3