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?

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).

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?