I have to say your code is clean and well thought-through. There is always space of improvement though.
Cut the fluff. Reduce your access modifiers. There is no (visible) reason for
running not to be
And no reason why timer-controlling methods have to be
public, either. Indeed, this is not how you would want the external word to communicate with your Activity - in the Android world, Activities have their own life-cycle and they get shut down and recreated by the OS, meaning you don't normally want to hold on to any particular instance of an Activity.
(Also, don't use redundant arguments: your timer-controlling methods don't make any use out of your
Due to the lifecycle thing I mentioned - more info at http://developer.android.com/training/basics/activity-lifecycle/index.html - you should check whether the Activity is still there before you call
Attempting to update a UI that's no longer in sight (and therefore gone) will result in a crash. How could it happen? Very simple - let's say your timer is running, and there's an incoming call in between two ticks. Or user switches to another app, or dims the screen. Etc. And it's fairly nondeterministic, especially when the device is running short on memory.
Handler is a convenient way of sending actions to the thread message queue. And this trick with your
Handler calling itself is an easy way of ensuring that "ticks" repeat over and over (without using the actual
Timer, whose API is a bit clunky).
Handler is a perpetuum mobile. It will prevent the
Activity from ever getting garbage-collected. Even if it refrains from touching the UI if it no longer exists, it would keep on going. You should kill it if user exits the Activity - see https://stackoverflow.com/a/20377022/168719
To minimize these problems, I'd recommend extracting the timer logic into a separate class, and have your StopWatchActivities (note the plural: you shouldn't assume there'll only be a single instance!) subscribe and unsubscribe from it in their
Another thing: a
Handler by default runs on the same thread on which it was created, in this case it's the UI thread. This is okay in this case because the task is light-weight and pretty much limited to updating the UI (which has to be done on the UI thread anyway). If it was computationally expensive though, you'd have to delegate it to another thread, but then make sure to eg. call the
I would also have a few cosmetic remarks related to naming.
stopTimer should rather be
pauseTimer, because it doesn't reset it.
having variables named
seconds is a bit confusing. I'd prefer
secondsElapsed - it distinguishes them better.
I'm not sure what's exactly wrong with the code from your link - for starters, I can't see where the 1 second delay between ticks is enforced?? - but it proves my point as for why having
sec next to
seconds isn't a good idea:
int sec = seconds % 60;
String time = String.format("%02d:%02d:%02d", hours, minutes, seconds);
Surely the author meant to pass
String.format, no? :)