1
\$\begingroup\$

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);
}
}
\$\endgroup\$
1
\$\begingroup\$

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 showNextQuestionto 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 checkAnswermethod 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)

\$\endgroup\$
  • \$\begingroup\$ I'd like to disagree on "you should remove the blanks or shorten code when possible". Readablity/understandability goes before shortness. The conditional expression would be much better readable with spaces and on a separate line: Color color = timeLeftInMillis < 10000 ? Color.RED : textColorDefaultCd; \$\endgroup\$ – RoToRa Mar 2 '18 at 7:59
  • \$\begingroup\$ uhm - i tried to point out that these changes are cosmetic proposals and depend on the OPs own view of things - i said maybe to emphase this personal view of mine and even added the no show stopper-part ^_^ @RoToRa - in my opinion i do not see any need to first get the color and then second set the text color with it - i for myself would simply set the text color directly... but what do i know ^^ \$\endgroup\$ – Martin Frank Mar 2 '18 at 8:02

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.