Fetching the definition of a word using a REST API

Question

This is the code I'm using to GET the definition of a word from Wordnik's REST API. I don't want to ignore any error but this is causing the code to be ridiculously long. I'm new to Swift so I'd appreciate it if someone could explain how best to trim it down.

class func define(params: DefinitionParameters!, delegate: WordnikDictionaryDelegate!){
 if let url = url(definitionParams: params){
 NSLog("URL: \(url)")
 let session = NSURLSession.sharedSession()
 let task = session.dataTaskWithURL(url, completionHandler: { (data, response, error) -> Void in
 var error : NSError?
 if let json = NSJSONSerialization.JSONObjectWithData(data, options: nil, error: &error) as? NSMutableArray{
 NSLog("Response: \(json)")
 if json.count == 0 {
 delegate.dictionaryDidLoad(
 nil,
 forWord: params.word,
 error: self.buildError(404, message: "No definitions found")
 )
 }else if let definitionJSON = json[0] as? NSMutableDictionary{
 delegate.dictionaryDidLoad(
 definitionJSON.valueForKeyPath("text") as? String,
 forWord: params.word,
 error: nil
 )
 }else{
 delegate.dictionaryDidLoad(
 nil,
 forWord: params.word,
 error: self.buildError(401, message: "Unexpected Result Format")
 )
 }
 }else{
 delegate.dictionaryDidLoad(
 nil,
 forWord: params.word,
 error: self.buildError(401, message: "Unexpected results format")
 )
 }
 })
 task.resume()
 }else{
 delegate.dictionaryDidLoad(
 nil,
 forWord: params.word,
 error: self.buildError(400, message: "Invalid URL")
 )
 }
 }

Answer 1

First and foremost, we 100% absolutely should not be writing Swift methods which take implicitly unwrapped optionals as arguments. The only reason we should be doing this is because we're implementing an Objective-C protocol which has not yet adopted the new Objective-C nullability annotations, but I don't think that's what is going on here.

So, our arguments should either be optionals or non-optionals. Here, I think we probably want non-optionals, so get rid of those ! operators on our method arguments. If we really want them to be optional, change the ! to ? and properly deal with the method arguments in the cases where they might be nil.

---

Next, we definitely don't want to even risk shipping our app with calls to NSLog, so let's take care of fixing that.

---

Finally, a delegate doesn't make particularly good sense here. Generally speaking, we want to set a delegate when we're going to instantiate an object, set its delegate property, and then reuse the object with the same delegate over and over. If we were to do this, our method wouldn't be a class method.

Here, it doesn't even necessarily make sense for this to be a class, and rather than taking an object as our delegate argument, we should take a closure as our completionHandler argument, exactly as session.dataTaskWithURL does.

So first, let's typealias a completion handler:

typealias DefineWordCompletionHandler = (word: String, definition: String, error: NSError?) -> Void

Now, let's modify our method signature:

class func define(params: DefinitionParameters, completionHandler: DefineWordCompletionHandler) {
 /* ... implementation ... */
}

And whenever we've done everything we need to do and we're ready to run the completionHandler, it's as simple as such:

completionHandler(word: params.word, definition: value, error: error)

---

One minor note about Swift syntax with closure arguments... when a closure is the last argument in a method, it doesn't have to be included inside the parenthesis like you've done with dataTaskWithURL. Instead we can do something more like this:

let task = session.dataTaskWithURL(url) { (data, response, error) -> Void in
 /* ... completion handler code ... */
}

This is a pretty minor thing, but it does avoid the awkward }) at the end, and I think the first line looks slightly cleaner too.

Answer 2

Inside the if let json = ... { ... }, all the execution branches will call delegate.dictionaryDidLoad, and the value of forWord is always params.word. Instead, you could initialize value and error to nil, set them in the appropriate conditional branches, and finally call the delegate. Something like this:

if let json = NSJSONSerialization.JSONObjectWithData(data, options: nil, error: &error) as? NSMutableArray{
 NSLog("Response: \(json)")
 if json.count == 0 {
 error = self.buildError(404, message: "No definitions found")
 } else if let definitionJSON = json[0] as? NSMutableDictionary{
 value = definitionJSON.valueForKeyPath("text") as? String,
 } else {
 error = self.buildError(401, message: "Unexpected Result Format")
 }
} else {
 error = self.buildError(401, message: "Unexpected Result Format")
}
delegate.dictionaryDidLoad(
 value,
 forWord: params.word,
 error: error
)