current community

your communities

more stack exchange communities

Stack Exchange
tour help
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.
up vote 5 down vote favorite

I'm developing an iOS Application in Swift that uses Google Cloud Endpoints as its backend. For now, I've just written the code for the user to login with his Gmail account and I've refactored it as much as possible but the callback chain is making me nervous.

The reason I'm worried is because firstly, I'm fairly new to programming, and secondly, the company where I work now uses a similar callback-chain method to make network calls and I sort of dislike it because it takes a while for a new developer to figure out the sequence of events and it feels a bit like spaghetti code.

I would appreciate any suggestions for improving the code especially to avoid callback chaining or at least suggestions to make it more navigable.

P.S.: If you're wondering why I'm rebuilding the OAuth 2 authentication object from NSUserDefaults, please see this question.

class ViewController: UIViewController{

    //Member variables

    //Outlets
    @IBOutlet weak var loginButton: UIButton!
    @IBOutlet weak var detailsLabel: UILabel!

    //Lifecycle Overrides

    override func viewDidLoad() {

        if let auth : GTMOAuth2Authentication = GoogleEndpointAssistant.rebuildGTMOAuth2AuthenticationFromUserDefaults(){

            setAuthentication(auth)
        }

    }

    //Methods

    func setAuthentication(auth : GTMOAuth2Authentication){

        //set OAuth2 as Google End Point service authorizer
        ASSession.sharedSession.gtlService.authorizer = auth;

        //save user's ID (Will be used to fetch user's information from server)
        ASSession.sharedSession.user.identifier = NSNumber(longLong: (auth.userID as NSString).longLongValue);

        configureSession()
    }

    /**
    Configures the session instance (ASSession.configure()) and, if successsful, calls identifyUser()
    */
    func configureSession(){

        MBProgressHUD.showHUDAddedTo(self.view, animated: true)

        ASSession.sharedSession.configure({ (error:NSError?) -> Void in

            if error == nil {

                self.identifyUser()

            }else{

                self.detailsLabel.text = error!.localizedDescription;

                MBProgressHUD.hideAllHUDsForView(self.view, animated: true)
                ErrorMessage.SessionSettingsDownloadFailed.alertView().show()

            }
        })


    }

    /**
    Checks whether or not the user is registered on the server.

    If the user is registered, a segue is peformed to the dashboard screen.

    If the user is not registered, a segue is performed to the complete registration screen.
    */
    func identifyUser(){

        let userExistsQuery : GTLQueryHdys = GTLQueryHdys.queryForUserGetUserWithIdentifier(ASSession.sharedSession.user.identifier.longLongValue) as GTLQueryHdys;

        ASSession.sharedSession.gtlService.executeQuery(userExistsQuery, completionHandler: { (ticket:GTLServiceTicket!, response:AnyObject!, error:NSError!) -> Void in

            MBProgressHUD.hideAllHUDsForView(self.view, animated: true)

            if response != nil {

                //user is registered on the server and details have been received.
                NSLog("User registered")
                self.loginButton.setTitle("Registered", forState: UIControlState.Normal)

            }else if GoogleEndpointAssistant.statusCodeFromError(error) == 404 {

                ////user is not registered on the server.
                NSLog("User not Registered")

                let auth = ASSession.sharedSession.gtlService.authorizer as GTMOAuth2Authentication;

                self.detailsLabel.text = "Expiry: \(auth.expirationDate), Auth: \(auth.canAuthorize)."
                self.loginButton.setTitle("Not Registered", forState: UIControlState.Normal)

            }else{

                //an error occurred while request was in progress.
                NSLog("Error: \(error.localizedDescription)")
                self.detailsLabel.text = error.localizedDescription;
                self.loginButton.setTitle("Error", forState: UIControlState.Normal)
            }

        })
    }

    //Actions

    /**
    Displays GTMOAuth2ViewControllerTouch so that the user can sign-in with his/her gmail account.

    Once complete, finishedWithAuth is invoked
    */
    @IBAction func authenticateUser(){

        let authenticationViewController = GTMOAuth2ViewControllerTouch(
            scope: kGTLAuthScopeHdysUserinfoEmail,
            clientID: GoogleCloudEndPointClientID,
            clientSecret: GoogleCloudEndPointClientSecret,
            keychainItemName: GoogleCloudKeychainItemName,
            delegate: self,
            finishedSelector: "viewController:finishedWithAuth:error:"
        )

        self.presentViewController(authenticationViewController, animated: true, completion: nil)
    }

    //Delegate Methods

    /**
    This callback is invoked after the user completes sign-in with his/her gmail account in the modal GMTOAuth2ViewControllerTouch.

    If the sign-in was successful, OAuth2Authentication is set as the Google Cloud Endpoint service authorizer

    ;see also; setAuthentication(auth: OAuth2Authentication)
    */
    @objc(viewController:finishedWithAuth:error:)
    func finishedWithAuth(viewController :GTMOAuth2ViewControllerTouch , auth:GTMOAuth2Authentication,error:NSError!){

        self.dismissViewControllerAnimated(true, completion: nil);

        if error != nil {

            println("Authentication Failure: \(error.localizedDescription)");
            ErrorMessage.OAuth2AuthenticationFailed.alertView().show()

        }else{

            GoogleEndpointAssistant.saveGTMOAuth2AuthenticationToUserDefaults(auth)

            setAuthentication(auth)

        }

    }
}
share|improve this question

1 Answer 1

up vote 4 down vote

I tend to not like callback closures myself and I agree with your sentiment toward "spaghetti code". With time and experience, these will tend to make sense, and you kind of just know what they're doing, but they still tend to give me pause. I personally vastly prefer the delegation pattern which is a common pattern throughout Apple's libraries.

Unfortunately, not every library we use is written by Apple, and not every library we use is written solely with iOS developers in mind, and accepting callback closures is a pretty common pattern outside of iOS development (though not wholly uncommon within). So sometimes we're just stuck.

To a degree, we a stuck here... but we can abstract a bit of this into a seperate class and apply a delegation pattern over top of the blocks. That will certainly untangle our view controller code quite a bit, right?

So, for starters, let's get a sort of road map of what our new class will need to do.

What functionality does it need?

  • Set Authentication
  • Configure Session
  • Identify User
  • Authenticate User

This is simply a list of what you're doing with it now.

Meanwhile, what sort of callbacks should we tell our delegate about?

  • Configure Session failed.
  • Configure Session succeeded.
  • User Identified.
  • User not registered.
  • User identification failed.
  • User authenticated.
  • User authentication failed.

Now, with these two lists, we can pretty well write the interface for what our class will look like, right? First a protocol:

@objc protocol GoogleCloudManagerDelegate {
    func cloudManager(manager: GoogleCloudManager, didFailSessionConfiguration error: NSError!)
    func cloudManagerDidConfigureSession(manager: GoogleCloudManager)
    func cloudManager(manager: GoogleCloudManager, didIdentifyUser ticket:GTLServiceTicket!, response:AnyObject!)
    func cloudManagerFoundUnregisteredUser(manager: GoogleCloudManager)
    func cloudManager(manager: GoogleCloudManager, didFailIdentifyingUser error: NSError!)
    func cloudManager(manager: GoogleCloudManager, didFinishAuthentication auth: GTMOAuth2Authentication, viewController: GTMOAuth2ViewControllerTouch)
    func cloudManager(manager: GoogleCloudManager, authenticationDidFailWithError error: NSError!)
}

So, that's a first draft at the protocol. May take some tweaking, but from here, I think you'll start to get the hang of it. (We're marking it with @objc so we can use weak in the class.)

Now we need a class that can perform all of this stuff and handle the call backs.

We need a weak var for our delegate variable, and then we need to implement methods for each of the four things we'll want to let the user do.

The gist of it go something like this:

class GoogleCloudManager : NSObject {
    weak var delegate: GoogleCloudManagerDelegate?

    init(delegate: GoogleCloudManagerDelegate? = nil) {
        self.delegate = delegate
    }

    func configureSession() {
        ASSession.sharedSession.configure() { (configError:NSError?) -> Void in
            if let error = configError  {
                self.delegate?.cloudManager(self, didFailSessionConfiguration: error)
            }else{
                self.delegate?.cloudManagerDidConfigureSession(self)
            }
        }
    }
}

From here, you should be able to figure out how to implement the other methods. The point is, we're putting all of the block-based code in this data class that doesn't do anything at all with the view. In the callback blocks, we're doing very minimal logic, to determine which methods we should call on our delegate.

In that configureSession() method, we may even want to add a optional func cloudManagerWillConfigureSession(manager: GoogleCloudManager) to our protocol, and optionally call that on our delegate just before configuring:

func configureSession() {
    self.delegate?.cloudManagerWillConfigureSession?(self)
    ASSession.sharedSession.configure() { (configError:NSError?) -> Void in
        if let error = configError  {
            self.delegate?.cloudManager(self, didFailSessionConfiguration: error)
        }else{
            self.delegate?.cloudManagerDidConfigureSession(self)
        }
    }
}

Now, for the final step, putting all the pieces together. Our view controller needs to declare itself as conforming to our nearly created GoogleCloudManagerDelegate protocol, implement the required method, optionally implement the optional methods, add an instance variable for the manager class, and call its functions.

class ViewController : UIViewController, GoogleCloudManagerDelegate {
    let cloudManager = GoogleCloudManager(delegate: self)

    override func viewDidLoad() {
        if let auth: GTMOAuth2Authentication = GoogleEndpointAssistant.rebuildGTMOAuth2AuthenticationFromUserDefaults(){
            self.cloudManager.setAuthentication(auth)
            self.cloudManager.configureSession()
        }
    }

    func cloudManager(manager: GoogleCloudManager, didFailSessionConfiguration error: NSError!) {
        // code to do when session config fails
    }

    func cloudManagerDidConfigureSession(manager: GoogleCloudManager) {
        // code to do when session config succeeds
    }

    // etc.
}

Now we've untangled our code quite a bit. We've also made a reusable class so if we need to implement access through the Google Cloud stuff from other view controllers, it's a simple as implementing these delegate methods and calling methods on this class.

share|improve this answer
    
Nice answer, Thanks for Chiming In! –  sᴉɔuɐɹɥԀ Apr 3 at 2:49

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.