5
\$\begingroup\$

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?

\$\endgroup\$
  • 3
    \$\begingroup\$ Please don't make edits that invalidate answers. \$\endgroup\$ – nhgrif May 28 '15 at 11:43
  • \$\begingroup\$ I was just trying to show the code which uses sender parameter. OK, I will revert. \$\endgroup\$ – sasquatch May 28 '15 at 11:47
  • \$\begingroup\$ 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. \$\endgroup\$ – nhgrif May 28 '15 at 11:49
  • \$\begingroup\$ 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. \$\endgroup\$ – sasquatch May 28 '15 at 11:51
  • 1
    \$\begingroup\$ 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. \$\endgroup\$ – nhgrif May 28 '15 at 12:00
7
\$\begingroup\$

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

\$\endgroup\$
  • \$\begingroup\$ Thanks a lot. I have edited the question to include sender parameter. Do you still think we can eliminate it? \$\endgroup\$ – sasquatch May 28 '15 at 6:14
  • \$\begingroup\$ The enum is named by Alamofire, should I subclass and rename it? \$\endgroup\$ – sasquatch May 28 '15 at 9:11
  • \$\begingroup\$ No. I didn't realize that the library would give such a poorly named type. I'd complain to Alamofire, if I were you. \$\endgroup\$ – nhgrif May 28 '15 at 11:50
4
\$\begingroup\$

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?

\$\endgroup\$
  • \$\begingroup\$ 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. \$\endgroup\$ – sasquatch May 28 '15 at 8:57
  • 1
    \$\begingroup\$ But you have to handle errors in general anyway. Why treat this kind of error differently? \$\endgroup\$ – 200_success May 28 '15 at 9:00
  • \$\begingroup\$ So are you suggesting that I should not check for connectivity explicitly? And let NSURLConnection handle it? \$\endgroup\$ – sasquatch May 28 '15 at 9:05
  • \$\begingroup\$ That's right. Just do what you want to do, let it fail, and use your standard error-handling mechanism to report the failure. \$\endgroup\$ – 200_success May 28 '15 at 9:07
  • 1
    \$\begingroup\$ Hmm, fair enough. Guess I was wrong. \$\endgroup\$ – nhgrif May 28 '15 at 11:50

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.