Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

In this iOS app I'm working on, there are two landing screens shown to the user depending on a particular status. The screens are Login screen and Activate screen.

Let me explain what this Activate screen is. There is something called activating users. It's done by a web backend and the app has no control over it. Each device the app runs has a unique device ID. This unique ID is generated in the device. A person manually activates this ID from the backend. And there are two ways this ID gets to the backend. Either it's sent from the device at first launch, or it can be obtained by some other method and entered to the backend database manually.

When the app launches, you must get that ID and send it over to the backend to check if it's activated or not. You get two statuses, 100 - Activated, 101 - Not Activated. I keep a boolean flag called DeviceIsActive in NSUSerDefaults to store the result.

When the app launches first it checks if the DeviceIsActive is true, if it is, the user is directed to the Login screen. If its false, you're directed to the Activate screen where you're informed to contact the person at the backend to activate it.

Here is the code I have in the didFinishLaunchingWithOptions in the AppDelegate file which is the method that runs first when the app is launched.

var window: UIWindow?
private enum Screen {
    case ActivateScreen
    case LoginScreen
}
let api = API()

func application(application: UIApplication!, didFinishLaunchingWithOptions launchOptions: NSDictionary!) -> Bool {

    if Preferences.getDeviceIsActive() == true {
        // Checks if deviceIsActive flag is true.
        redirectToView(.LoginScreen)
    } else {
        // If there is not deviceID, generate one.
        if Preferences.getDeviceID() == nil {
            var deviceID = NSUUID.UUID().UUIDString
            deviceID = deviceID.stringByReplacingOccurrencesOfString("-", withString: "") // e,g: 70788AEB8586513C46B0599AEA4F82BF
            Preferences.setDeviceID(deviceID)

            // Send the ID to the backend to check.
            register()
        } else {
            // If there is a deviceID already, still send it to the server to check and make sure.
            register()
        }
    }

    return true
}

private func register() {
    api.registerDevice(Preferences.getDeviceID(), completionHandler: { (response, error) -> Void in
        let (status, code) = response

        if status == Status.DeviceIsActive.toRaw() { // Status is an enum. It has all the statuses (100, 101).
            Preferences.setDeviceIsActive(true)
            self.redirectToView(.LoginScreen)
        } else {
            self.redirectToView(.ActivateScreen)
        }
    })
}

private func redirectToView(screen: Screen) {
    self.window = UIWindow(frame: UIScreen.mainScreen().bounds)
    let storyboard: UIStoryboard = UIStoryboard(name: "Main", bundle: nil)

    switch screen {
    case .ActivateScreen:
        let activateVC: ActivateViewController = storyboard.instantiateViewControllerWithIdentifier("ActivateController") as ActivateViewController
        let navigationVC = UINavigationController(rootViewController: activateVC)
        self.window?.rootViewController = navigationVC
    case .LoginScreen:
        let loginVC: LoginViewController = storyboard.instantiateViewControllerWithIdentifier("LoginController") as LoginViewController
        let navigationVC = UINavigationController(rootViewController: loginVC)
        self.window?.rootViewController = navigationVC
    default:
        println("No view controller specified")
    }

    self.window?.makeKeyAndVisible()
}

Now the code works fine. But it's just so ugly and messy with all the if else statements all over the place. Is there any way to make it cleaner?

share|improve this question

1 Answer 1

up vote 4 down vote accepted
if Preferences.getDeviceIsActive() == true {
    // Checks if deviceIsActive flag is true.
    redirectToView(.LoginScreen)
} else {
    // If there is not deviceID, generate one.
    if Preferences.getDeviceID() == nil {
        var deviceID = NSUUID.UUID().UUIDString
        deviceID = deviceID.stringByReplacingOccurrencesOfString("-", withString: "") // e,g: 70788AEB8586513C46B0599AEA4F82BF
        Preferences.setDeviceID(deviceID)

        // Send the ID to the backend to check.
        register()
    } else {
        // If there is a deviceID already, still send it to the server to check and make sure.
        register()
    }
}

This seems to be the chunk of code you're primarily concerned with, so it's where I'll focus on for now.


First, == true is basically always redundant. Our first if could simply be:

if Preferences.getDeviceIsActive() {

If the nested else were truly necessary, I'd recommend unnesting the nested if else and turning this into an if else if else, but the nested else is unnecessary. We're calling register() unconditionally inside the else. We can just write:

if Preferences.getDeviceID() == nil {
    var deviceID = NSUUID.UUID().UUIDString
    deviceID = deviceID.stringByReplacingOccurrencesOfString("-", withString: "")
    Preferences.setDeviceID(deviceID)
}

register()

api.registerDevice()

You haven't included any details about what API is or what the registerDevice method does specifically, but it looks like it most likely is asynchronous, and it probably calls to a web service of some sort.

This sort of logic would be fine in say a standard view controller that loads after your app delegate was finished. But we don't know how long we may be waiting for this return.

If the call is asynchronous, then your application(application:, didFinishLaunchingWithOptions launchOptions:) -> Bool will most likely return before we figure out where to navigate to. Best case scenario, your project is set up to have a root view controller via the storyboard without the redirectToView method being called and it just looks a little weird when it then later redirects you. Worst case scenario, you don't have a default view controller set up and we have to rely on redirectToView which means we may sometimes not have a view before we're done with the didFinishLaunchingWithOptions returns and the app will crash.

And heaven forbid this line of code ever get called without a default view already in place:

default:
    println("No view controller specified")

Because this might also leave the window without a view, which again, is a crash.

If the call is synchronous, now we're causing the didFinishLaunching to hang out for an unnecessarily long amount of time because we're performing network operations when we're supposed to be loading up resources and initializing our app.

Ultimately the best solution to this is to move this sort of logic into a base view controller. For now, let's call it the "Home" view controller. Our app will be set up to always load to "Home". We want to have somewhere to go to in order to get out of didFinishLaunchingWithOptions as quickly as possible.

Once we're at "Home", now we perform the logic for determining whether we need to login or activate.

Or perhaps, given that the activation is a one-time process, perhaps we should default to the login screen. The login screen can check if the device is activated or not. If it determines it's not activated, we can give the user a pop up. "This device is not activated. Would you like to activate it now?" and answering yes to this pop up takes us to the activation screen.

share|improve this answer
    
Thank you :) The code looks slightly better now. And yes I forgot to mention. The API calls are asynchronous. I understand the issue you described. I'm gonna do as you suggested. Have one landing view controller to do the checking and stuff and then redirect from there. It's much safer I agree. Thank you for the detailed answer. –  Isuru Aug 10 at 18:01

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.