Problem
The following class is the activity of a little SQLite quiz app, which takes care of showing the actual quiz. Other than that the app only has a starting screen.
I want to save the instance state and restore it. It works, but I wonder if the code is too messy now. I am a beginner of course.
Where should I remove redundancy?
How is this overall approach?
I save texts of TextViews, the button and the RadioButtons with the freezesText=true xml attribute.
public class QuizActivity extends AppCompatActivity {
public static final String EXTRA_SCORE = "extraScore";
private static final long COUNTDOWN_IN_MILLIS = 30000;
private static final String KEY_SCORE = "keyScore";
private static final String KEY_QUESTION_COUNT = "keyQuestionCount";
private static final String KEY_MILLIS_LEFT = "keyMillisLeft";
private static final String KEY_QUESTION_LIST = "keyQuestionList";
private static final String KEY_ANSWERED = "keyAnswered";
private TextView textViewQuestion;
private TextView textViewScore;
private TextView textViewQuestionCount;
private TextView textViewCountDown;
private RadioGroup rbGroup;
private RadioButton rb1;
private RadioButton rb2;
private RadioButton rb3;
private Button buttonConfirmNext;
private ColorStateList textColorDefaultRb;
private ColorStateList textColorDefaultCd;
private CountDownTimer countDownTimer;
private long timeLeftInMillis;
private List<Question> questionList;
private int questionCounter;
private int questionCountTotal;
private Question currentQuestion;
private int score;
private boolean answered;
private long backPressedTime;
@Override
protected void onCreate(Bundle savedInstanceState) {
super.onCreate(savedInstanceState);
setContentView(R.layout.activity_quiz);
textViewQuestion = findViewById(R.id.text_view_question);
textViewScore = findViewById(R.id.text_view_score);
textViewQuestionCount = findViewById(R.id.text_view_question_count);
textViewCountDown = findViewById(R.id.text_view_countdown);
rbGroup = findViewById(R.id.radio_group);
rb1 = findViewById(R.id.radio_button1);
rb2 = findViewById(R.id.radio_button2);
rb3 = findViewById(R.id.radio_button3);
buttonConfirmNext = findViewById(R.id.button_confirm_next);
textColorDefaultRb = rb1.getTextColors();
textColorDefaultCd = textViewCountDown.getTextColors();
if (savedInstanceState == null) {
QuizDbHelper dbHelper = new QuizDbHelper(this);
questionList = dbHelper.getAllQuestions();
questionCountTotal = questionList.size();
Collections.shuffle(questionList);
showNextQuestion();
} else {
questionList = savedInstanceState.getParcelableArrayList(KEY_QUESTION_LIST);
questionCountTotal = questionList.size();
questionCounter = savedInstanceState.getInt(KEY_QUESTION_COUNT);
currentQuestion = questionList.get(questionCounter-1);
score = savedInstanceState.getInt(KEY_SCORE);
timeLeftInMillis = savedInstanceState.getLong(KEY_MILLIS_LEFT);
answered = savedInstanceState.getBoolean(KEY_ANSWERED);
if (!answered) {
startCountDown();
} else {
showSolution();
}
}
buttonConfirmNext.setOnClickListener(new View.OnClickListener() {
@Override
public void onClick(View v) {
if (!answered) {
if (rb1.isChecked() || rb2.isChecked() || rb3.isChecked()) {
checkAnswer();
} else {
Toast.makeText(QuizActivity.this, "Please select an answer", Toast.LENGTH_SHORT).show();
}
} else {
showNextQuestion();
}
}
});
}
private void showNextQuestion() {
rb1.setTextColor(textColorDefaultRb);
rb2.setTextColor(textColorDefaultRb);
rb3.setTextColor(textColorDefaultRb);
rbGroup.clearCheck();
if (questionCounter < questionCountTotal) {
currentQuestion = questionList.get(questionCounter);
textViewQuestion.setText(currentQuestion.getQuestion());
rb1.setText(currentQuestion.getOption1());
rb2.setText(currentQuestion.getOption2());
rb3.setText(currentQuestion.getOption3());
questionCounter++;
textViewQuestionCount.setText("Question: " + questionCounter + "/" + questionCountTotal);
answered = false;
buttonConfirmNext.setText("Confirm");
timeLeftInMillis = COUNTDOWN_IN_MILLIS;
startCountDown();
} else {
finishQuiz();
}
}
private void startCountDown() {
countDownTimer = new CountDownTimer(timeLeftInMillis, 1000) {
@Override
public void onTick(long millisUntilFinished) {
timeLeftInMillis = millisUntilFinished;
updateCountDownText();
}
@Override
public void onFinish() {
timeLeftInMillis = 0;
updateCountDownText();
checkAnswer();
}
}.start();
}
private void updateCountDownText() {
int minutes = (int) (timeLeftInMillis / 1000) / 60;
int seconds = (int) (timeLeftInMillis / 1000) % 60;
String timeFormatted = String.format(Locale.getDefault(), "%02d:%02d", minutes, seconds);
textViewCountDown.setText(timeFormatted);
if (timeLeftInMillis < 10000) {
textViewCountDown.setTextColor(Color.RED);
} else {
textViewCountDown.setTextColor(textColorDefaultCd);
}
}
private void checkAnswer() {
answered = true;
countDownTimer.cancel();
RadioButton rbSelected = findViewById(rbGroup.getCheckedRadioButtonId());
int answerNr = rbGroup.indexOfChild(rbSelected) + 1;
if (answerNr == currentQuestion.getAnswerNr()) {
score++;
textViewScore.setText("Score: " + score);
}
showSolution();
}
private void showSolution() {
rb1.setTextColor(Color.RED);
rb2.setTextColor(Color.RED);
rb3.setTextColor(Color.RED);
switch (currentQuestion.getAnswerNr()) {
case 1:
rb1.setTextColor(Color.GREEN);
textViewQuestion.setText("Answer 1 is correct");
break;
case 2:
rb2.setTextColor(Color.GREEN);
textViewQuestion.setText("Answer 2 is correct");
break;
case 3:
rb3.setTextColor(Color.GREEN);
textViewQuestion.setText("Answer 3 is correct");
break;
}
if (questionCounter < questionCountTotal) {
buttonConfirmNext.setText("Next");
} else {
buttonConfirmNext.setText("Finish");
}
}
private void finishQuiz() {
Intent resultIntent = new Intent();
resultIntent.putExtra(EXTRA_SCORE, score);
setResult(RESULT_OK, resultIntent);
finish();
}
@Override
public void onBackPressed() {
if (backPressedTime + 2000 > System.currentTimeMillis()) {
finishQuiz();
} else {
Toast.makeText(this, "Press back again to finish", Toast.LENGTH_SHORT).show();
}
backPressedTime = System.currentTimeMillis();
}
@Override
protected void onDestroy() {
super.onDestroy();
if (countDownTimer != null) {
countDownTimer.cancel();
}
}
@Override
protected void onSaveInstanceState(Bundle outState) {
super.onSaveInstanceState(outState);
outState.putInt(KEY_SCORE, score);
outState.putInt(KEY_QUESTION_COUNT, questionCounter);
outState.putLong(KEY_MILLIS_LEFT, timeLeftInMillis);
outState.putParcelableArrayList(KEY_QUESTION_LIST, (ArrayList<? extends Parcelable>) questionList);
outState.putBoolean(KEY_ANSWERED, answered);
}
}
Solution
some minor things,
when you use showNextQuestion
you should split up that method into the three sub-methods to clarify what you’re doing:
private void showNextQuestion(){
currentQuestion = getNextQuestion();
showQuestion(currentQuestion );
startCountDown();
}
that would make showNextQuestion
to a mere delegation-method and it would be very clearly what this method does.
and if you did it this way you can re-use these methods during onCreate
:
public void onCreate(Bundle savedInstanceState) {
...
if (savedInstanceState == null) {
loadQuestionsFromDb();
}else{
loadQuestionsFromSavedInstance(savedInstanceState);
}
showQuestion(currentQuestion); //as mentioned above
if (answered) {
//FIXME you don't show current score
showSolution();
} else {
startCountDown();
}
...
}
remove the counting part of checkAnswer
into a seperate methode – thus makes the checkAnswer
method a clean delegation method
private void checkAnswer() {
answered = true;
countDownTimer.cancel();
score = score + countCurrentScore();
textViewScore.setText("Score: " + score);
showSolution();
}
there are also some cosmetic things, maybe you should remove the blanks or shorten code when possible
if (timeLeftInMillis < 10000) {
textViewCountDown.setTextColor(Color.RED);
} else {
textViewCountDown.setTextColor(textColorDefaultCd);
}
into
textViewCountDown.setTextColor(timeLeftInMillis<10000?Color.RED:textColorDefaultCd);
but thats no show stopper (there are some more issues of this type – but nevermind)