2
\$\begingroup\$

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

        }

    }
\$\endgroup\$
2
  • \$\begingroup\$ Does pathForResource return NSURL! or NSURL? and same question for everything else here that might return nil? \$\endgroup\$
    – nhgrif
    Apr 24, 2015 at 11:45
  • \$\begingroup\$ pathForResource returns String?. NSArray(contentsOfFile) returns [AnyObject]? \$\endgroup\$
    – W.K.S
    Apr 24, 2015 at 11:49

1 Answer 1

7
\$\begingroup\$

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")
}
\$\endgroup\$
1
  • \$\begingroup\$ 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 \$\endgroup\$
    – W.K.S
    Apr 24, 2015 at 20:03

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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