Review Request - Android CountDownTimer activity

Question

I'm new to Java and Android. While I've created a project that seems to work, I'd like some opinions on how to make it better.

The app is going to be used as an interval workout timer/music player for Android devices. I know there's a ton of these already, but I thought it'd be a good way to learn the language, and it's useful for me. I haven't gotten the music player piece working yet. I was thinking of letting you select between selecting a playlist to play or using the 8tracks.com API to play streaming music. Comments are welcome, too.

The code can be seen in full in this Github.

Here's the main package:

package com.blueflamesys.workoutstopwatch;

import android.app.Activity;
import android.content.Intent;
import android.content.SharedPreferences;
import android.os.Bundle;
import android.os.CountDownTimer;
import android.preference.PreferenceManager;
import android.util.Log;
import android.view.Menu;
import android.view.MenuInflater;
import android.view.MenuItem;
import android.view.View;
import android.view.View.OnClickListener;
import android.widget.Button;
import android.widget.TextView;

public class WorkoutStopwatch extends Activity implements OnClickListener {

private static final String TAG = "WorkoutMain";
private static final String API_KEY = "30eee341eb88a9647b40d187faf508b50ca318d9";
private boolean timerRunning, isPaused = false;
private long millisLeft = 0;
private WorkoutTimer workoutTimer;
private enum CounterState {
    UNKNOWN(0),
    WORKOUT(1), 
    REST(2);

    private int stateValue;

    CounterState(int state) {
        this.stateValue = state;
    }

    public int getStateValue() {
        return stateValue;
    }

    public void setStateValue(int state) {
        this.stateValue = state;
    }       
}
CounterState state = CounterState.WORKOUT;


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

    // Set up OnClickListeners
    ((Button) findViewById(R.id.start_button)).setOnClickListener(this);
    ((Button) findViewById(R.id.pause_button)).setOnClickListener(this);
    ((Button) findViewById(R.id.reset_button)).setOnClickListener(this);

}

@Override
public void onClick(View v) {
    switch(v.getId()) {
    case R.id.start_button:
        Log.d(TAG, "clicked on Start Button");
        if (!isPaused && !timerRunning) {
            Log.d(TAG, "Wasn't paused and wasn't running...");

            timerRunning = true;

            // If the state is unknown, set it to Workout first
            int State = state.getStateValue();
            if (State == 0) {
                state.setStateValue(1);
            }
            workoutTimer = new WorkoutTimer(50);
            workoutTimer.start();

        } 

        if (isPaused) {
            timerRunning = true;
            this.resumeTimer();
            Log.d(TAG, "Was paused resumed timer...");
        }

        break;
    case R.id.pause_button:
        Log.d(TAG, "clicked on Pause Button");

        if (timerRunning) {
            timerRunning = false;
            this.pauseTimer();
            Log.d(TAG, "Paused timer...");
        }

        break;
    case R.id.reset_button:

        Log.d(TAG, "clicked on Reset Button");

        if (!timerRunning) {
            Log.d(TAG, "Wasn't running...");
            isPaused = false;
            TextView digital_display = (TextView) findViewById(R.id.digital_display);
            digital_display.setText("00:00.0");
        }
        break;
    }
}

@Override
public boolean onCreateOptionsMenu(Menu menu) {
    super.onCreateOptionsMenu(menu);
    MenuInflater inflater = getMenuInflater();
    inflater.inflate(R.menu.stopwatch_settings, menu);
    return true;
}

@Override
public boolean onOptionsItemSelected(MenuItem item) {
    switch (item.getItemId()) {
    case R.id.settings:
        startActivity(new Intent(this, Prefs.class));
        return true;
    case R.id.about:
        Intent i = new Intent(this, About.class);
        startActivity(i);
        break;
    }
return false;
}

public void pauseTimer() {
    isPaused = true;
    SharedPreferences workoutTimerDB = getSharedPreferences("workoutTimerDB", MODE_PRIVATE);
    SharedPreferences.Editor edit = workoutTimerDB.edit();
    edit.putLong("millisLeft", (Long)this.millisLeft);
    edit.commit();
    workoutTimer.cancel();
}

public void resumeTimer() {
    if (isPaused) {
        isPaused = false;
        SharedPreferences workoutTimerDB = getSharedPreferences("workoutTimerDB", MODE_PRIVATE);
        millisLeft = workoutTimerDB.getLong("millisLeft", 0);
        workoutTimer = new WorkoutTimer(millisLeft, 50);
        workoutTimer.start();
    }
}

private class WorkoutTimer extends CountDownTimer{
    public WorkoutTimer(long interval) {
        super(getThisTime(), interval);
        Log.d(TAG, "WorkoutTimer Constructed with interval only...");
    }

    public WorkoutTimer(long millis, long interval) {
        super(millis, interval);
        Log.d(TAG, "WorkoutTimer Constructed with both...");
    }

    public void onFinish() {
        int State = state.getStateValue();
        int roundsLeft = 0;

        if (State == 1) {
            state.setStateValue(2);
        } else {
            state.setStateValue(1);
            decrementRounds();
        }

        Log.d(TAG, "WorkoutState = " + state.getStateValue());

        try {
            TextView numOfRounds = (TextView) findViewById(R.id.number_of_rounds);

            roundsLeft = Integer.parseInt(numOfRounds.getText().toString());
        } catch(NumberFormatException nfe) {
            roundsLeft = 999;
        }

        if (roundsLeft > 0) { // If there's still rounds, start the timer.
            workoutTimer = new WorkoutTimer(100);
            workoutTimer.start();
        } else { // Stop the timer and reset the display
            workoutTimer.cancel();

            TextView digital_display = (TextView) findViewById(R.id.digital_display);
            digital_display.setText("00:00.0");
        }
    }

    public void onTick(long millisUntilFinished) {
        final long minutes_left = ((millisUntilFinished / 1000) / 60);
        final long seconds_left = (millisUntilFinished / 1000) - (minutes_left * 60);
        final long millis = millisUntilFinished % 100;
        final String time_left = String.format("%02d:%02d.%01d", minutes_left, seconds_left, 
                millis/10);

        TextView digital_display = (TextView) findViewById(R.id.digital_display);
        digital_display.setText(time_left);
        millisLeft = millisUntilFinished;
    }

    @Override
    protected void finalize() throws Throwable {
        super.finalize();
        Log.d(TAG, "Object destroyed");
    }
}

private long getThisTime() {
    long time = 0;

    TextView workout_time = (TextView) findViewById(R.id.workout_time);
    TextView rest_time = (TextView) findViewById(R.id.rest_time);

    switch(state.getStateValue()) {
    case 1:
        try {
                time = Integer.parseInt(workout_time.getText().toString());
            } catch(NumberFormatException nfe) {
                time = 999;
            } 
        Log.d(TAG, "Workout time = " + time);
        break;
    case 2:
        try {
                time = Integer.parseInt(rest_time.getText().toString());
            } catch(NumberFormatException nfe) {
                time = 999;
            } 
        Log.d(TAG, "Rest time = " + time);
        break;
    case 0:
        time = 0;
        break;
    }
    return time * 1000;
}

private void decrementRounds() {
    int roundsLeft = 2;

    TextView numOfRounds = (TextView) findViewById(R.id.number_of_rounds);

    try {
        roundsLeft = Integer.parseInt(numOfRounds.getText().toString());
    } catch(NumberFormatException nfe) {
        roundsLeft = 999;
    }

    roundsLeft -= 1;

    numOfRounds.setText(String.valueOf(roundsLeft));
    Log.d(TAG, "set rounds to = " + roundsLeft);
}

}

Don't hold back, and feel free to ask any questions or give any criticisms.

Answer 1

Your usage of enums

You are misusing your enums. I would get rid of the getStateValue and setStateValue methods. The enum itself is a state.

I would change code that looks like this:

        int State = state.getStateValue();
        if (State == 0) {
            state.setStateValue(1);
        }

To this:

        if (state == UNKNOWN) {
            state.setStateValue(WORKOUT);
        }

What your original code here actually does is that it's taking the state value, which you initialize to CounterState.WORKOUT and then it's changing the internal integer value stored in the enum. You never change your state variable from WORKOUT, but instead you change the integer value of the WORKOUT-state. That's definitely not how enums should be used.

(You're also using the variable name State which defies Java coding conventions that variable names should start with a lower case letter)

So, by changing the code snippet in the way I suggested above you will get rid of a lot of the "magic numbers" 0, 1 and 2 by using your enums properly. You will also improve readability. You won't have to remember "Which state is 2" anymore.

I would also mark the enum type itself with static like this: private static enum CounterState { because the CounterState is not dependent on anything in your outer class). I would also mark its constructor with private - or rather, I would get rid of the constructor entirely. There is no use for it when you remove the getStateValue and setStateValue methods. You can use state.ordinal() if you really want to access its int value (which you should generally try to avoid but is useful for things like serialization). The ordinal() method returns the index of the enum.

Other variables

I would rename your isPaused to simply paused. isPaused is a naming convention for a method that returns your internal paused boolean value. You also have one boolean for if the timer is running (timerRunning) and one for if the timer is paused. This confuses me a lot. If the timer isn't running, then it's paused isn't it? Or should the possible options be STOPPED, PAUSED, RUNNING? In that case make it an enum (without any getters and setters!).

On click

I also agree with @thepoosh about defining onClick for your buttons in the XML, unless you're targeting Android API < 4 (which no-one really uses anymore). For information about using onClick in your XML, see the Android documentation.

Answer 2

from the little I saw I have just a few comments (the code looks fairly fine)

1. if all you do with the buttons is set the onClickListener there is no reason not to use the built in xml attribute onClick when designing the buttons.

2. when editing or reading from SharedPrefrences you should probably synchronize it like this:

   public static final String WORKOUT_SHARED_PREFS = "workoutTimerDB";
   public void pauseTimer() {
   isPaused = true;
   synchronized(WORKOUT_SHARED_PREFS) {
           SharedPreferences workoutTimerDB = getSharedPreferences(WORKOUT_SHARED_PREFS, MODE_PRIVATE);
           SharedPreferences.Editor edit = workoutTimerDB.edit();
           edit.putLong("millisLeft", (Long)this.millisLeft);
           edit.commit();
           WORKOUT_SHARED_PREFS.notifyAll();
       }
       workoutTimer.cancel();
   }