App for making calls to web services - AFNetworking 2.0

Question

I've built a base class that I use a lot in my iOS app to make calls to web services. I built the base class to make the actual call and this base class is only ever used by another class.

The idea was that should I decide to change from AFNetworking to something else, I would not need to rewrite code in many other classes and only need to change it in one class.

The rest of my app has no idea how the web service calls are actually made - it just know how to request them. That's it.

I've been thinking of late that maybe I could improve the code.

Here is the base class:

The header file:

BBWebService.h

#import <Foundation/Foundation.h>


@interface BBWebService : NSObject



#pragma mark - Properties 

@property (nonatomic) BOOL isBusy;
@property (assign) int webServiceId; 
@property (weak, nonatomic) id <NSObject, WebServiceDelegate> delegate; 

#pragma mark - Methods
/**
 This method is not allowed to instainsiate this class - cannot setup class correctly
 */

-(instancetype) init __attribute__((unavailable("Please use this classes designated initilizer initWithUrl")));


/** 
 Creates and runs an `NSURLSessionDataTask` with a HTTPS POST request.
 @param initWithURL The URL string used to create the request URL.
 @param RequestType HTTP request type - "POST" / "PUT" supported for this method only.
 @param UrlParameters  The parameters to be encoded according to the client request serializer.
 @param PostDataValuesAndKeys POST data (NSDictionary) 

 */

- (id) initWithURL: (NSString*) url RequestType: (NSString*) requestType PostDataValuesAndKeys: (NSDictionary*) postData  UrlParameters: (NSDictionary*) urlParameters;



/**
 Creates and runs an `NSURLSessionDataTask` with a HTTP request.

 @param initWithURL The URL string used to create the request URL.
 @param RequestType HTTP request type - "GET" / "DELETE" supported for this method only. 
 @param UrlParameters  The parameters to be encoded according to the client request serializer.

 */

- (id) initWithURL: (NSString*) url RequestType: (NSString*) requestType UrlParameters: (NSDictionary*) urlParameters;



@end

BBWebservice.m

-(instancetype)initWithURL:(NSString *)url RequestType:(NSString *)requestType  UrlParameters:(NSDictionary *)urlParameters
{

    [self getSessionManager];

    if ([requestType isEqualToString:@"GET"]){

        self.isBusy = YES;
        [self.manager GET:url parameters:urlParameters
                  success:^(NSURLSessionDataTask *task, id responseObject){

                      NSHTTPURLResponse *response = (NSHTTPURLResponse *)task.response;

                      int statusCode = (int)response.statusCode;

                      [self requestDone:responseObject StatusCode:statusCode];

                  }failure:^(NSURLSessionDataTask *task, NSError *error){

                      NSHTTPURLResponse *response = (NSHTTPURLResponse *)task.response;

                      int statusCode = (int)response.statusCode;
                      [self requestFailed:error StatusCode:statusCode];

                  }];

        //--** DELETE Request --**//

    }else if ([requestType isEqualToString:@"DELETE"]){
        self.isBusy = YES;


        [self.manager DELETE:url parameters:urlParameters success:^(NSURLSessionDataTask *task, id responseObject) {

            NSHTTPURLResponse *response = (NSHTTPURLResponse *)task.response;

            int statusCode = (int)response.statusCode;

            [self requestDone:responseObject StatusCode:statusCode];

        } failure:^(NSURLSessionDataTask *task, NSError *error) {

            NSHTTPURLResponse *response = (NSHTTPURLResponse *)task.response;

            int statusCode = (int)response.statusCode;
            [self requestFailed:error StatusCode:statusCode];
        }];
    }

    self.isBusy = NO;
    return self;

}


-(instancetype)initWithURL:(NSString *)url RequestType:(NSString *)requestType PostDataValuesAndKeys:(NSDictionary *)postData UrlParameters:(NSDictionary *)urlParameters
{
    [self getSessionManager];
    self.isBusy = YES;

    [self.manager POST:url parameters:urlParameters
              success:^(NSURLSessionDataTask *task, id responseObject){

                  NSHTTPURLResponse *response = (NSHTTPURLResponse *)task.response;

                  int statusCode = (int)response.statusCode;

                  [self requestDone:responseObject StatusCode:statusCode];

              }failure:^(NSURLSessionDataTask *task, NSError *error){

                  NSHTTPURLResponse *response = (NSHTTPURLResponse *)task.response;

                  int statusCode = (int)response.statusCode;
                  [self requestFailed:error StatusCode:statusCode];

              }];


    return self;
}

-(void) requestDone: (id)responseObject StatusCode: (int)statusCode
{

    self.isBusy = NO;

    if ([responseObject isKindOfClass:[NSData class]]){

        if (statusCode == 200)
            {
            NSData *data = [[NSData alloc]initWithData:responseObject];

            if (self.delegate)
                {
                if ([self.delegate respondsToSelector:@selector(webService:result:)])
                    {
                        [self.delegate webService:self result:data];
                    }
                else
                    {

                        [self.delegate result:data WebServiceId:self.webServiceId];
                    }
                }
            }
        else
            {
            if ([self.delegate respondsToSelector:@selector(httpError:WebServiceId:ErrorCode:Message:)])
                {
                    [self.delegate httpError:nil WebServiceId:self.webServiceId ErrorCode:statusCode Message:@"Web Service Failed!"];
                }
            else if ([self.delegate respondsToSelector:@selector(webService:httpError:ErrorCode:Message:)])
                {
                    [self.delegate webService:self httpError:nil ErrorCode:statusCode Message:@"Web Service Failed"];
                }
            }
    }
}

-(void)requestFailed: (NSError *)errorMessage StatusCode: (int)statusCode

{
    self.isBusy = NO;

    NSLog (@"Request failed - Status Code: %d Error: %@", statusCode, errorMessage);

    if (statusCode == 0){

        UIAlertView *offlineAlertView = [[UIAlertView alloc]initWithTitle:@"" message:@"Your internet connection appears to be offline. Please check your settings and try again" delegate:self
                                                        cancelButtonTitle:@"Okay" otherButtonTitles:nil, nil];

        [offlineAlertView show];
    }
    if ([self.delegate respondsToSelector:@selector(webService:connectionError:)])
        {
            [self.delegate webService:self connectionError: @"Connection Error!"];
        }
    else
        {
            //Add extra handling here
        }



}

-(AFHTTPSessionManager *)getSessionManager
{
    self.manager = [AFHTTPSessionManager manager];
    self.manager.responseSerializer = [AFHTTPResponseSerializer serializer];
    return self.manager;
}

@end

As you can see - BBWebService is used to make HTTP POST / GET / DELETE requests. (I need to modify it for PUT requests at some stage)

This class has not given me any issues and has worked just fine.

So to use these methods - I use another class to do the actual request that adds the URL / Dictionary parameters.

Edit:

The protocols

It's just a header file that has these declarations:

#import <Foundation/Foundation.h>

@protocol WebServiceDelegate

@optional

-(void) result:(id) result WebServiceId: (int) webServiceId;
-(void) connectionError:(id) result WebServiceId: (int) webServiceId;
-(void) generalError:(id) result WebServiceId: (int) webServiceId;
-(void) httpError:(id) result WebServiceId: (int) webServiceId ErrorCode: (int) errorCode Message: (NSString *) errorMessage;

-(void) webService:(id) webService result:(id) result;
-(void) webService:(id) webService connectionError:(id) result;
-(void) webService:(id) webService generalError:(id) result;
-(void) webService:(id) webService httpError:(id) result ErrorCode: (int) errorCode Message: (NSString *) errorMessage;

@end

I use the -(void) result:(id) result WebServiceId: (int) webServiceId;

In another class like so:

    #pragma mark - BBWSDelegate methods

-(void)result:(id)result WebServiceId:(int)webServiceId
{

    if (![result isKindOfClass:[NSData class]]){

    @throw [NSException exceptionWithName:NSInternalInconsistencyException reason:@"Result was not of type NSData!" userInfo:nil];

    }
}

This is in My web service class and is designed to stop execution if for some reason I did not get an NSData object back.

I then reuse the -(void) result:(id) result WebServiceId: (int) webServiceId;

Again in my XML parsing class - like so:

    -(void)result:(id)result WebServiceId:(int)webServiceId
{
    if ([result isKindOfClass:[NSData class]]){

        NSData *responseData = result;
        NSString *xmlData = [[NSString alloc] initWithData:responseData encoding:NSUTF8StringEncoding];

                NSLog (@" BBaseXmlWebService: XML Data Received - %@", xmlData);

        self.tbxml = [TBXML newTBXMLWithXMLData:responseData error:nil];

            //**-- Start parsing --**//

            [self parseResult];


        if (self.delegate)
            {

                [self.delegate result:self.baseResult WebServiceId:webServiceId];

            }
    }else
        {

            @throw [NSException exceptionWithName:NSInternalInconsistencyException reason:@"Result was not of type NSData!" userInfo:nil];
    }
}

And once again in my data class

    -(void)result:(id)result WebServiceId:(int)webServiceId
{

    [self webServiceEnded:webServiceId];

    if (![self handleWebServiceResult:result WebServiceId:webServiceId])
        {
            // do the default and store in the cache
        NSNumber* webServiceIdNumber = [NSNumber numberWithInt:webServiceId];

        [self.dataCache setObject:result forKey:webServiceIdNumber];

        }

        // update the last updated time in the cache

    NSTimeInterval time = [NSDate timeIntervalSinceReferenceDate];
    NSNumber* timeNum = [NSNumber numberWithDouble:time];
    NSNumber* webServiceIdNumber = [NSNumber numberWithInt:webServiceId];


    [self.dataLastUpdateTime setObject:timeNum forKey:webServiceIdNumber];

        // notify with success
    [self notify:webServiceId Result:NOTIFICATION_WEBSERVICE_RESULT_SUCCESS];
}

This way, I get valid, parsed data back into my data class ready to use in the app where I need it.

Edit 2

This is to answer some of nhgrif's questions as my reply is too long for comments.

The class needs to have a delegate as that is how I let another class know the request has been completed.

The second problem you mentioned with the init methods; the reason for the init method making the request is that is all I ever do with this class. Here is an example of how I would use this class:

-(void)fetch:(NSDictionary *)urlParameters
{
    if (!self.webService.isBusy){

        BBWebService *webService = [[BBWebService alloc]initWithURL:self.url RequestType:GET_REQUEST UrlParameters:urlParameters];

        self.webService = webService;
        self.webService.webServiceId = wsID;
        [self.webService setDelegate:self]; 

    }

}

In this class (ClassB) where I use this method - I initialize the BBWebService class with the URL and parameters I need. I also set the delegate for the BBWebService class here in ClassB at the same time. ClassB conforms to the protocols and once BBWebService class has completed the request - ClassB takes over and handles them - then hands it off to my XML class which parses it and then it moves up the chain to my data store class where I use the parsed data.

This is why I thought it would be best to make sure the BBWebService class could not be setup with just alloc / init as then you would be using it incorrectly.

Maybe there is a better approach to this?

Answer 1

I REALLY like this:

-(instancetype) init __attribute__((unavailable("Please use this classes designated initilizer initWithUrl")));

I follow this pattern myself a lot. But... is it okay for your class to not have a delegate? I see that the code will survive, but does it ever make sense for this class to be undelegated when instantiated? If not, I suggest the message point the user instead to a initWithDelegate: method and take the delegate during initialization.

---

Your init methods have several problems.

First problem is you're using property accessors in your init methods, which you should never do. Apple themselves recommend directly against this.

Second problem for me is that your init methods are doing way more than initializing. They're starting the request! And they're doing so without even giving me the chance to set a delegate!

Methods should do one thing, and their method name should reflect that one thing that they do.

And in Objective-C, methods in the init family have a very clear scope: setup an object and prepare it to be used. Moreover, all init methods should call either [super init] or call another init method within the same class (which calls super init itself).

The pattern for Objective-C init methods is very clear.

- (instancetype)initWithFoo:(Foo *)foo bar:(Bar *)bar {
    self = [super init];
    if (self) {
        _foo = foo;
        _bar = bar;
        // any other initializations
    }
    return self;
}

You should have a separate method for actually starting the network operations. There are plenty of good, practical reasons for this. But the best of all reasons is that it's OOP standard practice and would be the expected behavior.

---

@property (weak, nonatomic) id <NSObject, WebServiceDelegate> delegate; 

Is there ever a case where you'd want a property that conforms to the WebServiceDelegate protocol and don't necessarily need it to conform to NSObject protocol? I expect this is highly unlikely. So BETTER would be to mark your WebServiceDelegate protocol itself as conforming to NSObject protocol.

So, in the protocol declaration:

@protocol WebServiceDelegate <NSObject>

Now change the property to :

@property (weak, nonatomic) id <WebServiceDelegate> delegate; 

And now you've got a property called delegate which still conforms to both NSObject and WebServiceDelegate protocols, but your code is a little cleaner and makes a bit more sense.

If we were talking about a protocol other than NSObject, I may or may not necessarily recommend this, but since we're talking about NSObject, I'm definitely recommending this change.

---

---

---

EDIT: In response to your latest edit, let me expand on and clarify some things.

First of all, I understand how delegates work. I understand that it is through the delegate that your class makes callbacks to let whoever needs to know that it's done working (or whatever status update). And I understand that without a delegate, these callbacks can't be made without the current structure.

But that wasn't actually my question.

My question, for clarity, was more along the lines of:

Does it ever make sense for someone to instantiate this class and use nil for the delegate (or just not set it).

Given that every method in the protocol is flagged as @optional, to me, the answer has to be YES. It will sometimes make sense that someone using this class may not want to bother with a delegate.

If, however, nil is NEVER an acceptable option for the delegate, then I'm proposing a restructuring of some methods.

First of all, if nil isn't an acceptable delegate, then not every method in your protocol is really @optional. I don't know which ones aren't, but if the delegate MUST be set, then that can only mean at least one of the methods in the protocol that the delegate conforms to is @required.

And second of all, if nil isn't an acceptable delegate, then that's just one more reason on top of a big stack of reasons why we shouldn't be starting the networking in init. Because your init methods don't take a delegate argument, there's no guarantee that a delegate will EVER be set, and there's even less of a guarantee that it will be set before your class tries to call a method on a delegate, since it's starting the networking before anyone has an opportunity to set a delegate.

---

As for the init methods, I can clearly see from your class structure that the class is used for nothing more than doing the networking. This is not a good enough excuse to put the networking in the init method.

As I already explained, it's problematic enough that I don't get an opportunity to set the delegate before the networking calls start. So that's one reason to take the networking out init methods. But now you say "Well, I can just make the delegate as one of the init arguments!"

But that's still not good enough.

Even if all we're using the class for is making a single networking connection, it's still completely unacceptable for the init method to do anything other than init.

"This class is only used for making a single networking connection" is decent class design (though arguably it might be better to allow the object to be reused for multiple networking connections with the same delegate). It's not an excuse to cram everything into that objects init method however.

For a good example of how your class SHOULD work, let's look at similar Foundation class (Foundation classes should ALWAYS be used as an outline for Objective-C best practice).

In this case, the most similar Foundation class I can think of is NSURLConnection. How does that work?

There are a few different ways that NSURLConnection can be used, and they're all different from how your class is used.

First, let's look at the init/factory methods.

+ connectionWithRequest:delegate:
- initWithRequest:delegate:
- initWithRequest:delegate:startImmediately:

The first two don't open a network connection. The third does open a connection, but notice how the init method makes this explicitly clear--it also collects ALL the required information.

If we use the first two, or send NO as the argument for the third, the object has another method we use to actually start the networking. It's called:

- start

This actually begins the networking.

The nice thing for this is, I can instantiate multiple connection objects throughout the course of UI interactions, throw them in an array, and wait till some point later in time and call start on them all at the same time, without having to instantiate all the objects as I want to use them.

Now then, the important thing to note about NSURLConnection are two other class methods that are available.

The first is:

+ sendSynchronousRequest:returningResponse:error:

This one is mostly self explanatory. It sends the request synchronously. It also starts the networking immediately. But the big thing to note here is that the return value from this method is an NSData object. When you use this, whatever thread it's called on waits around for the return, and then you have an NSData return. You never get an NSURLConnection object out of this method, you NEVER create one at all.

The other one to look at is this:

+ sendAsynchronousRequest:queue:completionHandler:

Now this method handles the networking asynchronously on a queue that you specify, but what's important to note here is that the return is void. Once again, we never have or need an NSURLConnection object. In this case, we also don't have a delegate. Instead, we send a block for the completionHandler argument, and the code in this block is executed when the request completes.

So we have 5 ways to use NSURLConnection. Two of don't start the connection immediately and require a call to start to start. The third either does or doesn't start immediately depending on a BOOL argument, but this is sent explicitly by the user, so it's CLEAR what's going on. And the last two do start immediately, but don't actually instantiate an object of this class.

Your class doesn't follow any of these patterns.

---

---

UPDATE

After taking another look at NSURLConnection class, it seems that most of these init methods actually do start the networking. Still though, those that do either take the delegate as the argument or take a completion handler and return void, or perform the task synchronously and return the NSData object. These are things to keep in mind.