remote data fetching inside model object in objective c using AFNetworking

Question

In all of my iOS application I use this approach to respect MVC, I want to be sure that my implementation is correct and respects the best practices and the MVC design pattern :

Singleton of AFNetworking acting as API for network calls:

MyAPI.h :

#import "AFHTTPSessionManager.h"
#import "AFNetworking.h"

@interface MyAPI : AFHTTPSessionManager

+(MyAPI *)sharedInstance;

@end

MyAPI.m :

#pragma mark - Singleton

+(MyAPI*)sharedInstance
{
 static MyAPI *sharedInstance = nil;
 static dispatch_once_t onceToken;
 dispatch_once(&onceToken, ^{
    sharedInstance = [[MyAPI alloc] initWithBaseURL:[NSURL URLWithString:kROOT_URL]];
});
  return sharedInstance;
}

Model User that uses the singleton to fecth the data of user (is that good as implementation ?):

User.h

 @interface User : NSObject

 @property (strong,nonatomic) NSString *userId;
 @property (strong,nonatomic) NSString *email;
 @property (strong,nonatomic) NSString *password;


-(id) initWithDictionary: (NSDictionary *) dictionay;

 +(BOOL) isConnected;
 +(void) disconnect;
 +(NSString *) idOfConnectedUser;
 +(User *) connectedUser;

 +(void) loginWith : (NSString *) email andPassword :(NSString *) password complete:(void(^)(id result, NSError *error))block;
 +(void) searchUsersFrom : (NSString *) countryCode withName :(NSString *) name andLevel:(NSString *) levelCode complete: (void(^)(id result, NSError *error)) block;
 +(void) signup:(void(^)(id result, NSError *error)) block;
 +(void) getUserFriends:(void(^)(id result, NSError *error)) block;

@end

User.m

  [......]

 +(void) loginWith : (NSString *) email andPassword :(NSString *) password complete: (void(^)(id result, NSError *error)) block
 {

 __block NSString * result ;

NSDictionary *params = @{@"email": email, @"password": password};

[[MyAPI sharedInstance] POST:@"auth/" parameters:params success:^(NSURLSessionDataTask *task, id responseObject)
{

    if([responseObject objectForKey:@"id"])
    { 
        [[NSUserDefaults standardUserDefaults] setObject:(NSDictionary*) responseObject forKey:USER_KEY];
        [[NSUserDefaults standardUserDefaults] synchronize];
        result = [responseObject objectForKey:@"id"];
    }
    else
    {
        result = nil ;
    }


    if (block) block(result, nil);

} failure:^(NSURLSessionDataTask *task, NSError *error)
{
     if (block) block(nil, error);
}];

}
[.....]

LoginController.m :

-(void)loginButtonAction:(UIButton *)sender
{

    [......]

    [ User loginWith:text andPassword:text complete:^(id result, NSError *error)
     {
         if (result)
         {
             [APPDELEGATE start];
         }
         else
         {
          // ERROR
         }
       }];

   }

So does my implementation respects the MCV and follows the best practices and how can I improve it if not ?

Answer 1

Singletons: You might want to avoid using singletons, it'll help you to improve your API design and make code more testable. Also, in case of User, imagine you will want to support changing user (logout/guest user/etc). With current approach, you will be limited to sending a NSNotification because everyone who uses connectedUser can not know that underlying reference has changed.

ActiveRecord: What you did with your model User that is capable of performing networking is somewhat similar to active record approach which might not scale so well when you model becomes more complicated and the number of actions it can perform increases. Consider separating those into pure model and services that actually perform networking (or whatever else you will need in the future).

Model Serialisation: Consider encapsulating model & network response serialisation logic into a separate class (e.g. LoginResponse that among other things points to a User) frameworks like Mantle make it much easier.

MVC: from my experience in iOS MVC might not be the most optimal approach for anything but simple apps. With MVC tendency is to put put all the logic into your ViewController making it very big and hard to maintain. Consider other patterns such as MVVM

All in all I understand that it is hard to learn all the new technologies at once, but you can definitely start by making sure each class performs one thing and one thing only: Model does not do networking or persisting to the disk, API client doesn't deserialise each response or saves data to NSUserDefaults, view controller doesn't do anything except for listening to user events (button taps etc). This alone would make your code much easier to reason about and to follow if a new developer would be introduced to your codebase.

Hope it helps!

Answer 2

I dont have anything to say about your MVC(Model–view–controller) correct?

I just want to add something that may be useful approach avoiding unwanted crashes..

First is under

[[MyAPI sharedInstance] POST:@"auth/" parameters:params success:^(NSURLSessionDataTask *task, id responseObject) 
{
    if([responseObject objectForKey:@"id"])
    { 
        [[NSUserDefaults standardUserDefaults] setObject:(NSDictionary*) responseObject forKey:USER_KEY];
        [[NSUserDefaults standardUserDefaults] synchronize];
        result = [responseObject objectForKey:@"id"];
    }
    else
    {
        result = nil ;
    }
}];

it is always possible for so many reason to mention that the reponseObject could be nil and therefore object dont have the key @"id" and will lead to error(crash in worst case). i have this code i dont know if this can be considered as best practice but here it is:

if ([responseObject isKindOfClass:[NSArray class]])
{
    NSLog(@"Log: Response is of class NSArray");
}
else if ([responseObject isKindOfClass:[NSDictionary class]])
{
    NSLog(@"Log: Response is of class NSDictionary");
}
else 
{
    NSLog(@"Log: Kind of class is not supported");
}

This example restricts other kind of class especially [NSNull class]

Second in line:

NSDictionary *params = @{@"email": email, @"password": password};

by checking email and password first before assigning to NSDictionary, setting nil to NSDictionary will cause crash.

Third is line:

if (block) block(result, nil);

block returns void from your implementation. Is this working? Sorry for asking i haven't tried if-statement with a block like this..

complete: (void(^)(id result, NSError *error)) block

void in this is the returning value of your block, or i'm wrong.. hmm..

if (block) only checks if the block block exist, so intead of checking it (which we are sure that is existing)..

maybe you want to check the result instead...

if (result != nil) block(result, nil); is the right statement

the reason behind is:

if (result != nil) // will return NONE nil value only
{
    block(result, nil); 
}

// else will not set things to the block

//or maybe just 

block(result, nil); // which will allow the 'block(nil, nil);' and under your implementation

[ User loginWith:text andPassword:text complete:^(id result, NSError *error)
{
     if (result)
     {
         [APPDELEGATE start];
     }
     else if (result == nil & error == nil)
     {
         // NO objectForKey @"id"
     }
     else
     {
         // ERROR
     }
}];

while under failure:^(NSURLSessionDataTask *task, NSError *error) simply block(nil, error);