Multithreaded arithmetic quiz with 20-second time limit

Question

I learned something about threads and I tried to use them in an programming exercise. The user of the program has to solve as many exercises as he can within twenty seconds. After that time the program stops and and prints some statistical values.

Is my implementation a good implementation? Are there any blemishes or rookie mistakes that can be eliminated? I would be very thankful for a code review!

An executable can be downloaded here

Package game

Game.java

package game;

import game.TimeCounter;
import game.exercise.Examiner;

public class Game {
    public static void main(String[] args) {
        System.out.println("You have to solve as many exercises as you can " +
        "within 20 seconds");
        Examiner examiner = new Examiner();
        examiner.start();
        TimeCounter timeCounter = new TimeCounter(examiner, 20);
        timeCounter.countTimeAndInterrupt();
        examiner.printStats();
    }
}

TimeCounter.java

package game;

import game.exercise.Examiner;

public class TimeCounter {
    private Examiner examiner;
    private int seconds;

    public TimeCounter(Examiner examiner, int seconds) {
        this.examiner = examiner;
        this.seconds = seconds;
    }



    public void countTimeAndInterrupt() {
        for(int i = 0; i < seconds; i++) {
            try {
                Thread.sleep(1000);
            } catch (InterruptedException e) {
                e.printStackTrace();
            }
        }
        examiner.stop();
    }
}

Package game.exercise

Examiner.java

package game.exercise;

import java.util.Scanner;

public class Examiner extends Thread {
    private int right; // number of exercises solved right
    private int wrong;

    @Override
    public void run() {
        Scanner scanner = new Scanner(System.in);

        while (true) {
            int input;
            Exercise exercise;

            exercise = Exercise.getRandomExercise();
            System.out.print(exercise);
            input = scanner.nextInt();
            if (exercise.check(input)) {
                right++;
                System.out.println("Right!");

            } else {
                wrong++;
                System.out.println("Wrong!");
            }
        }
    }

    public void printStats() {
        System.out.println();
        System.out.println("Number of all exercises: " + 
        getNumberOfExercises());
        System.out.println("Exercises solved right: " + right);
        System.out.println("Exercises solved wrong: " + wrong);
        int percentage = (int)((float)(right) / getNumberOfExercises() * 100);
        System.out.println("Rate: " + percentage + " % ");
    }

    public int getNumberOfExercises() {
        return right + wrong;
    }
}

Exercise.java

package game.exercise;

import java.util.Random;

public class Exercise {
    private static Random random = new Random();
    private int number1;
    private int number2;
    private Type type;

    public Exercise(Type type, int number1, int number2) {
        this.type = type;
        this.number1 = number1;
        this.number2 = number2;
    }

    public static Exercise getRandomExercise() {
        return getRandomExercise(Type.random());
    }

    public static Exercise getRandomExercise(Type type) {
        switch(type) {
            case ADDITION: 
            case SUBTRACTION: 
                return new Exercise(type, random.nextInt(40) + 1, 
                random.nextInt(40) + 1);
            case MULTIPLICATION: 
                return new Exercise(type, random.nextInt(20) + 1, 
                random.nextInt(20) + 1);
            case DIVISION: 
                return new Exercise(type, random.nextInt(100) + 1, 
                random.nextInt(15) + 1);
            default: throw new IllegalArgumentException("not a valid type");
        }
    }

    @Override
    public String toString() {
        return number1 + " " + getOperator() + " " + number2 + " = ";
    }

    private String getOperator() {
        switch(type) {
            case ADDITION: return "+";
            case SUBTRACTION: return "-";
            case MULTIPLICATION: return "*";
            case DIVISION: return "/";
            default: throw new IllegalArgumentException("not a valid type");
        }
    }

    private int getSolution() {
        switch(type) {
            case ADDITION: return number1 + number2;
            case SUBTRACTION: return number1 - number2;
            case MULTIPLICATION: return number1 * number2;
            case DIVISION: return (int)(number1 / number2);
            default: throw new IllegalArgumentException("not a valid type");
        }
    }

    public boolean check(int guess) {
        return getSolution() == guess;
    } 
}

Type.java

package game.exercise;

import java.util.Random;

public enum Type {
    ADDITION, SUBTRACTION, MULTIPLICATION, DIVISION;

    public static Type random() {
        Random random = new Random();
        int choice = random.nextInt(4);
        if(choice == 0) return ADDITION;
        else if(choice == 1) return SUBTRACTION;
        else if(choice == 2) return MULTIPLICATION;
        else if(choice == 3) return DIVISION;
        else throw new IllegalArgumentException();
    }
}

Answer 1

Type.java

• Your way of picking a random value from the enum contains code duplication, namely the hard-coding of each of the four possible cases. There is a more elegant way to achieve this, you might take a look at this question on stackoverflow. Note also that, in this question, as well as in the accepted answer, the number of values in the enum is not hard-coded into the call to the method Random.nextInt(int), but obtained via .values().length (as opposed to your choice = random.nextInt(4)). By doing this, the call to Random.nextInt(int) will still pass the correct upper boundary even if the number of values of the enum changes, which reduces the potential for bugs.

• Regardless of the above, your usage of an IllegalArgumentException does not fit at all in this context. The purpose of an IllegalArgumentException is "to indicate that a method has been passed an illegal or inappropriate argument", which is not the case here. What you have is a statement that is practically unreachable, but the compiler does not know that it is unreachable, only you know. Hence, if this statement should ever be reached against your expectations, then it is not because an illegal argument has been passed to the method, but because your assumptions about your code were incorrect, and this is exactly what assertions are for:

  if(choice == 0) return ADDITION;
  else if(choice == 1) return SUBTRACTION;
  else if(choice == 2) return MULTIPLICATION;
  else {
      assert choice == 3;
      return DIVISION;
  }
  

Exercise.java

• The fields number1, number2 and type can be final. Actually, random can also be final.

• The method getRandomExercise(Type) contains code duplication. The creation of a new Exercise instance is common for all four types, the only thing specific to the type is the upper boundary of the two numbers. So instead, you could do this:

  public static Exercise getRandomExercise(Type type) {
      int upperBoundary1;
      int upperBoundary2;
  
      switch (type) {
          case ADDITION:
          case SUBTRACTION:
              upperBoundary1 = 40;
              upperBoundary2 = 40;
              break;
          case MULTIPLICATION:
              upperBoundary1 = 20;
              upperBoundary2 = 20;
              break;
          default:
              assert type == Type.DIVISION;
              upperBoundary1 = 100;
              upperBoundary2 = 15;
              break;
      }
  
      return new Exercise(
              type,
              random.nextInt(upperBoundary1) + 1,
              random.nextInt(upperBoundary2) + 1);
  }
  

  I also removed your IllegalArgumentException and replaced it with an assertion. There is, by the way, a possible scenario where type is neither of the four values, and that is if it is null. To prevent this from happening, you would have to check in the Exercise(Type, int, int) constructor whether the passed Type is null, and if it is, then an IllegalArgumentException would really be appropriate (although a NullPointerException might even be more fitting).

• case DIVISION: return (int)(number1 / number2);
  

  Here, the cast to int is unnecessary, because dividing two integers will always produce an integer, regardless of whether the actual result of the division would be an integer or not. For instance, 7 / 2 will yield the int 3, and not a double with the value of 3.5 (7.0, on the other hand, would be treated as a double, so 7.0 / 2.0 would yield 3.5).

TimeCounter.java

• I don't understand why you create a loop that is executed once for every second, instead of simply letting the thread sleep for the whole period of time once, without a loop.

• You are calling Thread.stop(), which is deprecated. An alternative would be to interrupt the examiner thread from the time counter thread instead of stopping it, which would mean that the examiner thread has to check whether it is interrupted, for example like this:

  public void run() {
      Scanner scanner = new Scanner(System.in);
  
      while (!Thread.interrupted()) { // clears the interrupted flag if it is set
          Exercise exercise = Exercise.getRandomExercise();
          System.out.print(exercise);
          int input = scanner.nextInt();
  
          if (!Thread.currentThread().isInterrupted()) { //does not clear the interrupted flag
              if (exercise.check(input)) {
                  right++;
                  System.out.println("Right!");
              } else {
                  wrong++;
                  System.out.println("Wrong!");
              }
          } else {
              System.out.println("Too late!");
          }
      }
  }
  

  I understand that, this way, the examiner thread only terminates after the user inputs something, but it doesn't seem to be very simple to stop a thread that is waiting for user input (at least judging by this question on stackoverflow).