4
\$\begingroup\$

I have a "Like" button in my iOS app that updates the votes every time it's tapped. Just like Facebook, Instagram, ProductHunt, etc.

I have a custom cell where that IBAction and IBOutlet is declared and that's where I'm making the query to the Parse db to update the UI (initially I'm querying the vote values from the other file from the cellForRowAtIndexPath.) In the custom cell file, I also created a delegate method, with a Protocol type that I'm accessing in my TableViewController in the cellForRowAtIndexPath.

I'm a bit of a newbie and studying the MVC data structure (assuming that's the biggest issue here), so please be easy on me!

Here is the code in the custom cell:

protocol CellButtonDelegate {

    func buttonClicked(cell : PFTableViewCell)

}
var delegate : CellButtonDelegate?

class FeedTableViewCell: PFTableViewCell {

    var delegate : CellButtonDelegate?

    var parseObject:PFObject?


    internal var buttonEnabled : Bool? {

        get {

            return loveOutlet.enabled

        } set {

            loveOutlet.enabled = newValue!
        }
    }

 @IBAction func loveButton(sender: AnyObject) {

        if(parseObject != nil) {

            if var votes: Int? = parseObject!.objectForKey("votes") as? Int {
                votes!++

                parseObject!.setObject(votes!, forKey: "votes")
                parseObject!.saveInBackground()

                print(votes!)
            }

            delegate?.buttonClicked(self)
        loveOutlet.enabled = false

        }

    }

Here is the code in the cellForRowAtIndexPath:

   override func tableView(tableView: UITableView, cellForRowAtIndexPath indexPath: NSIndexPath, object: PFObject?) -> PFTableViewCell? {

        var cell: FeedTableViewCell? = tableView.dequeueReusableCellWithIdentifier(cellIdentifier) as? FeedTableViewCell

        if(cell == nil) {

            cell = NSBundle.mainBundle().loadNibNamed("FeedTableViewCell", owner: self, options: nil)[0] as? FeedTableViewCell
        }

        objects![indexPath.row]

        if let pfObject = objectArray?[indexPath.row] {
                     cell?.parseObject = object 
var votes: Int? = pfObject["votes"] as? Int
            if votes == nil {
                votes = 0 
            }


            cell?.delegate = self

            let isEnabled = pfObject["isEnabled"] as! Int
            if isEnabled == 1 {

                cell?.buttonEnabled = true

            } else {

                cell?.buttonEnabled = false

            }

            print(pfObject["isEnabled"] as? Bool)
}

}


func buttonClicked(cell: PFTableViewCell) {

    let indexPath = tableView.indexPathForCell(cell)

    var object : PFObject = objectArray![indexPath!.row] as! PFObject

    object["isEnabled"] = 0
    objectArray?.removeAtIndex((indexPath?.row)!)
    objectArray?.insert(object, atIndex: (indexPath?.row)!)


}

Currently, when the button is tapped, the UI updates, but it also sends a query to the db (the activity indicator/internet indicator at the top right of the iPhone is spinning every time I "like" something).

When I use other apps, like Facebook for example, the query isn't being immediately sent and updated, it does it at a later point (don't understand when or how; can't figure that out but it does seem more efficient).

Lastly, sometimes an individual cell updates, and on other (awful) days the vote appears for every 3rd cell. Trying to figure this out for days.

In summary: ultimately, I want to optimize the query for voting in an iOS app. How do Facebook, Instagram and everyone else do it so smoothly?

\$\endgroup\$
  • \$\begingroup\$ Top apps use a technique called optimistic programming. They assume the like will get sent to their server so they update it locally to appear faster. You can read more here fastcodesign.com/1669788/… but its a fairly complicated task to construct a model that can compare local "truth" with server-side truth \$\endgroup\$ – Daniel Galasko Dec 3 '15 at 7:57
  • \$\begingroup\$ Wow, great article. Will look deeper into this. But yes I agree, this is fairly complicated. I should be tackling other issues in the meantime! And then get to this if and when the time calls for it! Thank you \$\endgroup\$ – Lukesivi Dec 3 '15 at 10:12
  • 1
    \$\begingroup\$ Definitely, always be conscious of premature optimisation -> programmers.stackexchange.com/questions/80084/… \$\endgroup\$ – Daniel Galasko Dec 3 '15 at 11:34
  • \$\begingroup\$ Thanks for this. I'm new to programming so these tips help out a lot. Very much appreciated. \$\endgroup\$ – Lukesivi Dec 3 '15 at 12:07
5
\$\begingroup\$

Formatting

On the whole, there are several indentation problems through out your code. Here is made the most obvious example:

        if let pfObject = objectArray?[indexPath.row] {
                     cell?.parseObject = object 
var votes: Int? = pfObject["votes"] as? Int

The second line has two extra levels of indentation over what it should have. The third line needs three more indentation levels to be in the correct place.

Xcode will pretty much put every single line of code exactly where it belongs. All you have to do to get your indentation correct is simply not fight Xcode. Even if you disagree with Xcode about how something should be indented, it's usually a lot easier to just do it Xcode's way because everyone else tends to give in to Xcode... and it's a lot less effort. (For a personal example, I tend to feel like case statements within a switch should be indented one level from the switch statement, but Xcode disagrees... and it's a lot less work for me to just let Xcode have it its way.)


Delegates

var delegate : CellButtonDelegate?

Delegates should always be weak. Without making a delegate weak, you're almost always setting up some sort of a retain cycle. But what's more important, people expect for delegates to be marked as weak. If you want to replicate the typical "delegate" pattern, but for some reason need the default strong reference, then you need to call your property by something other than "delegate".


Computed Properties

I have several comments about this property:

internal var buttonEnabled : Bool? {

    get {

        return loveOutlet.enabled

    } set {

        loveOutlet.enabled = newValue!
    }
}

First of all, internal is the default access level. As you marked nothing else as public or private, everything in your code actually has this internal access level. Above all else, we should prefer consistency. If you're going to mark internal things with the internal keyword, you're now going to need to mark every property, method, extension, protocol, class, struct, enum, and function (and whatever else I forgot) with its access modifier. Anything else is inconsistent. But marking them all is too verbose. We all know (or should know) that internal is the default level, and if we mark all the internal things as internal, it just becomes an unconstructive cluttered keyword. I would only ever recommend marking something as internal in a class where basically everything else is already marked as public or private. In that case, internal indicates "No, I didn't forget to mark this one--yes, it really is internal, I promise."

Next, there's a whole lot of white space in this. In fact, you consistently have a ton of vertical white space throughout the entirety of your code. Let's use white space more constructively, shall we? Every single line of this property is all associated to this property. By eliminating the blank lines in the middle, it helps the eye associate all of these lines together visually (in a way far more immediately obvious than the curly braces do).

Finally, let's talk about optionals... Your loveOutlet doesn't appear to be defined in this file, so I don't know if it is an optional or if it is a non-optional but the enabled property is optional. I don't know what has led you to marking this buttonEnabled property as a Bool? rather than a non-optional Bool, but based on the force-unwrapping in your setter, it seems clear that this should probably be a non-optional. We should allow the user to pass nil for the buttonEnabled property--how can that even make sense? It clearly doesn't make sense, because if they ever do, we simply crash. This is supremely bad design. Swift gives you the power to clearly dictate which properties can and cannot be set to nil. What you've done here is created a property that can be set to true, false, or "crash my app please".

So more realistically, this property should look about like this:

var buttonEnabled: Bool {
    get {
        return loveOutlet.enabled
    } 
    set {
        loveOutlet.enabled = newValue
    }
}

If loveOutlet is optional (in this case, for it to compile, it'd have to be an implicitly unwrapped optional), or its enabled property is optional, then we can go for one of these two options in the get:

loveOutlet?.enabled ?? false

or

loveOutlet.enabled ?? false

if true then true else false

let isEnabled = pfObject["isEnabled"] as! Int
if isEnabled == 1 {

    cell?.buttonEnabled = true

} else {

    cell?.buttonEnabled = false

}

Here, we see another issue with overuse of vertical white space, poor optional management, and a pattern that I simply don't understand: if true then true else false.

This as! Int can cause an app crash in multiple ways. First, the value can simply not exist for the key. Second, the value could exist, but not be convertible to an Int. If this is a dictionary representing JSON, the value could be a string, floating point, or bool. If it's not representing JSON, it could be anything. But we can rewrite this safer and using significantly fewer lines:

cell?.buttonEnabled = (pfObject["isEnabled"] as? Int) == 1

Although, arguably, we'd really want to fix our model so that the value for the "isEnabled" key is actually a bool, then we can write something that makes a bit more sense:

cell?.buttonEnabled = (pfObject["isEnabled"] as? Bool) ?? false

Optional Safety

It's really unfortunate that being safe with optionals is an optional thing at all. It seems mostly a hold-over from the short period of time after Swift was introduced and before Objective-C's nullability annotations.

Despite this, we should still strive to always be as safe as possible with our optionals. Force-unwrapping nil is always a crash.

if(parseObject != nil) {

        if var votes: Int? = parseObject!.objectForKey("votes") as? Int {
            votes!++

            parseObject!.setObject(votes!, forKey: "votes")
            parseObject!.saveInBackground()

            print(votes!)
        }

        delegate?.buttonClicked(self)
    loveOutlet.enabled = false

    }

Here, we have not been safe with our optionals. And the worst part here is that there's not even an imaginably good reason for this lack of safety--we're checking for nil in an if statement and only force-unwrapping within that block.

The most dangerous problem with this is that it's not thread safe. If thread 1 checks that foo is non-nil, finds it is not, then enters the if block, then thread 2 sets foo (an instance variable) to nil, then back on thread 1, we're in the if block and force unwrap, we've now crashed (despite having checked against nil). This is why we should prefer the if let construct.

if let pObj = parseObject {
    // safely use pObj as a constant, unwrapped, thread-safe, locally-scoped version of parseObject
}

The next line is particularly strange. It's a clear indication that we're aware of the if let/if var syntax, but we've used it in a mysterious and strange way.

(For the record, I absolutely never want to see votes!++ in my code base, and if a developer on my project checked that code in, I'd be looking to move that dev to a different platform. It's horrendous...)

You've used if var to properly check for nil... but then you specify the type as an optional which still has to be unwrapped, despite having already checked against nil. If we simply don't specify the type, votes would be a non-optional that doesn't need to be force unwrapped.

We should also generally prefer let variables to var variables. In fact, we should default to let and only change it to var when we absolutely must modify the value. I don't think votes is something we absolutely must modify.

Finally, keep in mind we can unwrap multiple values in a single if let statement. I'd prefer to see this code snippet as something more like this:

if let 
    pObj = parseObject, 
    votes = pObj.objectForKey("votes") as? Int {
        pObj.setObject(votes + 1, forKey: "votes")
        pObj.saveInBackground()
}
delegate?.buttonClicked(self)
loveOutlet.enabled = false

(The actual logic for when to call buttonClicked() and set the outlet disabled may be slightly different, but I wanted to provide you with this example.)

|improve this answer|||||
\$\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.