Code Review Stack Exchange is a question and answer site for peer programmer code reviews. Join them; it only takes a minute:

Sign up
Here's how it works:
  1. Anybody can ask a question
  2. Anybody can answer
  3. The best answers are voted up and rise to the top

I'm new to iOS development and also Swift. I'm working on a project consisting of a menu that leads to my various experimentations with the iOS SDK.

I've written the menu titles and segueIdentifiers in a plist and I'm populating an array of NavigationItem in viewDidLoad().

Is the following code idiomatic, particularly in the way it handles optional values? What might be better?

override func viewDidLoad() {
        super.viewDidLoad()

        let path = NSBundle.mainBundle().pathForResource("NavigationMenu", ofType: "plist")

        if path == nil{
            NSLog("Unable to locate file : NavigationMenu.plist")
            return
        }

        let items = NSArray(contentsOfFile: path!)

        if items == nil {
            NSLog("Navigation menu items could not be read from plist.")
            return
        }

        for var i = 0 ; i < items!.count ; i++ {

            let item : NSDictionary = items!.objectAtIndex(i) as! NSDictionary
            navigationItems.append(NavigationItem(
                name:(item.objectForKey("name") as! String),
                andSegueIdentifier: (item.objectForKey("segueIdentifier") as! String)
                ))

        }

    }
share|improve this question
    
Does pathForResource return NSURL! or NSURL? and same question for everything else here that might return nil? – nhgrif Apr 24 '15 at 11:45
    
pathForResource returns String?. NSArray(contentsOfFile) returns [AnyObject]? – W.K.S Apr 24 '15 at 11:49
up vote 7 down vote accepted

A better way to handle optional values is "optional binding":

if let path = NSBundle.mainBundle().pathForResource("NavigationMenu", ofType: "plist") {
    // do something with `path`
} else {
    NSLog("Unable to locate file : NavigationMenu.plist")
}

which tests and unwraps the optional result as a single action. Inside the if-block, path is the unwrapped String.

You can do the same with the items:

if let path = NSBundle.mainBundle().pathForResource("NavigationMenu", ofType: "plist") {
    if let items = NSArray(contentsOfFile: path) {
        // do something with `items` ...
    } else {
        NSLog("Navigation menu items could not be read from plist.")
    }
} else {
    NSLog("Unable to locate file : NavigationMenu.plist")
}

As of Swift 1.2, multiple optional bindings can be combined in a single if-statement, which reduces the number of nested if-levels:

if
    let path = NSBundle.mainBundle().pathForResource("NavigationMenu", ofType: "plist"),
    let items = NSArray(contentsOfFile: path) {
        // do something with `items` ...
} else {
    NSLog("Unable to load NavigationMenu")
}

The downside is that you have only a common else-block for the error condition and cannot distinguish which of the optional bindings failed.

Instead of objectAtIndex() and objectForKey() you can use subscripting for NSArray and NSDictionary:

for var i = 0 ; i < items!.count ; i++ {
    let item = items![i] as! NSDictionary
    let navItem = NavigationItem(name: item["name"] as! String,
        andSegueIdentifier: item["segueIdentifier"] as! String)
    navigationItems.append(navItem)
}

But a better way is to cast the NSArray to a Swift array of Swift dictionaries in the first step. Then you can use array enumeration and don't need any casts later:

if
    let path = NSBundle.mainBundle().pathForResource("NavigationMenu", ofType: "plist"),
    let items = NSArray(contentsOfFile: path) as? [[String : String]] {

        for item in items {
            let navItem = NavigationItem(name: item["name"]!, andSegueIdentifier: item["segueIdentifier"]!)
            navigationItems.append(navItem)
        }
} else {
    NSLog("Unable to load NavigationMenu")
}

This assumes that values for the "name" and "segueIdentifier" are present (and will crash at runtime otherwise). Alternatively, use optional binding again:

for item in items {
    if let name = item["name"], ident = item["segueIdentifier"] {
        let navItem = NavigationItem(name: name, andSegueIdentifier: ident)
        navigationItems.append(navItem)
    } else {
        NSLog("Invalid navigation item")
    }
}

Finally, you could replace the inner loop by a map() operation:

if
    let path = NSBundle.mainBundle().pathForResource("NavigationMenu", ofType: "plist"),
    let items = NSArray(contentsOfFile: path) as? [[String : String]] {
        navitationItems = map(items) {
            NavigationItem(name: $0["name"]!, andSegueIdentifier: $0["segueIdentifier"]!)
        }
} else {
    NSLog("Unable to load NavigationMenu")
}
share|improve this answer
    
This is a terrific answer! I didn't know you could do multiple optional bindings with a single if else statement. The large number of nested statement is what put me off – W.K.S Apr 24 '15 at 20:03

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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