HiveBrain v1.2.0
Get Started
← Back to all entries
patternMinor

Filling text boxes with player names received from an array

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
boxeswithplayertextarraynamesfillingreceivedfrom

Problem

I'm currently trying to make the code of an app I'm developing a bit more efficient and easier to read. Basically what this does is after the user taps with two fingers (gesture) it retrieves a previously saved array from NSUserDefaults of player names, and fills in the 6 text boxes (tagged 6-11) on a table view with these names. If there isn't an existing array it'll use a default set of names.

I'm trying to reduce the length of a long for/if statement. Any ideas for simplifying this code would be appreciated.

```
if (gestureRecognizer.state == UIGestureRecognizerStateEnded) {
NSMutableArray *names = [[NSMutableArray alloc] initWithArray:[[NSUserDefaults standardUserDefaults] objectForKey:@"nameArray"]];
for (int i = 0; i <= 5; i++) {
NSIndexPath *indexPath = [NSIndexPath indexPathForRow:i inSection:0];
UITableViewCell *cell = [playerTable cellForRowAtIndexPath:indexPath];

for (UIView *view in cell.contentView.subviews) {
if ([view isKindOfClass:[UITextField class]]) {
UITextField txtField = (UITextField )view;
if (txtField.tag == 6) {
if([[NSUserDefaults standardUserDefaults] boolForKey:@"customNames"]) {
txtField.text = [names objectAtIndex:0]; }
else {
txtField.text = @"Peter";
}
}
if (txtField.tag == 7) {
if([[NSUserDefaults standardUserDefaults] boolForKey:@"customNames"]) {
txtField.text = [names objectAtIndex:1]; }
else {
txtField.text = @"Julia";
}
}
if (txtField.tag == 8) {
if([[NSUserDefaults standardUserDefaults] boolForKey:@"customNames"]) {
txtField.text

Solution

We can do better still.

First of all, we're talking about a UITableViewCell to which you've added custom views--or at least modified existing views (hence the tags) as well as adding a gesture recognizer.

What makes most sense is to instead subclass UITableViewCell. This would help both to clean up the method you have posted here as well as clean up the cellForRowAtIndexPath: which I'm sure is a bit of a mess.

Then, you would no longer need to iterate through all of the content view's subviews. Instead, best case scenario, you only have a single text field on the cell, and you can access it directly, and switch on its tag, and at worst, you can create a custom IBCollection--an array of text fields--that would be relevant to this method and iterate over just that (which you know would contain only text fields relevant to this method) looking at the tags.

int index = [txtField tag]-offsetIndex;
if (tag >= 6 && tag <= 11)
{
    if (needCustomNames)
        textField.text = [names objectAtIndex:index];
    else
        textField.text = [defaultNames objectAtIndex:index];
}


This chunk of code is confusing.

First of all, tag isn't declared anywhere.

Second of all, presuming the if is supposed to be checking textField.tag, you should either move the int index line inside the if block, or probably better, refactor the if to check the index that was just calculated based on textField.tag and is the value actually used inside the if block. As it stands, this code might crash if someone unknowingly changes the offsetIndex 7 lines previously and that might be a hard crash to figure out.

I think there's some more cleanup to do, but I'm kind of interested in seeing whether you'd be interested in posting a new question where you've rewritten the code to use a UITableViewCell subclass, which truly would be the best option.

Code Snippets

int index = [txtField tag]-offsetIndex;
if (tag >= 6 && tag <= 11)
{
    if (needCustomNames)
        textField.text = [names objectAtIndex:index];
    else
        textField.text = [defaultNames objectAtIndex:index];
}

Context

StackExchange Code Review Q#56632, answer score: 6

Revisions (0)

No revisions yet.