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 <= are ugly and only use them when there's no alternative (or the alternative is even uglier). In this case, length is an unsigned integer, so if([data length] > 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)fooBarObject:(FooBarObject *)fooBarObject requestDidFail:(NSError *)error;
NSURLConnectionis deprecated. You should useNSURLSessionand notNSURLConnection. See developer.apple.com/videos/play/wwdc2015/711\$\endgroup\$