2
\$\begingroup\$

I got tired of stringing together objectAtIndex: and objectForKey: and hoping nothing fails along the way. I parse a lot of JSON from sources like the Google Directions API and it was cluttering my code and hard to read. In this example I am parsing the JSON returned by this call:

http://maps.googleapis.com/maps/api/directions/json?origin=415OliveStMenloPark&destination=425ShermanAvePaloAlto&sensor=false&mode=driving

I just learned Objective-C and how to program (beyond Matlab) and want some feedback on the best practices for what I am doing.

Usage

I call the class method below and pass in a dictionary along with a cascading series of keys for a dictionary and indexes for arrays. I defined a couple of macros to reduce typing.

Usage code

 NSTimeInterval flightTime = [(NSNumber *)[RDUtilities objectFromNestedJSON:responseDictionary usingCascadedKeys:RDJSONKey(@"routes"),RDJSONIndex(0), RDJSONKey(@"legs"), RDJSONIndex(0), RDJSONKey(@"duration"), RDJSONKey(@"value")] doubleValue];

Implementation Code

#define RDJSONKey(x) @{@"dKey":x}
#define RDJSONIndex(x) @{@"aIndex":@x}    

@implementation RDUtilities

+(id) objectFromNestedJSON:(id)JSONObject usingCascadedKeys:(NSDictionary*)firstArg,...

{
    NSMutableArray *keyDexList = [[NSMutableArray alloc] init];
    NSArray *subArray = nil;
    NSDictionary *subDictionary = nil;
    id subObject = nil;

    va_list args;
    va_start(args, firstArg);

    // Figure out the type of JSONObject.
    if ([JSONObject isKindOfClass:[NSDictionary class]])
    {
        subDictionary = JSONObject;
    } else if ([JSONObject isKindOfClass:[NSArray class]])
    {
        subArray = JSONObject;
    } else {
        return nil;
    }

    // Iterate through the list of arguments.
    for (NSDictionary *arg = firstArg; arg != nil; arg = va_arg(args, NSDictionary *))
    {
        if ( [[arg allKeys] containsObject:@"dKey"] || [[arg allKeys] containsObject:@"aIndex"])
        {
            [keyDexList addObject:arg];
        }
        else {
            NSLog(@"Invalid input types");
            return nil;
        }
    }

    // Look at the keyDex and pull out the next subObject from the current correct subObject.
    NSArray *allButLastKeyDex = [keyDexList objectsAtIndexes:[NSIndexSet indexSetWithIndexesInRange:NSMakeRange(0, [keyDexList count]-1)]];
    for (NSDictionary *keyDex in allButLastKeyDex)
    {
        if (subDictionary != nil && [[keyDex allKeys] containsObject:@"dKey"])
        {
            // Get the sub object that this key references.
            subObject = [subDictionary objectForKey:[keyDex objectForKey:@"dKey"]];
            // Figure out what we got.
            if ([subObject isKindOfClass:[NSDictionary class]])
            {
                subDictionary = subObject;
                subArray = nil; // We can be sure we don't try to use this incorrectly now.
            }
            else if ([subObject isKindOfClass:[NSArray class]])
            {
                subArray = subObject;
                subDictionary = nil; // We can be sure we don't try to use this incorrectly now.
            }
            // Unneeded due to containsObject constraint.
            continue;
        }

        if (subArray != nil && [[keyDex allKeys] containsObject:@"aIndex"])
        {
            // Get the sub object that this key references.
            subObject = [subArray objectAtIndex:[(NSNumber *)[keyDex objectForKey:@"aIndex"] integerValue]];
            // Figure out what we got.
            if ([subObject isKindOfClass:[NSDictionary class]])
            {
                subDictionary = subObject;
                subArray = nil; // Safer until we verify logic.
            }
            else if ([subObject isKindOfClass:[NSArray class]])
            {
                subArray = subObject;
                subDictionary = nil; // Safer until we verify logic.
            }
            // Unneeded due to containsObject constraint.
            continue;
        }
    }

    // Get the last key or index.
    NSDictionary *finalKeyDex = [keyDexList lastObject];

    // Pull out the final value and return it.
    if ([[finalKeyDex allKeys] containsObject:@"dKey"])
    {
        return [subDictionary objectForKey:[finalKeyDex objectForKey:@"dKey"]];
    }
    else if ([[finalKeyDex allKeys] containsObject:@"aIndex"])
    {
        return [subArray objectAtIndex:[(NSNumber *)[finalKeyDex valueForKey:@"aIndex"] integerValue]];
    }
    else {
        return nil;
    }
}
@end
\$\endgroup\$
3
\$\begingroup\$

First off, good job recognizing this problem and coming up with an innovative solution! I definitely like, and it's always nice to see a variadic argument list implementation. I also like that the code is written fairly accessibly, and doesn't try to get into any particularly clever solution.

The primary issue I see with this code is that it relies on conditional logic based on the type of objects rather than polymorphism, object-oriented programming, or useful features of Objective-C. This code does considerable work that the Objective-C runtime could be handling.

For instance we see groups of repeated, quite similar logic in the loop that iterates the key / index dictionaries. We could instead add methods to NSArray and NSDictionary using categories that will handle the key or index dictionaries as appropriate.

There are a few other issues, too. There is a repetition of logic at the end of the method to allow returns from the body of the if/else statements, but you could have iterated your entire loop and then just return subObject. The code in most cases due to missing/incorrect object types will return nil but in the case of an incorrect array index it throws an exception. I think a consistent behavior there is best. Also, some style issues - abbreviations like keyDex that aren't particularly explicit (something like keyOrIndexDictionary would be an improvement). This class method could easily be a function, actually, as it never refers to self - remember that in Objective-C you don't have to make your utility functions class methods on some Utilities class like you do in Java. Finally, I notice a lot of use of hard-coded strings, those should be moved out into constants.

As part of reviewing this code, I placed it in an empty iOS project with a unit test target, and document its behavior so that I could refactor it and demonstrate an alternative way of doing things. I used the sample response you linked to as input to the tests, and wrote the following test case:

@implementation RDUtilitiesTests

- (void)testOneKey
{
    id responseObject = [self responseObject];
    id returnValue = [RDUtilities objectFromNestedJSON:responseObject usingCascadedKeys:RDJSONKey(@"status"),nil];
    STAssertEqualObjects(returnValue, @"OK", nil);
}

- (void)testKeyThenIndex
{
    id responseObject = [self responseObject];
    id value = [RDUtilities objectFromNestedJSON:responseObject usingCascadedKeys:RDJSONKey(@"routes"),RDJSONIndex(0),nil];
    STAssertTrue([value isKindOfClass:[NSDictionary class]], nil);
    STAssertNotNil([(NSDictionary *)value objectForKey:@"bounds"], nil);
}

- (void)testSelfCheck
{
    id responseObject = [self responseObject];
    STAssertNotNil(responseObject, nil);
}

- (void)testIndexFirstReturnsNil
{
    id responseObject = [self responseObject];
    id value = [RDUtilities objectFromNestedJSON:responseObject usingCascadedKeys:RDJSONIndex(0),nil];
    STAssertNil(value, nil);
    value = [RDUtilities objectFromNestedJSON:responseObject usingCascadedKeys:RDJSONIndex(1),nil];
    STAssertNil(value, nil);
}

- (void)testOutOfBoundsIndexThrowsException
{
    id responseObject = [self responseObject];
    STAssertThrows(([RDUtilities objectFromNestedJSON:responseObject usingCascadedKeys:RDJSONKey(@"routes"),RDJSONIndex(1),nil]),nil);
}

- (void)testNonexistentKeyReturnsNil
{
    id responseObject = [self responseObject];
    id value = [RDUtilities objectFromNestedJSON:responseObject usingCascadedKeys:RDJSONKey(@"routes"),RDJSONIndex(0),RDJSONKey(@"LOLOLOLOLOL"),nil];
    STAssertNil(value, nil);
}

- (void)testReturnsNilWhenGivenNonDictionaryNonArrayObject
{
    NSObject *object = [[NSObject alloc] init];
    id value = [RDUtilities objectFromNestedJSON:object usingCascadedKeys:RDJSONKey(@"routes"),nil];
    STAssertNil(value, nil);
    value = [RDUtilities objectFromNestedJSON:object usingCascadedKeys:RDJSONIndex(0),nil];
    STAssertNil(value, nil);
}

- (void)testReturnsNilWhenIndexLeadsToNonDictionaryNonArrayObject
{
    id value = [RDUtilities objectFromNestedJSON:[self responseObject] usingCascadedKeys:RDJSONKey(@"routes"),RDJSONIndex(0),RDJSONKey(@"bounds"),RDJSONKey(@"northeast"),RDJSONKey(@"lat"),RDJSONKey(@"value"),nil];
    STAssertNil(value, nil);
    value = [RDUtilities objectFromNestedJSON:[self responseObject] usingCascadedKeys:RDJSONKey(@"routes"),RDJSONIndex(0),RDJSONKey(@"bounds"),RDJSONKey(@"northeast"),RDJSONKey(@"lat"),RDJSONIndex(0),nil];
    STAssertNil(value, nil);
}

- (void)testReturnsPrimitive
{
    id value = [RDUtilities objectFromNestedJSON:[self responseObject] usingCascadedKeys:RDJSONKey(@"routes"),RDJSONIndex(0),RDJSONKey(@"bounds"),RDJSONKey(@"northeast"),RDJSONKey(@"lat"),nil];
    STAssertTrue([value isKindOfClass:[NSNumber class]], nil);
    NSNumber *expected = @37.45040780;
    STAssertEqualObjects(value, expected, nil);
}

- (id)responseObject
{
    NSString *responsePath = [[NSBundle bundleForClass:[self class]] pathForResource:@"response" ofType:@"json"];
    NSData *responseData = [NSData dataWithContentsOfFile:responsePath];
    id object = [NSJSONSerialization JSONObjectWithData:responseData options:0 error:NULL];
    return object;
}

@end

It seems to provide decent coverage of the behavior to give us the freedom to change things with confidence.

As I said, the primary issue is using conditional logic based on types rather than take advantage of polymorphism. I ended up with the following implementation:

RDUtilities.h:

#import <Foundation/Foundation.h>

extern NSString *const RDJSONKeyKey;
extern NSString *const RDJSONIndexKey;

#define RDJSONKey(x) @{@"dKey":x}
#define RDJSONIndex(x) @{@"aIndex":@x}

@interface RDUtilities : NSObject

+(id) objectFromNestedJSON:(id)JSONObject usingCascadedKeys:(NSDictionary*)firstArg,...;

@end

RDUtilities.m:

#import "RDUtilities.h"

NSString *const RDJSONKeyKey = @"dKey";
NSString *const RDJSONIndexKey = @"aIndex";

@interface NSObject (RDUtilityAdditions)

- (id)rdValueForKeyOrIndexDictionary:(NSDictionary *)dictionary;

@end

@implementation NSObject (RDUtilityAdditions)

- (id)rdValueForKeyOrIndexDictionary:(NSDictionary *)dictionary
{
    return nil;
}

@end


@implementation NSArray (RDUtilityAdditions)

- (id)rdValueForKeyOrIndexDictionary:(NSDictionary *)dictionary
{
    id toReturn = nil;
    NSUInteger index = NSUIntegerMax;
    NSNumber *indexNumber = dictionary[RDJSONIndexKey];
    if ([indexNumber respondsToSelector:@selector(unsignedIntegerValue)]) {
        index = [indexNumber unsignedIntegerValue];
    }
    if (index < [self count]) {
        toReturn = self[index];
    }

    return toReturn;
}

@end

@implementation NSDictionary (RDUtilityAdditions)

- (id)rdValueForKeyOrIndexDictionary:(NSDictionary *)dictionary
{
    id toReturn = nil;
    id key = dictionary[RDJSONKeyKey];
    if (key) {
        toReturn = self[key];
    }
    return toReturn;
}

@end

@implementation RDUtilities

+(id) objectFromNestedJSON:(id)JSONObject usingCascadedKeys:(NSDictionary*)firstArg,...

{
    /*** rename for clarity - Objective-C style is usually very explicit about purpose and type ***/
    NSMutableArray *mutableKeysAndIndexes = [[NSMutableArray alloc] init];
    id subObject = nil;

    va_list args;
    va_start(args, firstArg);

    // Iterate through the list of arguments.
    for (NSDictionary *arg = firstArg; arg != nil; arg = va_arg(args, NSDictionary *))
    {
        if ( [[arg allKeys] containsObject:RDJSONKeyKey] || [[arg allKeys] containsObject:RDJSONIndexKey])
        {
            [mutableKeysAndIndexes addObject:arg];
        }
        else
        {
            NSLog(@"Invalid input types");
            return nil;
        }
    }

    subObject = JSONObject;

    for (NSDictionary *pathDictionary in mutableKeysAndIndexes) {
        subObject = [subObject rdValueForKeyOrIndexDictionary:pathDictionary];
    }

    return subObject;
}

@end

As you can see, the complexity of the method has been reduced significantly. We no longer have hard-coded string checks, or separate code paths for arrays and dictionaries, or extra logic for the last key or index.

As I mentioned earlier, passing an out of bounds index as an index key threw an exception; since the other errors seemed to return nil I decided to follow that convention with the code and I adjusted the unit tests appropriately, replacing testOutOfBoundsIndexThrowsException with testOutOfBoundsIndexReturnsNil:

- (void)testOutOfBoundsIndexReturnsNil
{
    id responseObject = [self responseObject];
    id value = [RDUtilities objectFromNestedJSON:responseObject usingCascadedKeys:RDJSONKey(@"routes"),RDJSONIndex(1),nil];
    STAssertNil(value, nil);
}

Taking a look at this refactor, I then realized that this allows key or index arguments to not be passed within dictionaries, but rather we can simply pass the keys or indexes themselves in the argument list. I rewrote the category methods and the dictionary wrapper macros, as well as modifying the types in the RDUtilities class method, and ended with the following implementation:

RDUtilities.h, second refactor:

#import <Foundation/Foundation.h>

#define RDJSONKey(x) x
#define RDJSONIndex(x) @x

@interface RDUtilities : NSObject

+(id) objectFromNestedJSON:(id)JSONObject usingCascadedKeys:(id)firstArg,...;

@end

RDUtilities.m, second refactor:

#import "RDUtilities.h"

@interface NSObject (PrivateRDUtilityAdditions)

- (id)rdValueForKeyOrIndex:(id)keyOrIndex;

@end

@implementation NSObject (PrivateRDUtilityAdditions)

- (id)rdValueForKeyOrIndex:(id)keyOrIndex
{
    return nil;
}

@end


@implementation NSArray (PrivateRDUtilityAdditions)

- (id)rdValueForKeyOrIndex:(id)keyOrIndex
{
    id toReturn = nil;
    NSUInteger index = NSUIntegerMax;
    if ([keyOrIndex respondsToSelector:@selector(unsignedIntegerValue)]) {
        index = [keyOrIndex unsignedIntegerValue];
    }
    if (index < [self count]) {
        toReturn = self[index];
    }

    return toReturn;
}

@end

@implementation NSDictionary (PrivateRDUtilityAdditions)

- (id)rdValueForKeyOrIndex:(id)keyOrIndex
{
    id toReturn = nil;
    if (keyOrIndex) {
        toReturn = self[keyOrIndex];
    }
    return toReturn;
}

@end

@implementation RDUtilities

+(id) objectFromNestedJSON:(id)JSONObject usingCascadedKeys:(id)firstArg,...
{
    NSMutableArray *mutableKeysAndIndexes = [[NSMutableArray alloc] init];
    id subObject = nil;

    va_list args;
    va_start(args, firstArg);

    // Iterate through the list of arguments.
    for (id arg = firstArg; arg != nil; arg = va_arg(args, id))
    {
        [mutableKeysAndIndexes addObject:arg];
    }

    subObject = JSONObject;

    for (id indexOrKey in mutableKeysAndIndexes) {
        subObject = [subObject rdValueForKeyOrIndex:indexOrKey];
    }

    return subObject;
}

@end

This code is considerably simpler, and opens up more opportunities for flexibility. One could develop a system of passing strings that represent a key/index path, much like the existing Cocoa key paths work. There are other opportunities for passing arguments now that can allow more dynamic use of the method. You could potentially refactor all of the work into categories, and then simply call a method specifying the order of keys and indexes on the deserialized JSON object without having to involve a separate object.

Bottom line, make sure that any time you are using an Objective-C object's class as control flow, you take a step back and look at how you could simplify that by use of polymorphism and other object oriented principles.

\$\endgroup\$
  • \$\begingroup\$ Carl, thanks for your suggestions. I've read through this and understood most of it (new to iOS [: ). I will take your suggestion and further refactor it to be categories on NSDictionary and NSArray. Also, this was inspired by the idea of NSIndexPath - perhaps I could make something conceptually similar for traversing JSON. My final question -- where can I read about or understand best practices for figuring out "how you could simplify that by the use of polymorphism and other object oriented principles". I guess I don't understand what's wrong with using an Object's class conditionals? \$\endgroup\$ – ashutosh Aug 27 '13 at 6:55
  • \$\begingroup\$ You're welcome! Why to avoid conditional logic based on the type of an object is a big subject, but I'd say it boils down to correct assignment of responsibilities of an object. Consider NSArray and NSDictionary's implementation of valueForKey:. They both implement what makes sense to them. Now imagine if client code had to implement that, and test the class of an object - there would be a lot of duplicated code, which is bad. Also see the "replace conditional with polymorphism" refactoring. \$\endgroup\$ – Carl Veazey Aug 27 '13 at 16:15

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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