Asynchronous networking iOS API calls

I have the code above in my iOS SDK that I am building that makes it easy for users of the SDK to make API calls. Basically, users can specify an API endpoint and easily make API calls.

Is there any way to improve the code above to make it more maintainable and easy for users? Is that the proper way to handle asynchronous networking in iOS?

```
+ (void) makeRequestToEndPoint:(NSString ) endpoint values:(NSMutableDictionary ) params onCompletion:(SDKCompletionBlock) responseHandler
{
NSString * urlString = [self createApiUrlFromEndpoint: endpoint];
NSURL * url = [NSURL URLWithString: urlString];

NSMutableURLRequest *request = [[NSMutableURLRequest alloc] initWithURL: url];
request.HTTPMethod = @"POST";
[request setValue:@"application/json" forHTTPHeaderField:@"Accept"];
[request setValue:@"charset" forHTTPHeaderField:@"utf-8"];
[request setValue:@"application/json" forHTTPHeaderField:@"Content-Type"];
request.HTTPBody = [[params urlEncodedString] dataUsingEncoding:NSUTF8StringEncoding];

[NSURLConnection sendAsynchronousRequest:request
queue:[NSOperationQueue mainQueue]
completionHandler:^(NSURLResponse response, NSData data, NSError *error)
{

NSDictionary * dictionary = nil;
NSError * returnError = nil;
NSString * errorCode = nil;
NSString * errorText = nil;
NSInteger newErrorCode = 0;

if([data length] >= 1) {
dictionary = [NSJSONSerialization JSONObjectWithData: data options: 0 error: nil];
}

if(dictionary != nil) {

if([dictionary objectForKey: @"error_code"] != nil) {
errorCode = [dictionary objectForKey: @"error_code"];
errorText = [dictionary objectForKey: @"error_description"];
}
}

NSInteger statusCode = [(NSHTTPURLResponse *)response statusCode];

if(statusCode >= 400)
{

NSDictionary * dictionary = nil;
NSError * returnError = nil;
NSString * errorCode = nil;
NSString * errorText = nil;

This is superfluous and unnecessary. It's more than fine to just declare them. They're nil by default.

NSDictionary *dictionary;
NSError *returnError;
NSString *errorCode;
NSString *errorText;

You should use modern syntax for your mutable dictionaries.

Rather than [someDictionary setValue:someValue forKey:someKey];, you can and should simply do:

someDictionary[someKey] = someValue;

And the same can be done for accessing the variable. Rather than NSString *foo = [someDictionary objectForKey:someKey];, you can and should simply do:

NSString *foo = someDictionary[someKey];

Most Objective-C programmers I know (and certainly myself included) don't particularly like foo == nil or foo != nil. We can instead use:

if (foo) // foo != nil

And

if (!foo) // foo == nil

I don't know what exactly createApiUrlFromEndpoint: does for you, as it's not included in the review. But I'd argue, strictly speaking, that this method (the one posted for review) should probably take an NSURL for the endpoint argument and createApiUrlFromEndpoint: should be a public method. If for no other reason, it could save multiple calls to createApiUrlFromEndpoint: if a user wants to use the same URL multiple times with a different params dictionary.

Moreover, as the method is called "create API URL", it should return an NSURL object, not an NSString object.

if([data length] >= 1)

I personally thing >= and 0) is exactly the same... except less ugly in my opinion.

Your method is called makeRequestToEndPoint:values:onCompletion:

I'm going to argue that values: should be changed to params: since that's what you're naming the the variably internally.

I'm also going to argue that onCompletion: should be changed to completionHandler: or completion: so that it matches every other existing Apple method which takes a completion block as an argument.

dictionary = [NSJSONSerialization JSONObjectWithData: data options: 0 error: nil];

You've got an error object. You call a method that could have an error, but you don't care at all about the possible error that could happen within this method? You should send &returnError rather than nil as the last argument, and immediately follow this call with:

if (error) {
    responseHandler(dictionary,returnError);
    return;
}

There's no point in continuing to execute code in the method if all the other code relies on the dictionary being initialized correctly, AND if the dictionary wasn't initialized correctly AND we're going to return some sort of an error, we should at least care about the one that goes with this method and will give a decent description of what problem occurred.

To add to the previous point, this is all happening within a completion block which sends as an argument an NSError object. So the absolute first thing we should do before we try to initialize the dictionary is check this error object, and just as before, if it exists, call responseHanlder(nil,error); and return out of the method.

As it stands, other than the points I made above, the code seems decent enough. For completeness however, you might implement a way of accomplishing this within the same class with the delegate-protocol pattern, rather than completion blocks.

To do this, you'd need to create a protocol, add a delegate property (which conforms to the protocol and is a weak property), and add a instance method version of this class method--the primary difference being that the instance method doesn't take a completion block argument, and instead of executing a completion block, it calls the delegate methods.

Unlike Ravi D's answer, however, the delegate methods defined in your protocol need more arguments so they can send more detail.

You could create a single method that's called regardless of whether or not there's an error:

@required - (void)fooBarObject:(FooBarObject *)fooBarObject 
            didCompleteRequest:(NSDictionary *)results 
                         error:(NSError *)error;

Where the first part of the method is sending a self argument (similar to tableView:cellForRowAtIndexPath:, etc), the second argument is the results dictionary (though would be nil in the case of an error) and the third argument is the error (and would be nil when there's not an error. These leaves it up to the delegate to handle checking whether or not there was an error in this method.

Alternatively, you can require two methods, one for success and one for failure. You're checking that in your methods, and the delegate doesn't have to do the check--simply has to implement a method for each scenario:

```
@required - (void)fooBarObject:(FooBarObject *)fooBarObject
didCompleteRequest:(NSDictionary *)results;

@required - (void)fooB

StackExchange Code Review Q#32405, answer score: 6