views:

16028

answers:

12

I'm doing some Objective-C programming that involves parsing an NSXmlDocument and populating an objects properties from the result.

First version looked like this:

if([elementName compare:@"companyName"] == 0) 
  [character setCorporationName:currentElementText]; 
else if([elementName compare:@"corporationID"] == 0) 
  [character setCorporationID:currentElementText]; 
else if([elementName compare:@"name"] == 0) 
  ...

But I don't like the if-else-if-else pattern this produces. Looking at the switch statement I see that i can only handle ints, chars etc and not objects... so is there a better implementation pattern I'm not aware of?

BTW I did actually come up with a better solution for setting the object's properties, but I want to know specifically about the if-else vs switch pattern in Objective-C

+2  A: 

The most common refactoring suggested for eliminating if-else or switch statements is introducing polymorphism (see http://www.refactoring.com/catalog/replaceConditionalWithPolymorphism.html). Eliminating such conditionals is most important when they are duplicated. In the case of XML parsing like your sample you are essentially moving the data to a more natural structure so that you won't have to duplicate the conditional elsewhere. In this case the if-else or switch statement is probably good enough.

Bradley Harris
+3  A: 

The if-else implementation you have is the right way to do this, since switch won't work with objects. Apart from maybe being a bit harder to read (which is subjective), there is no real downside in using if-else statements this way.

Palmin
+1  A: 

In this case, I'm not sure if you can easily refactor the class to introduce polymorphism as Bradley suggests, since it's a Cocoa-native class. Instead, the Objective-C way to do it is to use a class category to add an elementNameCode method to NSSting:

   typedef enum { 
       companyName = 0,
       companyID,  
       ...,
       Unknown
    } ElementCode;

    @interface NSString (ElementNameCodeAdditions)
    - (ElementCode)elementNameCode; 
    @end

    @implementation NSString (ElementNameCodeAdditions)
    - (ElementCode)elementNameCode {
        if([self compare:@"companyName"]==0) {
            return companyName;
        } else if([self compare:@"companyID"]==0) {
      return companyID;
     } ... {

     }

     return Unknown;
    }
    @end

In your code, you could now use a switch on [elementName elementNameCode] (and gain the associated compiler warnings if you forget to test for one of the enum members etc.).

As Bradley points out, this may not be worth it if the logic is only used in one place.

Barry Wark
+2  A: 

Although there's not necessarily a better way to do something like that for one time use, why use "compare" when you can use "isEqualToString"? That would seem to be more performant since the comparison would halt at the first non-matching character, rather than going through the whole thing to calculate a valid comparison result (though come to think of it the comparison might be clear at the same point) - also though it would look a little cleaner because that call returns a BOOL.

if([elementName isEqualToString:@"companyName"] ) 
  [character setCorporationName:currentElementText]; 
else if([elementName isEqualToString:@"corporationID"] ) 
  [character setCorporationID:currentElementText]; 
else if([elementName isEqualToString:@"name"] )
Kendall Helmstetter Gelner
As indicted in the original question i already found a better way to do the setting of properties. I was just looking for advice on any better pattern for dealing with the if-else pattern.
craigb
Nice downvote. It's still useful information for people who would prefer cleaner ways to compare string. You are abusing downvotes, the only consolation is that it's costing you reputation to discard perfectly good information. I'll not make the mistake of helping you again.
Kendall Helmstetter Gelner
+10  A: 

You should take advantage of Key-Value Coding:

[character setValue:currentElementText forKey:elementName];

If the data is untrusted, you might want to check that the key is valid:

if (![validKeysCollection containsObject:elementName])
    // Exception or error
jmah
As stated in question, I already found a better way of setting these properties and was just looking for advice on the if-else pattern that i didn't like.
craigb
But that is the point, to avoid doing the multiple dispatch yourself, and letting aspects of the language (or framework) handle it. The if-else-else-else or switch pattern on object value should be discouraged, when you can do things like dictionary look-ups.
jmah
+2  A: 

One way I've done this with NSStrings is by using an NSDictionary and enums. It may not be the most elegant, but I think it makes the code a little more readable. The following pseudocode is extracted from one of my projects:

typedef enum { UNKNOWNRESIDUE, DEOXYADENINE, DEOXYCYTOSINE, DEOXYGUANINE, DEOXYTHYMINE } SLSResidueType;

static NSDictionary *pdbResidueLookupTable;
...

if (pdbResidueLookupTable == nil)
{
    pdbResidueLookupTable = [[NSDictionary alloc] initWithObjectsAndKeys:
           [NSNumber numberWithInteger:DEOXYADENINE], @"DA", 
           [NSNumber numberWithInteger:DEOXYCYTOSINE], @"DC",
           [NSNumber numberWithInteger:DEOXYGUANINE], @"DG",
           [NSNumber numberWithInteger:DEOXYTHYMINE], @"DT",
           nil]; 
}

SLSResidueType residueIdentifier = [[pdbResidueLookupTable objectForKey:residueType] intValue];
switch (residueIdentifier)
{
    case DEOXYADENINE: do something; break;
    case DEOXYCYTOSINE: do something; break;
    case DEOXYGUANINE: do something; break;
    case DEOXYTHYMINE: do something; break;
}
Brad Larson
+7  A: 

I hope you'll all forgive me for going out on a limb here, but I would like to address the more general question of parsing XML documents in Cocoa without the need of if-else statements. The question as originally stated assigns the current element text to an instance variable of the character object. As jmah pointed out, this can be solved using key-value coding. However, in a more complex XML document this might not be possible. Consider for example the following.

<xmlroot>
    <corporationID>
        <stockSymbol>EXAM</stockSymbol>
        <uuid>31337</uuid>
    </corporationID>
    <companyName>Example Inc.</companyName>
</xmlroot>

There are multiple approaches to dealing with this. Off of the top of my head, I can think of two using NSXMLDocument. The first uses NSXMLElement. It is fairly straightforward and does not involve the if-else issue at all. You simply get the root element and go through its named elements one by one.

NSXMLElement* root = [xmlDocument rootElement];

// Assuming that we only have one of each element.
[character setCorperationName:[[[root elementsForName:@"companyName"] objectAtIndex:0] stringValue]];

NSXMLElement* corperationId = [root elementsForName:@"corporationID"];
[character setCorperationStockSymbol:[[[corperationId elementsForName:@"stockSymbol"] objectAtIndex:0] stringValue]];
[character setCorperationUUID:[[[corperationId elementsForName:@"uuid"] objectAtIndex:0] stringValue]];

The next one uses the more general NSXMLNode, walks through the tree, and directly uses the if-else structure.

// The first line is the same as the last example, because NSXMLElement inherits from NSXMLNode
NSXMLNode* aNode = [xmlDocument rootElement];
while(aNode = [aNode nextNode]){
    if([[aNode name] isEqualToString:@"companyName"]){
        [character setCorperationName:[aNode stringValue]];
    }else if([[aNode name] isEqualToString:@"corporationID"]){
        NSXMLNode* correctParent = aNode;
        while((aNode = [aNode nextNode]) == nil && [aNode parent != correctParent){
            if([[aNode name] isEqualToString:@"stockSymbol"]){
                [character setCorperationStockSymbol:[aNode stringValue]];
            }else if([[aNode name] isEqualToString:@"uuid"]){
                [character setCorperationUUID:[aNode stringValue]];
            }
        }
    }
}

This is a good candidate for eliminating the if-else structure, but like the original problem, we can't simply use switch-case here. However, we can still eliminate if-else using NSInvocation. The first step is to define the a method for each element.

- (NSNode*)parse_companyName:(NSNode*)aNode
{
    [character setCorperationName:[aNode stringValue]];
    return aNode;
}

- (NSNode*)parse_corporationID:(NSNode*)aNode
{
    NSXMLNode* correctParent = aNode;
    while((aNode = [aNode nextNode]) == nil && [aNode parent != correctParent){
        [self invokeMethodForNode:aNode prefix:@"parse_corporationID_"];
    }
    return [aNode previousNode];
}

- (NSNode*)parse_corporationID_stockSymbol:(NSNode*)aNode
{
    [character setCorperationStockSymbol:[aNode stringValue]];
    return aNode;
}

- (NSNode*)parse_corporationID_uuid:(NSNode*)aNode
{
    [character setCorperationUUID:[aNode stringValue]];
    return aNode;
}

The magic happens in the invokeMethodForNode:prefix: method. We generate the selector based on the name of the element, turn it into an NSInvocation, pass aNode in as the only parameter (NSInvocation parameters start at 2), and invoke the NSInvocation. Presto bango, we've eliminated the need for an if-else statement. Here's the code for that method.

- (NSNode*)invokeMethodForNode:(NSNode*)aNode prefix:(NSString*)aPrefix
{
    NSNode* ret = nil;
    NSMutableString* methodName = [NSMutableString stringWithCapacity:[prefix length] + [[aNode name] length] + 1];
    [methodName appendString:prefix];
    [methodName appendString:[aNode name];
    [methodName appendString"@":"];
    SEL selector = NSSelectorFromString(methodName);
    NSMethodSignature* sig = [[self class] instanceMethodSignatureForSelector:selector];
    if(sig != nil){
        NSInvocation* invocation = [NSInvocation invocationWithMethodSignature:sig];
        [invocation setSelector:selector];
        [invocation setTarget:self];
        [invocation setArgument:&aNode atIndex:2];
        [invocation invoke];
        [invocation getReturnValue:&ret];
    }
    return ret;
}

Now, instead of our larger if-else statement (the one that differentiated between companyName and corporationID), we can simply write one line of code

NSXMLNode* aNode = [xmlDocument rootElement];
while(aNode = [aNode nextNode]){
    aNode = [self invokeMethodForNode:aNode prefix:@"parse_"];
}

Now I apologize if I got any of this wrong, it's been a while since I've written anything with NSXMLDocument, it's late at night and I didn't actually test this code. So if you see anything wrong, please leave a comment or edit this answer.

However, I believe I have just shown how NSInvocation can be used in Cocoa to completely eliminate if-else statements in cases like this. There are a few gotchas and corner cases. You have to make sure that the method names you generate aren't going to call other methods, especially if your NSInvocation is targeting another object, and this particular method naming scheme won't work on elements with non-alphanumeric characters. You could get around that by escaping the XML element names in your method names somehow, or by building an NSDictioanry using the method names as the keys and the selectors as the values. This can get pretty memory intensive and end up taking a longer time. NSInvocation dispatch like I described is pretty fast. For very large if-else statements, this method may even be faster than an if-else statement.

Michael Buckley
+5  A: 

Dare I suggest using a macro?

#define TEST( _name, _method ) \
  if ([elementName isEqualToString:@ _name] ) \
    [character _method:currentElementText]; else
#define ENDTEST { /* empty */ }

TEST( "companyName",      setCorporationName )
TEST( "setCorporationID", setCorporationID   )
TEST( "name",             setName            )
:
:
ENDTEST
epatel
Nice, hadn't thought of that one. Not used to using macros much.
craigb
+6  A: 

If you want to use as little code as possible, and your element names and setters are all named so that if elementName is @"foo" then setter is setFoo:, you could do something like:

SEL selector = NSSelectorFromString([NSString stringWithFormat:@"set%@:", [elementName capitalizedString]]);

[character performSelector:selector withObject:currentElementText];

or possibly even:

[character setValue:currentElementText forKey:elementName]; // KVC-style

Though these will of course be a bit slower than using a bunch of if statements.

[Edit: The second option was already mentioned by someone; oops!]

Wevah
+1 for it's pretty neat as ObjC can be...
epatel
I added a correction to this below -- http://stackoverflow.com/questions/104339/objective-c-switch-using-objects/1215783#1215783
Dennis Munsie
+1  A: 

There is actually a fairly simple way to deal with cascading if-else statements in a language like Objective-C. Yes, you can use subclassing and overriding, creating a group of subclasses that implement the same method differently, invoking the correct implementation at runtime using a common message. This works well if you wish to choose one of a few implementations, but it can result in a needless proliferation of subclasses if you have many small, slightly different implementations like you tend to have in long if-else or switch statements.

Instead, factor out the body of each if/else-if clause into its own method, all in the same class. Name the messages that invoke them in a similar fashion. Now create an NSArray containing the selectors of those messages (obtained using @selector()). Coerce the string you were testing in the conditionals into a selector using NSSelectorFromString() (you may need to concatenate additional words or colons to it first depending on how you named those messages, and whether or not they take arguments). Now have self perform the selector using performSelector:.

This approach has the downside that it can clutter-up the class with many new messages, but it's probably better to clutter-up a single class than the entire class hierarchy with new subclasses.

+1  A: 

What we've done in our projects where we need to so this sort of thing over and over, is to set up a static CFDictionary mapping the strings/objects to check against to a simple integer value. It leads to code that looks like this:

static CFDictionaryRef  map = NULL;
int count = 3;
const void *keys[count] = { @"key1", @"key2", @"key3" };
const void *values[count] = { (uintptr_t)1, (uintptr_t)2, (uintptr_t)3 };

if (map == NULL)
    map = CFDictionaryCreate(NULL,keys,values,count,&kCFTypeDictionaryKeyCallBacks,NULL);


switch((uintptr_t)CFDictionaryGetValue(map,[node name]))
{
    case 1:
        // do something
        break;
    case 2:
        // do something else
        break;
    case 3:
        // this other thing too
        break;
}

If you're targeting Leopard only, you could use an NSMapTable instead of a CFDictionary.

Mike Shields
+3  A: 

Posting this as a response to Wevah's answer above -- I would've edited, but I don't have high enough reputation yet:

unfortunately the first method breaks for fields with more than one word in them -- like xPosition. capitalizedString will convert that to Xposition, which when combined with the format give you setXposition: . Definitely not what was wanted here. Here is what I'm using in my code:

NSString *capName = [elementName stringByReplacingCharactersInRange:NSMakeRange(0, 1) withString:[[elementName substringToIndex:1] uppercaseString]];
SEL selector = NSSelectorFromString([NSString stringWithFormat:@"set%@:", capName]);

Not as pretty as the first method, but it works.

Dennis Munsie
Thanks for the catch!
Wevah