10
\$\begingroup\$

In my app, the user can upvote a cycling track if they want to ride on it.

Note: I do understand I can use a boolean column to check if a user has voted or not, but, I am just testing at the moment.

My logic works the following way:

First, it needs to determine what user is accessing the program, which is done by sending a query to find their lastName and gather their objectId and hasVoted score.

The program works out which course the user is currently looking at and assigns it to the currentCourse variable (just for testing). Then, it queries the Votes database, and tells the user if they have voted.

If hasVoted is equal to 1, then the user has voted, otherwise it'd equal 0, meaning they haven't.

After hasVoted comes back with 0 the program queries the database class Votes again, but, this time increments the Votes (row that shows total votes) and hasVoted.

Is there a better or cleaner way to express this logic?

@IBAction func voteUp(sender: AnyObject) {

    let courseOne = "DFGhKzW5Ze"

    // Assign the "currentCourse" variable to the correct course
    var currentCourse = courseOne

    // Check which course the user is voting for
    var userVotes: Int!
    var userId: String!

    // Query the user database to find their hasVoted & their object ID (based on "lastName")

    var query = PFQuery(className:"Users")
    query.whereKey("lastName", equalTo:"\(self.lastName)")
    query.findObjectsInBackgroundWithBlock {
        (objects: [AnyObject]?, error: NSError?) -> Void in

        if error == nil {
            // The find succeeded.
            println("Successfully retrieved \(objects!.count) scores.")
            // Do something with the found objects
            if let objects = objects as? [PFObject] {
                for object in objects {
                    userVotes = object["hasVoted"] as! Int!
                    userId = object.objectId!
                    println(userVotes)
                }
            }
                    // If "hasVoted" is 0 then the user can vote
                    if userVotes == 0 {
                        var query = PFQuery(className:"Votes")
                        // Each Course has its own unique objectID
                        query.getObjectInBackgroundWithId("\(currentCourse)") {
                            (votes: PFObject?, error: NSError?) -> Void in
                            // Increments the Vote and whether or not the user has Voted
                            votes!.incrementKey("Votes")
                            votes!.saveInBackgroundWithBlock {
                                (success: Bool, error: NSError?) -> Void in
                                // If it succeeds then increment the "hasVoted" for the user
                                if (success) {
                                    var query = PFQuery(className:"Users")
                                    // Grab userId to increment for required user
                                    query.getObjectInBackgroundWithId("\(userId)") {
                                        (votes: PFObject?, error: NSError?) -> Void in
                                        // Increments "hasVoted" by 1
                                        votes!.incrementKey("hasVoted")
                                        votes!.saveInBackgroundWithBlock {
                                            (success: Bool, error: NSError?) -> Void in
                                            if (success) {
                                                println("You have sucessfully Voted")

                                            } else {
                                                // There was a problem, check error.description
                                            }
                                        }
                                    }

                                } else {
                                    // There was a problem, check error.description
                                }
                            }
                        }
                    } else {
                        // Ther user has already voted
                        println("You have already voted for this course")
                    }

        } else {
            // Log details of the failure
            println("Error: \(error!) \(error!.userInfo!)")
        }
    }
}
\$\endgroup\$
13
\$\begingroup\$

Lately, when writing Code Review posts, I like to point out the first thing I notice when I look at someone code.

enter image description here

This is the first thing I notice when I look at your code.

Without looking at anything else, I can already tell you that your code is way too deeply nested and therefore absolutely confusing.

With looking at your code, I can tell you that not counting any of the if/else block nesting, you've quadruple nested "background" blocks.

Your code boils down to this:

query.findObjectsInBackgroundWithBlock {
    // do stuff
    query.getObjectInBackgroundWithId("\(currentCourse)") {
        // do stuff
        votes!.saveInBackgroundWithBlock {
            // do stuff
            query.getObjectInBackgroundWithId("\(userId)") {
                // do stuff
                votes!.saveInBackgroundWithBlock {
                    // do stuff
                }
            }
        }
    }
}

This is impossible to follow once you have the actual code in.

We need to break these things down into individual methods.

So, start from the inside and work your way out.

What's the inner-most piece of logic doing? Incrementing vote count.

func incrementVoteCount(#user: PFObject, completion: (Bool, NSError?) -> Void) {
    user.incrementKey("hasVoted")
    user.saveInBackgroundWithBlock(completion)
}

Now step out a level, what are we doing? Fetching a user (so we can update the user's vote count), right?

func incrementVoteCount(#userID: String, completion: (Bool, NSError?) -> Void) {
    let query = PFQuery(className:"Users")
    query.getObjectInBackgroundWithId("\(userId)") { user, error in 
        if let user = user {
            incrementVoteCount(user: user, completion: completion)
        } else {
            // handle error
        }
    }
}

Continue this logic all the way out. Each time you're passing a closure, break that out into a separate method.

I'd also uncouple the logic slightly I think. It's hard for me to follow what you're doing exactly... and I'm not familiar with what Parse allows, but in the end, you're making five separate network request just to make a single vote. I don't know what you can do with Parse, but it really seems to me that this should be one simple network request.

I mean, that doesn't count the network request to get the current user, and it doesn't count the request to get the list of courses that the user is voting on. But you should have both of these before the user decides what to vote for. And then ideally, you should have a simple method that looks something like this:

user.voteForCourse(course) { success, error in 
    // vote for course completion block
}

And it should only make a single network request, passing the user ID and the course ID or whatever is needed to figure out which user and which course.

Then the server side takes care of the rest (makes all these checks). If the user can't vote for whatever reason, the server should return false for success and report an error (something like "User has already voted.").

... but I don't know what's available with Parse.com. If you were writing your own API endpoint, this is definitely what you'd want to set up.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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