10
\$\begingroup\$

I'm learning Swift and would appreciate any code review suggestions (the more nit-picky, the better). In particular, I'm wondering if there's a more efficient way to format the time in the timeToString function I wrote, since I'm repeating myself three times.

import UIKit
import Foundation

class ViewController: UIViewController {


    @IBOutlet var timerLabel: UILabel!

    var timer = NSTimer()
    var count = 0

    @IBAction func start(sender: AnyObject) {
      timer = NSTimer.scheduledTimerWithTimeInterval(1, target: self, selector: Selector("result"), userInfo: nil, repeats: true)
    }

    @IBAction func stop(sender: AnyObject) {
      timer.invalidate()
    }

    @IBAction func reset(sender: AnyObject) {
      timer.invalidate()
      count = 0
      let (h, m, s) = convertSeconds(count)

      timerLabel.text = timeToString(h, m: m, s: s)
    }

    func result(){
      count++
      let (h, m, s) = convertSeconds(count)

      timerLabel.text = timeToString(h, m: m, s: s)
    }

    func convertSeconds(totalSeconds: Int) -> (h:Int, m:Int, s:Int){
      var seconds = totalSeconds % 60
      var minutes = (totalSeconds / 60) % 60
      var hours = totalSeconds / 3600
      return (hours, minutes, seconds)
    }

    func timeToString(h:Int, m:Int, s:Int) -> String {

      var h = String(format: "%02d", h)
      var m = String(format: "%02d", m)
      var s = String(format: "%02d", s)
      return "\(h)h \(m)m \(s)s"
    }

    override func viewDidLoad() {
      super.viewDidLoad()
      let (h, m, s) = convertSeconds(count)

      timerLabel.text = timeToString(h, m: m, s: s)
    }

    override func didReceiveMemoryWarning() {
      super.didReceiveMemoryWarning()
    }

}
\$\endgroup\$
10
\$\begingroup\$

The primary problem I see here is the way we're keeping track of the time passed. NSTimer is not guaranteed to tick with the exact amount of time you passed it. It will try getting as close as possible, but the most specific problem is when the thread has a lot of activity on it.

If between two ticks, the thread the timer is one gets a lot of activity stuck on it, then our timer can start displaying a strange time.

So, instead of using a count variable as an int, let's use a startTime as an NSDate object. We may also need an NSTimeInterval variable in the case when the timer is starter, then stopped, then restarted without being reset.

Once this is done, the start method will do something like this:

func start() {
    self.startTime = NSDate()
    self.updateUI()
    timer.invalidate()
    timer = NSTimer.scheduledTimerWithTimeInterval(1.0f, target: self, selector: Selector("updateUI"), userInfo: nil, repeats: true)
}

while the stop method looks more like this:

func stop() {
    timer.invalidate()
    self.accumulatedTime += NSDate().timeIntervalSinceDate(self.startTime)
    self.updateUI()
}

and the reset method looks something like this:

func reset() {
    timer.invalidate()
    self.accumulatedTime = 0
    self.start()
}

We can replace convertSeconds and timeToString with the following method:

func timeIntervalToString(ti: NSTimeInterval) -> String? {
    let dcf = NSDateComponentsFormatter()
    dcf.zeroFormattingBehavior = .Pad
    dcf.allowedUnits = (.CalendarUnitHour | .CalendarUnitMinute | .CalendarUnitSecond)
    return dcf.stringFromTimeInterval(ti)
}

And now our updateUI method looks something like this:

func updateUI() {
    var time = self.accumulatedTime + NSDate().timeIntervalSinceDate(self.startTime)
    self.timerLabel.text = self.timeIntervalToString(time)        
}

Now our label should always give us an accurate representation of the time passed in hours, minutes, and seconds no matter how busy the thread is. A busy thread would only mean that the UI isn't updated as often, but in the instant it is updated, it is perfectly accurate.

But more importantly, we're able to track more precisely the exact amount of time passed in case we ever decide we want to display more precision.

Moreover, if we ever decide we want to change the rate at which the UI is updated, we can do that, because our timer doesn't rely on the update method being ticked once per second in order to be accurate. The time could be calculated in any instant, and we will get a precise time.

As one final note, rather than putting all of this code in our view controller, I'd probably abstract this into a stop watch struct. Something like this:

func timeIntervalToString(ti: NSTimeInterval) -> String? {
    let dcf = NSDateComponentsFormatter()
    dcf.zeroFormattingBehavior = .Pad
    dcf.allowedUnits = (.CalendarUnitHour | .CalendarUnitMinute | .CalendarUnitSecond)
    return dcf.stringFromTimeInterval(ti)
}

public struct StopWatch {
    private var startTime: NSDate?
    private var accumulatedTime: NSTimeInterval = 0.0

    public var elapsedTimeInterval: NSTimeInterval {
        get {
            return self.accumulatedTime + NSDate().timeIntervalSinceDate(self.startTime ?? NSDate())
        }
    }

    public var elapsedTimeString: String {
        get {
            return timeIntervalToString(self.elapsedTimeInterval) ?? "0:00:00"
        }
    }

    public mutating func start() {
        self.startTime = NSDate()
    }

    public mutating func stop() {
        self.accumulatedTime += NSDate().timeIntervalSinceDate(self.startTime ?? NSDate())
        self.startTime = nil
    }

    public mutating func reset() {
        self.accumulatedTime = 0.0
        self.start()
    }
}

And this allows us to minimize the amount of code that actually goes into our view controller, which can now look something more like this:

class StopWatchViewController: UIViewController {
    @IBOutlet var timerLabel: UILabel!
    var stopWatch: StopWatch = StopWatch()
    var timer: NSTimer?

    func viewDidAppear(animated: Bool) {
        super.viewDidAppear(animated)
        self.timer = NSTimer.scheduledTimerWithTimeInterval(1.0, target: self, selector: Selector("updateUI"), userInfo: nil, repeats: true)
    }

    func viewDidDisappear(animated: Bool) {
        super.viewDidDisappear(animated)
        self.timer?.invalidate()
    }

    func updateUI() {
        self.timerLabel.text = self.stopWatch.elapsedTimeString
    }

    @IBAction func start(sender: UIButton) {
        self.stopWatch.start()
    }

    @IBAction func stop(sender: UIButton) {
        self.stopWatch.stop()
    }

    @IBAction func reset(sender: UIButton) {
        self.stopWatch.reset()
    }
}

The main point here being that we've abstracted all of the non-UI code out of the view controller class.

\$\endgroup\$

protected by Simon Forsberg Mar 20 '15 at 10:11

Thank you for your interest in this question. Because it has attracted low-quality or spam answers that had to be removed, posting an answer now requires 10 reputation on this site (the association bonus does not count).

Would you like to answer one of these unanswered questions instead?

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