4
\$\begingroup\$

I just finished a model class that represents an Instagram Networking client for my iOS app.

I am specifically looking for a review of my use of static constants. These are strings that represent notification names and notification userInfo dictionary keys.

Here is my networking class:

POPInstagramNetworkingClient.h:

#import "AFHTTPSessionManager.h"
@interface POPInstagramNetworkingClient : AFHTTPSessionManager

+ (id)sharedPOPInstagramNetworkingClient;

- (instancetype)initWithBaseURL:(NSURL *)url;
- (void)requestPopularMedia;
- (void)requestMediaWithTag:(NSString *)tag;

@end

POPInstagramNetworkingClient.m:

#import "POPInstagramNetworkingClient.h"
#import <AFNetworking.h>

@implementation POPInstagramNetworkingClient

#pragma mark - Singleton
+ (instancetype)sharedPOPInstagramNetworkingClient
{
    static POPInstagramNetworkingClient *sharedPOPInstagramNetworkingClient = nil;
    static dispatch_once_t onceToken;
    dispatch_once(&onceToken, ^{

        //Define base URL string
        static NSString * const BaseURLString = @"https://api.instagram.com/v1/";

        //Create our shared networking client for Instagram with base URL
        sharedPOPInstagramNetworkingClient = [[POPInstagramNetworkingClient alloc]initWithBaseURL:[NSURL URLWithString:BaseURLString]];
    });

    return sharedPOPInstagramNetworkingClient;
}

#pragma mark - Initializer Methods
- (instancetype)initWithBaseURL:(NSURL *)url
{
    self = [super initWithBaseURL:url];

    if (self) {
        //Set the serializers
        self.requestSerializer = [AFJSONRequestSerializer serializer];
        self.responseSerializer = [AFJSONResponseSerializer serializer];
    }

    return self;
}

#pragma mark - Instance Methods
- (void)requestPopularMedia
{
    //Define notification names
    static NSString * const kRequestForPopularMediaSuccessful = @"RequestForPopularMediaSuccessful";
    static NSString * const kRequestForPopularMediaUnsuccessful = @"RequestForPopularMediaUnsuccessful";

    //Create manager and execute GET method to retreive popular media
    AFHTTPRequestOperationManager *manager = [AFHTTPRequestOperationManager manager];
    [manager GET:[NSString stringWithFormat:@"%@media/popular?client_id=76566d0e6d5a41069ea5e8c86fbbd509", self.baseURL] parameters:nil success:^(AFHTTPRequestOperation *operation, id responseObject) {

        //Post success notification
        [[NSNotificationCenter defaultCenter]postNotificationName:kRequestForPopularMediaSuccessful object:nil userInfo:@{@"requestForPopularMediaResults": responseObject}];

    } failure:^(AFHTTPRequestOperation *operation, NSError *error) {
        NSLog(@"Error: %@", error);

        //Post failure notificaton
        [[NSNotificationCenter defaultCenter]postNotificationName:kRequestForPopularMediaUnsuccessful object:nil userInfo:@{@"requestForPopularMediaResults": error}];

    }];
}

- (void)requestMediaWithTag:(NSString *)tag
{
    NSLog(@"requesting media with tag");

    //Define notification names
    static NSString * const kRequestForMediaWithTagSuccessful = @"RequestForMediaWithTagSuccessful";
    static NSString * const kRequestForMediaWithTagUnsuccessful = @"RequestForMediaWithTagUnsuccessful";

    //new - @"%@tags/%@/media/recent?client_id=76566d0e6d5a41069ea5e8c86fbbd509"
    //old - @"%@tags/search?q=%@&client_id=76566d0e6d5a41069ea5e8c86fbbd509"

    //Create manager and execute GET method to retreive media with tag
    AFHTTPRequestOperationManager *manager = [AFHTTPRequestOperationManager manager];
    [manager GET:[NSString stringWithFormat:@"%@tags/%@/media/recent?client_id=76566d0e6d5a41069ea5e8c86fbbd509", self.baseURL, tag] parameters:nil success:^(AFHTTPRequestOperation *operation, id responseObject) {

        NSLog(@"Success!");
        NSLog(@"Response Object: %@", responseObject);

        //Post success notification
        [[NSNotificationCenter defaultCenter]postNotificationName:kRequestForMediaWithTagSuccessful object:nil userInfo:@{@"requestForTaggedMediaResults": responseObject}];

    } failure:^(AFHTTPRequestOperation *operation, NSError *error) {
        NSLog(@"Error: %@", error);

        //Post failure notification
        [[NSNotificationCenter defaultCenter]postNotificationName:kRequestForMediaWithTagUnsuccessful object:nil userInfo:@{@"requestForTaggedMediaResults": error}];
    }];
}

@end

I was told previously that when defining static variables I should keep their scope as limited as possible, so you will notice that I defined these inside of specific methods instead of at the top of this file.

One thing that is bothering me though is the fact that I have other classes observing for these notifications and also accessing the passed userInfo dictionary when the notification posts. I basically just copy/pasted the notification name's static string and userInfo dictionary key string.

This is definitely wrong, and I feel like these variables might need to be placed into a single file that would be included in the precompiled header, but I'm not sure if this is the correct approach so any advice on this would be greatly appreciated.

I want to make sure I am following best practices.

\$\endgroup\$
  • 1
    \$\begingroup\$ Eventually, when I have some more time, I will post an answer that covers why you shouldn't be using NSNotificationCenter for this. \$\endgroup\$ – nhgrif Aug 26 '14 at 21:45
  • \$\begingroup\$ @nhgrif I don't want you to think I haven't been listening. I actually plan on using delegates exclusively from here on out, and I also want to start playing around with chained blocks. The only reason I used notifications is because the "client" requested it. \$\endgroup\$ – user3344977 Aug 26 '14 at 22:01
4
\$\begingroup\$
//Define notification names
static NSString * const kRequestForPopularMediaSuccessful = @"RequestForPopularMediaSuccessful";
static NSString * const kRequestForPopularMediaUnsuccessful = @"RequestForPopularMediaUnsuccessful";

The scope of a notification name should never be limited to a single method or function.

The whole point of notification is that one object posts the notification, and as many objects who are registered can receive the notification.

Moreover, you've defined these identical constants in multiple methods.

So, first of all, if you use it in more than one method, it variable should at least be scoped to the class or file you're using it in. Redefining it every time you use it absolutely defeats the purpose of having a variable at all.

Second, this variable actually needs to be defined in the .h file. It's a notification name. If other objects are going to register for this notification, they need to know the name of the notification, and the best way to let them know the notification name is by giving them a variable in the .h file to use rather than having them look up the notification string.


//Define base URL string
static NSString * const BaseURLString = @"https://api.instagram.com/v1/";

//Create our shared networking client for Instagram with base URL
sharedPOPInstagramNetworkingClient = [[POPInstagramNetworkingClient alloc]initWithBaseURL:[NSURL URLWithString:BaseURLString]];

In this case, since we're only ever using this string once, it's fine to just use the literal string directly in the method call.

The point of declaring a variable like this would be if we reused it multiple times within the method--as it stands, we don't re-use it, so we don't need to create the variable.

Though... if we do create a variable, it should follow proper camelCase naming conventions.

| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ I don't want you to think I haven't been listening. I actually plan on using delegates exclusively from here on out, and I also want to start playing around with chained blocks. The only reason I used notifications is because the "client" requested it. \$\endgroup\$ – user3344977 Aug 26 '14 at 22:05
  • \$\begingroup\$ "Moreover, you've defined these identical constants in multiple methods." - Sorry I don't follow. If you look at the constants in both methods you will see that they have similar, but different names or am I misunderstanding? \$\endgroup\$ – user3344977 Aug 26 '14 at 22:06
  • \$\begingroup\$ "Second, this variable actually needs to be defined in the .h file." - How should I do this? Readonly property in .h and readwrite in .m? \$\endgroup\$ – user3344977 Aug 26 '14 at 22:07
  • \$\begingroup\$ You're right--they are different variables. You can just cut and paste them as they are into the .h. \$\endgroup\$ – nhgrif Aug 26 '14 at 22:13
  • \$\begingroup\$ Do you have any input on my third comment? \$\endgroup\$ – user3344977 Aug 26 '14 at 22:29

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.