Asynchronous HTTP JSON request

Question

For asynchronous requests, I use Alamofire. I have created one method for requesting async data.

func requestData(method: Method, urlString: String, onSuccess: (AnyObject) -> Void , onFailure: (NSError) -> Void, sender: UIViewController, postParams:AnyObject? = nil) {
 let AFmanager = Manager(configuration: seesionConfiguration)
 let URL = NSURL(string: urlString)!
 let urlRequest = NSMutableURLRequest(URL: URL)
 urlRequest.HTTPMethod = method.rawValue
 if let param: AnyObject = postParams {
 urlRequest.HTTPBody = NSJSONSerialization.dataWithJSONObject(param, options: nil, error: nil)
 }
 if method == .POST {
 urlRequest.addValue("application/json", forHTTPHeaderField: "Content-Type")
 }
 urlRequest.addValue("application/json", forHTTPHeaderField: "Accept")
 AFmanager.request(urlRequest)
 .responseJSON { (request, response, JSONResult, error) in
 if (error == nil) {
 onSuccess(JSONResult!)
 } else {
 onFailure(error!)
 }
 } 
}

Here, I have made postParams of type AnyObject as parameter can be of any dictionary type. Is there another way of defining the method so that postParams can be checked to be of type dictionary?

Any other improvement/suggestion?

• nhgrif: Please don't make edits that invalidate answers.
• saurabh: I was just trying to show the code which uses sender parameter. OK, I will revert.
• nhgrif: I already rolled back the edit. You may want to put back in some of the other edits you made. Just don't edit the code. Editing the code should be impossible because you should have copy & pasted from your source code anyway. If you want an updated version of your code reviewed, you're welcome to post a new question.
• saurabh: Sure, Actually the method contained some other irrelevant code, so I decided to show only what matters. I will post a new question for other things.
• nhgrif: Make sure to take into consideration the points in the answers in your new question. If the new question is too similar, we may consider it to be a duplicate.

Answer 1

I know this is technically outside the scope of the review here, but your first parameter's type is Method. This enum desperately needs to be renamed. When I see Method, I cannot help but think of a programming method... I think you're passing a pointer to a class's method here. This type needs a better, more descriptive name.

---

Our sender parameter doesn't need to be a UIViewController. Does it? In fact, are we even using the sender parameter? Can we just eliminate it?

---

The postParams needs to be an AnyObject parameter. We can turn more than just dictionaries into valid JSON. The NSJSONSerialization class offers a isValidJSONObject method for which to check whether or not we can serialize. So, we can leave this parameter as is, and simply make this check:

if let param: AnyObject = postParams {
 if NSJSONSerialization.isValidJSONObject(param) {
 urlRequest.HTTPBody = NSJSONSerialization.dataWithJSONObject(param, options: nil, error: nil)
 } /* else {
 // call the error handler?
 } */
}

---

And finally, one comment on the parameters our method takes... taking two completion blocks isn't the suggested best practice. By taking two completion blocks, you force the user into duplicating any code they want to happen whenever this callback occurs (if they want it to happen under both scenarios). So, we should combine these arguments into a single completion block (much like AFmanager is doing in the code you're using) and more importantly, we should make this argument the last argument.

In Swift, when the last argument is a closure, we get a slightly nicer syntax. Instead of something like:

someObject.someMethod(param1: foo, param2: bar, closureParam: { println("hello world") })

We get a slightly better syntax:

someObject.someMethod(param1: foo, param2: bar) {
 println("hello world")
}

Again, you made use of that when you used the AFmanager, so let's make our code follow suit. Combine your success and error completion handlers into a single closure parameter which is called no matter success or failure, and move it to the last argument to give users a better syntax option (and so we don't have any dangling parameters after the space-consuming closure).

• saurabh: Thanks a lot. I have edited the question to include sender parameter. Do you still think we can eliminate it?
• saurabh: The enum is named by Alamofire, should I subclass and rename it?
• nhgrif: No. I didn't realize that the library would give such a poorly named type. I'd complain to Alamofire, if I were you.

Answer 2

I agree with @nhgrif that sender is not an appropriate parameter for this function. The UIViewController is code for the view, and it has no business being mentioned in a function that makes HTTP requests.

If the network is unavailable, then I would expect that the NSURLConnection would fail with an NSError whose message is "The Internet connection appears to be offline.". You support error handling via the onFailure callback anyway — so why not treat Internet inaccessibility as just another error?

• saurabh: Because I want to check connectivity before even making the request. If I do it in every ViewController before calling this method, I will have to do it a lot of time.
• 200_success: But you have to handle errors in general anyway. Why treat this kind of error differently?
• saurabh: So are you suggesting that I should not check for connectivity explicitly? And let NSURLConnection handle it?
• 200_success: That's right. Just do what you want to do, let it fail, and use your standard error-handling mechanism to report the failure.
• nhgrif: Hmm, fair enough. Guess I was wrong.