5
\$\begingroup\$

I'm fairly new to Objective-C. The app I'm trying to make, is using the foursquare API to list up places that is close by. I've re-written this app more times than I care to admit, because I feel the implementation get messier the bigger the app gets. I take that as a sign that my basic code structure is in need of some improvement.

I'm trying to let my models handle all my data. Here is the main model:

- (instancetype)initVenueServiceWithClientID:(NSString *)clientID clientSecret:(NSString *)clientSecret
{
    if (self = [super init]) {
        _clientID = clientID;
        _clientSecret = clientSecret;
        _name = @"name";
        _pluralName = @"pluralName";
        _categories = @"categories";
        _identifier = @"id";
        _location = @"location";
        _radius = @"2000";
        _requestString = @{@"venueCategories" : @"https://api.foursquare.com/v2/venues/categories?client_id=%@&client_secret=%@&v=%@",
                          @"venueBasedOnLocation" : @"https://api.foursquare.com/v2/venues/search?ll=%f,%f&radius=%@&categoryId=%@&client_id=%@&client_secret=%@&v=%@",
                          @"venueImageURLs" : @"https://api.foursquare.com/v2/venues/%@?client_id=%@&client_secret=%@&v=%@"};
        NSDate *date = [NSDate date];
        NSDateFormatter *formatter = [[NSDateFormatter alloc] init];
        [formatter setDateFormat:@"yyyMMdd"];

        _todaysDate = [formatter stringFromDate:date];
    }
    return self;
}


- (NSURLRequest *)URLRequestWithFormat:(NSString *)attributes, ...
{
    va_list arguments;

    va_start(arguments, attributes);
    NSString *urlPath = [[NSString alloc] initWithFormat:_requestString[attributes] arguments:arguments];
    va_end(arguments);

    NSURL *url = [NSURL URLWithString:urlPath];
    NSURLRequest *request = [NSURLRequest requestWithURL:url];

    return request;
}

- (void)performRequest:(NSURLRequest *)request withKeyPath:(NSString *)keyPath completion:(TNVenueServiceCompletionBlock)completion
{
    dispatch_queue_t queue = dispatch_get_global_queue(DISPATCH_QUEUE_PRIORITY_DEFAULT, 0);
    dispatch_async(queue, ^{
        NSError *error;
        NSURLResponse *response;

        NSData *data = [NSURLConnection sendSynchronousRequest:request returningResponse:&response error:&error];

        NSDictionary *responseDictionary = [NSJSONSerialization JSONObjectWithData:data options:kNilOptions error:&error];
        _venueData = [responseDictionary valueForKeyPath:keyPath];
        if (!error) {
            completion(_venueData, nil);
        } else {
            completion(nil, error);
        }
    });
}

And the model where I store the categories I get from my JSON request:

- (instancetype)initCategoriesWithIdentifier:(NSString *)identifier
                                  pluralName:(NSString *)pluralName
                                  categories:(NSString *)categories
{
    if (self = [super init]) {
        _identifier = identifier;
        _pluralName = pluralName;
        _categories = categories;
    }
    return self;
}

The implementation in my viewController then looks something like this:

_venueService = [[TNVenueService alloc] initVenueServiceWithClientID:@"MY_CLIENT_ID" clientSecret:@"MY_CLIENT_SECRET"];

    NSURLRequest *request = [_venueService URLRequestWithFormat:@"venueCategories",
                             _venueService.clientID, _venueService.clientSecret, _venueService.todaysDate];

    [_venueService performRequest:request withKeyPath:@"response.categories" completion:^(NSArray *array, NSError *error) {
        if (!error) {
            NSMutableArray *tempArray = [NSMutableArray arrayWithCapacity:[array count]];
            for (NSDictionary *data in array) {
                TNVenueCategories *category = [[TNVenueCategories alloc] initCategoriesWithIdentifier:data[_venueService.identifier]
                                                                                           pluralName:data[_venueService.pluralName]
                                                                                           categories:data[_venueService.categories]];
                [tempArray addObject:category];
            }
            _storedCategories = [NSArray arrayWithArray:tempArray];
            dispatch_async(dispatch_get_main_queue(), ^{
                [self.tableView reloadData];
            });
        }
    }];

Sure, this works. But am I on the right track? What could I do differently to make it better?

\$\endgroup\$
3
\$\begingroup\$

Right now this is just a quick answer with some first impressions I notice before I get to work. I'll edit and improve this answer later.

First of all, I don't like what's going on in your init method. We're wasting time initializing a handful of strings to literal values. And this is done for every initialization.

These instead should be constants available to the class, so at the top of the .m, before the interface or implementation, define these constants as such:

NSString * const kName = @"name";
NSString * const kPluralName = @"pluralName";

Etc., etc., on down the line.


This can also be done with the 6 strings that make up the keys and values in the dictionary, although it cannot be done with the dictionary itself. You still stop from creating a new dictionary every time the class is instantiated and create the dictionary only when needed when the Objective-C worked around to having a class level variable (instead of instance variable). The pattern looks like this:

+ (NSDictionary *)requestStringDictionary {
    static NSDictionary *requestStringDictionary;
    @synchronized(self) {
        if (!requestStringDictionary) {
            requestStringDictionary = @{kKEY_venueCategories      : kVenueCategories,
                                        kKEY_venueBasedOnLocation : kVenueBasedOnLocation,
                                        kKEY_venueImageURLs       : kVenueImageURLS};
        }
    }
    return self;
}

So, now when you need this dictionary, you just call [[self class] requestStringDictionary];

The method itself checks whether or not requestStringDictionary points to nil. If it does, it creates a new dictionary, which requestStringDictionary points to (and no longer point to nil). Otherwise, it returns requestStringDictionary, which points to this very same dictionary. The code inside the if block can be called multiple times if the dictionary is ever deallocated, but it will only ever be called again after it's been deallocated.

The @synchronized(self) ensures thread safety.


And finishing off the init method, I really don't like what we're doing with todaysDate.

Why do we need to lock in the date right now, when the object is initialized? The only reason to lock in initialization date would be for debugging purposes, and at that, time seems way more useful... and either way, simply NSLogging will give us that information. I don't see how initialization date is important here (and you haven't provided complete code, because I see it used no where).

Moreover, if we must keep the date, why do we have to have it as a string representation in init? I don't understand this either. If you really must save the initialization time stamp, you need a variable called "creationDate" or "initializationDate" or something, and it should be an NSDate object, and you should just set it to [NSDate date] in init.


In your performRequest:withKeyPath:completion: method, the following pattern is a bit silly:

if (!error) {
    completion(_venueData, nil);
} else {
    completion(nil, error);
}

First of all, if !error returns true, that means error is nil. So in this case completion(_venueData, nil); and completion(_venueData, error); are literally the exact same thing.

As for whether or not to pass _venueData, I don't know enough about NSJSONSerialization to know for sure, but my assumption would be that if error is not nil here, then _venueData is nil. And if this is definitely the case, then we can definitely just write:

completion(_venueData,error);

Rather than the if else block. But even if it isn't the case that _venueData will always be nil when there's an error, we should STILL call the completion block in this manner.

And why? Because everyone will use the completion block just as you are. It's going to start with if (!error). We're going to do this error check in our completion block. If you want to check it before you dispatch the completion block, then don't offer it as an argument to the completion block--it will be double checked every time.

\$\endgroup\$
  • \$\begingroup\$ like you said, I didn't provide the full code, but todaysDate is used in the viewController-bit when I make create request. Anyway I see your point. Thanks for your help :) \$\endgroup\$ – Nilzone- May 14 '14 at 14:07
  • \$\begingroup\$ I will improve my model as much as possible and edit my question \$\endgroup\$ – Nilzone- May 14 '14 at 15:43

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.