Mesfin Bekele's OptionSelector HW

[mesbekmek]

Final Push

[tannerwelsh]

Interesting use of NSUserDefaults here. I like it. 👍

However, you've also introduced an anti-pattern here, which is that this code is dependent on the number of and order of cells in the table. If your array contained 20 items, for example, this code would re-assign the defaults object at key @"cell3" 18 times, leaving the last one assigned in place, but ignoring items 3 through 19. !

You want to avoid coupling your code to the size of an array, but instead make it flexible enough to handle any array size.

In this case, one way to improve your code is to use the type's name property as the key in the NSDefaults store:

NSUserDefaults *defaults = [NSUserDefaults standardUserDefaults];
NSArray *indexesOfCells = [self.tableView visibleCells];

for (int i=0; i < indexesOfCells.count; i++) {
    UITableViewCell *thisCell = indexesOfCells[i];
    [defaults setObject:thisCell.detailTextLabel.text forKey:thisCell.textLabel.text];
}

and then, down in tableView:cellForRowAtIndexPath:, you can refactor your code to do this instead:

Type *thisType = self.array[indexPath.row];
cell.textLabel.text = thisType.name;

cell.detailTextLabel.text = [thisType.selection isEqualToString:@" "] ? [[NSUserDefaults standardUserDefaults] objectForKey:thisType.name] : thisType.selection;

We can still improve this further, however. Your tableView:cellForRowAtIndexPath: method uses the model (Type) for state, whereas saveButtonTapped uses the view for state. This is not good. Views should not be responsible for maintaining state - the model should always be the canonical place where state is maintained.

So we can improve saveButtonTapped even more by using the models instead:

NSUserDefaults *defaults = [NSUserDefaults standardUserDefaults];

for (int i=0; i < self.array.count; i++) {
    Type *thisType = self.array[i];
    [defaults setObject:thisType.selection forKey:thisType.name];
}

I tried this out in your project, and there was still a bit left to do to make everything work out well. Specifically, I needed to modify the tableView:cellForRowAtIndexPath: to the following:

- (UITableViewCell *)tableView:(UITableView *)tableView cellForRowAtIndexPath:(NSIndexPath *)indexPath {

    UITableViewCell *cell = [tableView dequeueReusableCellWithIdentifier:@"OptionSelectorCellIdentifier" forIndexPath:indexPath];

    Type *thisType = self.array[indexPath.row];
    cell.textLabel.text = thisType.name;

    NSString *selection = thisType.selection;
    NSString *defaultSelection = [[NSUserDefaults standardUserDefaults] objectForKey:thisType.name];
    if (defaultSelection != nil && [selection isEqualToString:@" "]) {
        selection = defaultSelection;
    }

    thisType.selection = selection;
    cell.detailTextLabel.text = selection;

    return cell;
}

Notice that I included a new conditional that handles the case when the app is loaded and there is a default value to be used. With this change, it is clearer that the code will choose a selection with the following logic:

1. Always use the current value of selection
2. If the current value of selection is an empty string AND there is a non-nil default value available, use the default value

This way, selection will only ever be set to a string, and never to nil.

[tannerwelsh]

@mesbekmek nice work! Overall I thought you did a good job of adhering to best practices re: separation of concerns, using the delegate pattern, and encapsulating your data.

The main suggestions I had for improvement I wrote as inline comments. Let me know if you have any questions about them.