StopWatch app in android

Activity

public class StopWatchActivity extends AppCompatActivity {

    int seconds;
    boolean running;

    @Override
    protected void onCreate(Bundle savedInstanceState) {
        super.onCreate(savedInstanceState);
        setContentView(R.layout.activity_stop_watch);
        runTimer();
    }

    public void startTimer(View view){
        running = true;
    }

    public void stopTimer(View view){
            running = false;
        }

    public void resetTimer(View view){
            running = true;
            seconds = 0;
        }

    public void runTimer(){
        final TextView textView = (TextView) findViewById(R.id.timer);
        final Handler handler = new Handler();
        handler.post(new Runnable() {
            @Override
            public void run() {
                    int hours = seconds / 3600;
                    int minutes = (seconds % 3600) / 60;
                    int sec = seconds % 60;
                    String time = String.format("%d:%02d:%02d", hours, minutes, sec);
                    textView.setText(time);
                if(running) {
                    seconds++;
                }
                handler.postDelayed(this, 1000);
            }
        });

    }
}

I am learning android. I made this app work from my reference book.

I want to know few things.

• How do I improve my code?

• Why should I use handler?

• If other wasy to do this, thenWhy does this approach fails to update the layout?

• What else should I consider in this app in terms of performance?

• If third question cannot be answered in this domain, please leave that.

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 seconds nor running not to be private.

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 View parameter.)

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 textView.setText(time)!

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.

Using a 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).

However, this 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 onResume / onStop methods.

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 textView.setText(time) via Activity.runOnUiThread().

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 sec and seconds is a bit confusing. I'd prefer seconds and totalSeconds or 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 sec, not seconds, to String.format, no? :)

StackExchange Code Review Q#124350, answer score: 4