Model to handle all the data/networking from Foursquare API in IOS

I posted not too long ago, asking for tips/improvements I could use on my models. I have changed quite a lot, so I thought I'll give it another go.

My model is supposed to handle all the data I get from the Foursquare API:

venueService.h

typedef void (^TNVenueServiceCompletionBlock)(UIImage *image, NSError *error);

@class TNVenueImageData;

@interface TNVenueService : NSObject

@property (nonatomic) NSString *clientID;
@property (nonatomic) NSString *clientSecret;
@property (nonatomic) NSString *radius;

@property (nonatomic) NSArray *venueObject;
@property (nonatomic) NSArray *imageData;

+ (instancetype)venueService;

- (NSDictionary *)mainCategoryKeys;
- (NSArray *)mainCategories;
- (void)performVenueLocationRequest:(CLLocation *)location identifier:(NSString *)identifier;
- (void)performPhotoDetailsRequest:(NSString *)identifier;
- (void)downloadImage:(TNVenueImageData *)imageData completionBlock:(TNVenueServiceCompletionBlock)completion;

@end

venueService.m

```
@implementation TNVenueService

+ (instancetype)venueService
{
TNVenueService *venueService = [[TNVenueService alloc] init];
venueService.clientID = @"MY_CLIENT_ID";
venueService.clientSecret = @"MY_CLIENT_SECRET";
venueService.radius = @"2000"; //start-radius

return venueService;
}

• (NSURLRequest )URLRequestWithFormat:(NSString )attributes, ...

{
va_list arguments;

va_start(arguments, attributes);
NSString *urlPath = [[NSString alloc] initWithFormat:attributes arguments:arguments];
va_end(arguments);
NSURL *url = [NSURL URLWithString:urlPath];
NSURLRequest *request = [NSURLRequest requestWithURL:url];

return request;
}

• (void)performVenueLocationRequest:(CLLocation )location identifier:(NSString )identifier

{
dispatch_queue_t queue = dispatch_get_global_queue(DISPATCH_QUEUE_PRIORITY_DEFAULT, 0);
dispatch_async(queue, ^{
NSError *error = nil;
NSURLResponse *response = nil;

NSData *data = [NSURLConn

First, I'll focus on some of the easy, obvious things I see. I know the main point of this code is some of the networking stuff, but there are some other things I want to comment on first.

First and foremost, this line:

@class TNVenueImageData;

It's fine to declare the class in the .h and wait until the .m to import the file... but you must have clipped off some lines, because I don't see the actual import anywhere.

But at the end of the day, it doesn't actually hurt to import the file in the .h. After all, anyone who imports this file is likely going to also import the other file. Double importing doesn't hurt (it's only imported once, no matter how many times you try). And now if I'm using this class, I could just import venueService and not have to import the image data file, because the venue service file is importing it for me.

Next, what's this stuff exactly?

venueService.clientID = @"MY_CLIENT_ID";
venueService.clientSecret = @"MY_CLIENT_SECRET";
venueService.radius = @"2000"; //start-radius

Surely "MY_CLIENT_..." can't be the actual values here. But the method is simply venueService and takes no arguments, so these are definitely constant values. They're also public properties.

So, first of all, we can start by defining these as constants within the .m file.

Outside of the @interface or @implementation, up near the imports, define some constants:

static NSString * const kVenueServiceClientID = @"MY_CLIENT_ID";
static NSString * const kVenueServiceClientSecret = @"MY_CLIENT_SECRET";
static NSString * const kVenueServiceRadius = @"2000";

Now if you need to use these values within the class, simple call the constant variable.

If you want to use the variables outside the class, you can create readonly properties for each.

In the .h, set up your properties as such:

@property (nonatomic,readonly) NSString *clientID;
@property (nonatomic,readonly) NSString *clientSecret;
@property (nonatomic,readonly) NSString *radius;

Now in the .m, overwrite the default accessor for each:

- (NSString *)clientID {
    return kVenueServiceClientID;
}

And repeat for the other two.

Now, as for these methods: mainCategories and mainCategoryKeys, I have a handful of problems.

The first problem is that the method names are confusing from an Objective-C standpoint. mainCategoryKeys returns a dictionary of values... while mainCategories returns an array which confusingly enough happens to be the same values you'd get if you called allKeys on the dictionary returned by mainCategoryKeys.

I assume the strange values in the value part of the dictionary are keys used with the API.

So, first of all, let's see what we can do to fix the method names here.

The method that returns a dictionary returns a dictionary, so how about: mainCategoryKeyDictionary, which makes it more clear that the values are actually keys to something else.

As for the other method, some name options include mainCategoryNames, or mainCategoryDescriptions, but honestly, the fact that an identical array can be produced simply by calling allKeys on the dictionary makes me think this method is kind of extraneous.

Now that we've got them renamed, I there are still other problems. First, constants, again. The same logic as above applies.

But the bigger issue I see here is that this should probably be a class level method rather than an instance method. In another language like Java, this might not even be a method, but just a public static variable. Objective-C doesn't offer class level variables, but there's a commonly used pattern to achieve the same effect in Objective-C. It looks like this:

+ (NSDictionary *)mainCategoryKeyDictionary {
    @synchronized(self) {
        static NSDictionary *keyDictionary;
        if (!keyDictionary) {
            keyDictionary = @{ /*stuff*/ };
        }
        return keyDictionary;
    }
}

Where /stuff/ is replace with the actual keys and values of course.

This is more efficient in terms of speed and memory usage.

As for the methods that do the actual networking, NSNotificationCenter is the 3rd best option out of 3 ways I know for doing what you're trying to do with it.

If you are going to use NSNotificationCenter, there's two important things you need to do with the notification name. First, use the reverse domain format for the actual name.

com.example.yourNotificationsActualName

The same naming scheme, by the way, needs to be applied to any threads you create. You don't want to accidentally register for some other notification that coincidentally has the same name you picked... and reverse domain naming is a good way to avoid that.

Second of all, these notification names absolutely must be defined constants available to anyone that has imported the .h for this file. How else will they know what notifications to register for when using your code?

But ultimately, these are

StackExchange Code Review Q#51158, answer score: 4