Review Request - Android CountDownTimer activity

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 OnClickListener

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.

StackExchange Code Review Q#31295, answer score: 2