Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

I have some values pulled from a web service giving me a bool value for a key. I did it this way, but feel like it is repeating without need. How can I refactor this?

Please note that the key on the dictionary has the same name as the object property.

for (NSDictionary *itemDicto in switchesArray) {
    if ([[itemDicto objectForKey:@"id"]isEqualToString:@"FindBookMeeting"]) {
        currentAppswitches.FindBookMeeting = [[itemDicto objectForKey:@"value"]boolValue];
    }
    if ([[itemDicto objectForKey:@"id"]isEqualToString:@"FindCapabilityLocate"]) {
        currentAppswitches.FindCapabilityLocate = [[itemDicto objectForKey:@"value"]boolValue];
    }
    if ([[itemDicto objectForKey:@"id"]isEqualToString:@"FindDesk"]) {
        currentAppswitches.FindDesk = [[itemDicto objectForKey:@"value"]boolValue];
    }
    if ([[itemDicto objectForKey:@"id"]isEqualToString:@"FindEmployeeLocate"]) {
        currentAppswitches.FindEmployeeLocate = [[itemDicto objectForKey:@"value"]boolValue];
    }
    if ([[itemDicto objectForKey:@"id"]isEqualToString:@"Help"]) {
        currentAppswitches.Help = [[itemDicto objectForKey:@"value"]boolValue];
    }
    if ([[itemDicto objectForKey:@"id"]isEqualToString:@"ReportProblemIT"]) {
        currentAppswitches.ReportProblemIT = [[itemDicto objectForKey:@"value"]boolValue];
        //test ok
        //currentAppswitches.ReportProblemIT = NO;
    }
    if ([[itemDicto objectForKey:@"id"]isEqualToString:@"ReportProblemProperty"]) {
        currentAppswitches.ReportProblemProperty = [[itemDicto objectForKey:@"value"]boolValue];
    }
    if ([[itemDicto objectForKey:@"id"]isEqualToString:@"Settings"]) {
        currentAppswitches.Settings = [[itemDicto objectForKey:@"value"]boolValue];
    }
}
share|improve this question

3 Answers 3

up vote 7 down vote accepted

Assuming the values under the id keys always match properties of currentAppswitches.

If you can change currentAppswitches's class to accept an NSNumber object rather than a BOOL you have some convenient options.

If currentAppswitches is KVO compliant you could write:

for (NSDictionary *item in switchesArray) {
  id value = [item objectForKey:@"value"];
  id key = [item objectForKey:@"id"];
  [currentAppswitches setValue:value forKeyPath:key];
}

Alternately you could use:

for (NSDictionary *item in switchesArray) {
  BOOL value = [[item objectForKey:@"value"] boolValue];

  //construct a selector for the property setter
  NSMutableString *selectorName = [item objectForKey:@"id"];
  NSString *firstCharacter = [selectorName substringToIndex:1];
  [selectorName stringByReplacingCharactersInRange:NSMakeRange(0,1) withString:[firstCharacter uppercaseString]];
  [selectorName insertString:@"set" atIndex:0];
  [selectorName appendString:@":"];

  SEL selector = NSSelectorFromString(selectorName);
  [currentAppswitches performSelector:selector withObject:value];
}

If currentAppswitches must continue to accept BOOLs then you could create an invocation:

for (NSDictionary *item in switchesArray) {
  id value = [item objectForKey:@"value"];

  //construct a selector for the property setter
  NSMutableString *selectorName = [item objectForKey:@"id"];
  NSString *firstCharacter = [selectorName substringToIndex:1];
  [selectorName stringByReplacingCharactersInRange:NSMakeRange(0,1) withString:[firstCharacter uppercaseString]];
  [selectorName insertString:@"set" atIndex:0];
  [selectorName appendString:@":"];

  SEL selector = NSSelectorFromString(selectorName);
  NSMethodSignature* signature = [[currentAppswitches class] instanceMethodSignatureForSelector:selector];
  NSInvocation* invocation = [NSInvocation invocationWithMethodSignature:signature];
  [invocation setTarget:currentAppswitches];
  [invocation setSelector:selector ];
  [invocation setArgument:&myBoolValue atIndex:2];
  [invocation invoke];
}
share|improve this answer
    
I upvoted this for the first solution proposed. I don't particularly like the idea of building a string to then create a selector. It shouldn't be a problem to use objects in place of bools, considering we can just do @NO/@YES. Or, given BOOL value;, we can @(value). –  nhgrif Aug 22 at 2:46
    
@nhgrif I agree, building a property setter selector like that is a mess but it was fun to think about how to manage it. All of these are actually potentially fragile and could result in "unrecognized selector" or similar exceptions if the initial assumption that the "ids" match selectors does not hold. I think the first is the only one I'd be happy to see in a production app. –  Jonah Aug 22 at 2:55
    
By the way, if ([currentAppswitches respondsToSelector:selector]) probably belongs in this answer... –  nhgrif Aug 22 at 2:57
    
What does [[currentAppswitches class] instanceMethodSignatureForSelector:selector]; return if currentAppswitches doesn't know the selector? nil or throw exception? –  nhgrif Aug 22 at 2:58

First of all, notice that you repeatedly call [itemDicto objectForKey:@"id"] and [[itemDicto objectForKey:@"value"] boolValue]. Let's pull those out and store it as a NSString* and BOOL, respectively.

Also, let's change every if after the first one into an else if to speed up execution via short-circuiting. That means that if the first one is true, we don't want to check every other condition as well (because they will obviously be false), so we skip right to the end.

In more detail:

if ([string isEqualToString:@"A"]) {
    // Always executes with string = @"A", @"B" or @"C"
}
if ([string isEqualToString:@"B"]) {
    // Always executes with string = @"A", @"B" or @"C"
}
if ([string isEqualToString:@"C"]) {
    // Always executes with string = @"A", @"B" or @"C"
}

Whereas with else if:

if ([string isEqualToString:@"A"]) {
   // Always executes with string = @"A", @"B" or @"C"
}
else if ([string isEqualToString:@"B"]) {
    // Only executes with string = @"B" or @"C"
}
else if ([string isEqualToString:@"C"]) {
    // Only executes with string = @"C"
}

Finally, let's change itemDicto to a more reasonable and shorter itemDict.

The resulting code is then:

for (NSDictionary *itemDict in switchesArray) {
    NSString* task = [itemDict objectForKey:@"id"]; // Choose a name that describes the string
    BOOL value = [[itemDict objectForKey:@"value"] boolValue];
    if ([task isEqualToString:@"FindBookMeeting"]) {
        currentAppswitches.FindBookMeeting = value;
    }
    else if ([task isEqualToString:@"FindCapabilityLocate"]) {
        currentAppswitches.FindCapabilityLocate = value;
    }
    else if ([task isEqualToString:@"FindDesk"]) {
        currentAppswitches.FindDesk = value;
    }
    else if ([task isEqualToString:@"FindEmployeeLocate"]) {
        currentAppswitches.FindEmployeeLocate = value;
    }
    else if ([task isEqualToString:@"Help"]) {
        currentAppswitches.Help = value;
    }
    else if ([task isEqualToString:@"ReportProblemIT"]) {
        currentAppswitches.ReportProblemIT = value;
    }
    else if ([task isEqualToString:@"ReportProblemProperty"]) {
        currentAppswitches.ReportProblemProperty = value;
    }
    else if ([task isEqualToString:@"Settings"]) {
        currentAppswitches.Settings = value;
    }
}

Finally, I would suggest have currentAppswitches have a NSMutableDictionary property called tasks where the keys are @"FindBookMeeting", @"FindDesk" etc. and the values are the BOOL's associated with them, but represented as NSNumber. Then, this snippet can be reduced tremendously to this:

for (NSDictionary *itemDict in switchesArray) {
    NSString* task = [itemDict objectForKey:@"id"]; // Choose a name that describes the string
    BOOL value = [[itemDict objectForKey:@"value"] boolValue];
    if (task != nil) {
        [currentAppswitches.tasks setObject:[NSNumber numberWithBool:value] forKey:task];
    }
share|improve this answer

Since the other answers are quite good, I will just talk about a couple of minor points.

Your variable names need some help. For example:

for (NSDictionary *itemDicto in switchesArray)

When you have variable names that contain an explanation of how that information is being stored (such as Dicto or Array), then if you ever change the method that you use to store the information (such as switching an array to a dictionary or converting the whole structure to a class) you will need to change the names of the variables to match. It is better to avoid this altogether and use a name that does not include the type of information store.

So instead of itemDicto, I would call this something that better describes the information actually contained inside it. Something that makes it more clear what the items are, for example. The same thing is true of switchesArray, it is unclear what the actual meaning of the word switches is in this context.

Good names are very hard to come up with when programming, but they are important. A big part of making your code self documenting is using the best and most explicit names that you can.

I believe this principle extends to the following names as well: FindBookMeeting, FindCapabilityLocate, Help. The names are not very helpful for determining what is going to happen when these choices are selected.

Maybe it is because I just do not completely understand the context of the code, but I have a hard time imagining what the program will do with the BOOL value being returned based on the names you have chosen. I believe that this is a sign that the whole logic structure should be changed to something more clear, but the other answers have provided tips on this topic. Briefly I will suggest that the fact that you might do a Find, make a Report, or look up Settings all from this same menu suggests that too many different things are all being scrunched together in this dictionary.

Another suggestion I would make is to use NSString constants instead of string literals when you are making calls to a dicitonary. Either at the top of the file, or in another file that you import into this file, you would do something like this:

NSString* const kHelp = @"Help";
NSString* const kSettings = @"Settings";
NSString* const kReportProblemIT = @"ReportProblemIT";

This gives you some advantages. First, it is harder to make a mistake later if you ever need to use the string again. If you just type a string literal with @"" then the compiler will not do any error checking. If there are any typos, the code will fail and you might see strange errors that are hard to debug. Second, once you have it set up like the above, when you start typing one of the string constants you will be able to use autocomplete to avoid having to type the whole thing every time. Finally, if you ever want to make changes to the names of the strings, they will all be in one place and easily visible.

share|improve this answer
1  
Additionally, comparing two strings that are set to a constant string can sometimes more efficient than comparing string literals. –  nhgrif Aug 21 at 21:23

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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