Math Quiz program - programming excercise number 5 chapter 6 from "Art and Science of Java"

Question

I have written the following program: Math Quiz. It is a programming exercise number 5 chapter 6 from the book "Art and Science of Java".

The program seems to be working. However, I would be grateful for any suggestions on any areas that I can improve on (any bad habits etc.).

I am also intrested if there are any more efficient or elegant ways of randomly choosing two numbers which meet the criteria listed beneath.

The program should meet these requirements:

• It should ask a series of five questions - arithmetic problems - coded as a named constant.

• Each question should consist of a single addition or subtraction problem involving just two numbers.

• The problem — addition or subtraction — should be chosen randomly for each question.

• None of the numbers involved, including the answer, should be less than 0 or greater than 20 - within these constraints, the program should choose the numbers randomly.

• The program should give the student three chances to answer each question. If the student gives the correct answer, the program should indicate that fact in some properly congratulatory way.

• If the student does not get the answer in three tries, the program should give the answer and go on to another problem.

/*
 * File: MathQuiz.java
 * ---------------------
 */
import acm.program.*;
import acm.util.*;
public class MathQuiz extends ConsoleProgram {
 private static final int NUMBER_OF_QUESTIONS = 5;
 public void run() {
 println("Welcome to MathQuiz");
// Starts a function which will ask questions 
 for (int i=0; i < NUMBER_OF_QUESTIONS; i++) {
// Draws a random number between 0 and 20 
 int x = getRandomX();
// Draws a random sign - plus or minus with a probability of 50 per cent 
 String sign = getPlusOrMinus(); 
// Initializes a second variable which will be used for second random number
 int y = 0;
// Initializes a variable where the good answer to the arithmetic problem will be kept 
 int goodanswer = 0;
// Draws a second random number between 0 and 20-x (method getRandomYPlus) if the drawn sign is + or between 0 and x (method getRandomYMinus) if the drawn sign is -
// I have created two methods because that numbers have to be such so that the answer will not be greater than 20 or less than zero 
 if (sign == "+") {
 y = getRandomYPlus(x);
 goodanswer = x + y;
 } else {
 y = getRandomYMinus(x);
 goodanswer = x-y;
 }
// Poses an arithmetic problem with the two random numbers and a random sign
 int answer = readInt("What is " + x + sign + y + "? ");
// Initializes a variable which will count the number of answers
 int count = 0;
// Students gets three chances to give a correct answer 
 while (answer != goodanswer && count !=3) {
 answer = readInt("That`s incorrect - try a different answer: ");
 count ++;
 }
 if (answer == goodanswer) {
 println("You got it!");
// After three wrong answers program gives a correct answer and moves to another problem 
 } else {
 println ("No, the answer is " + goodanswer + ".");
 }
 }
 }
// Method which draws a random number between 0 and 20 
 private int getRandomX() {
 int x = 0;
 x = rgen.nextInt(0,20);
 return x;
 }
// Method which draws a random sign - plus or minus - with a probability of 50 per cent
 private String getPlusOrMinus() {
 String sign = null;
 sign = rgen.nextBoolean() ? "+" : "-";
 return sign;
 }
// Draws a second random number between 0 and 20-x (method getRandomYPlus) if the drawn sign is + or between 0 and x (method getRandomYMinus) if the drawn sign is -
// I have created two methods because that numbers have to be such so that the answer will not be greater than 20 or less than zero 
 private int getRandomYPlus(int x) {
 int y = 0;
 y = rgen.nextInt(0,20-x); 
 return y;
 }
 private int getRandomYMinus(int x) {
 int y = 0;
 y = rgen.nextInt(0,x);
 return y;
 }
// Creates an instance variable for random number generator 
 private RandomGenerator rgen = new RandomGenerator();
}

Answer 1

The biggest problem is that you have hard-coded the maximum number size (20) at multiple places. Just like the number of rounds this should be a constant.

---

You should void importing using a wild card. Always import only the specific classes you want to use. Otherwise you'll be having a difficult time figuring out name collisions when two packages have declared classes with the same name.

---

Move the initialization of the RandomGenerator to the beginning of the class, so that people don't need to guess what the variable rgen is. You also can make it final and static since it won't change and you (probably) can use the same instance for all instances of your class.

---

You should avoid initializing variable to values that you won't be using:

private int getRandomX() {
 int x;
 x = rgen.nextInt(0,20);
 return x;
}

or

private int getRandomX() {
 int x = rgen.nextInt(0,20);
 return x;
}

or in a function even:

private int getRandomX() {
 return rgen.nextInt(0,20);
}

That way people don't need to figure out what the zero could mean. It also helps debugging, because the code won't compile if you have forgotten to do your actual initialization.

For example, this code won't compile, because goodanswer isn't initialized in the else block.

 int y;
 int goodanswer;
 if (sign == "+") {
 y = getRandomYPlus(x);
 goodanswer = x + y;
 } else {
 y = getRandomYMinus(x);
 }

If you however had left int goodanswer = 0;, then the code will compile and run, but won't work correctly and you'd have to put time in finding the bug.

Answer 2

• Way too much whitespace. You don't need a blank line between each line of code.

• Way too many comments. Comments should explain why decisions were made, not what the code is doing. The code already says what it does. If it's too complex to read easily, you need to fix the code, not add a comment.

• Don't ever compare strings with ==. Always use equals().

• Instance variable declarations belong at the top of the class, not the bottom or interspersed in your methods.

• Use whitespace consistently. For instance, !=3 should be != 3. int i=0 should be int i = 0.

• this.rgen.nextInt(min, max) can be left inline. You don't need to declare three separate wrapper methods for it.

• Use final aggressively to reduce the cognitive load of readers.

• Multi-word variable names should use camelCase, not lowercase. (goodAnswer, not goodanswer).

• Although y and goodAnswer both rely on the sign, I would argue that they're separate computations. For clarity, they might be better served in separate methods.

• 20 is a magic number, and should be a constant.

• count should be more descriptive - maybe attempts?

• It's convention to compare < instead of !=, just in case something assigns count to be bigger than 3. If it did, your loop would go on forever.

• Bug - you're giving four attempts, and the requirements say you should be giving three. You should start count at 1, since you ask the first time right before you assign count.

If I were to take a stab at refactoring your code to include the ideas above, it might look something like:

public final class MathQuiz extends ConsoleProgram {
 private static final int NUMBER_OF_QUESTIONS = 5;
 private static final int BIGGEST_POSSIBLE_NUMBER = 20;
 private final RandomGenerator rgen = new RandomGenerator();
 public void run() {
 this.println("Welcome to MathQuiz");
 for (int i = 0; i < NUMBER_OF_QUESTIONS; i++) {
 final int x = this.rgen.nextInt(0, BIGGEST_POSSIBLE_NUMBER);
 final String sign = this.rgen.nextBoolean() ? "+" : "-";
 final int y = this.computeY(x, sign);
 final int goodAnswer = this.computeAnswer(x, sign, y);
 int answer = this.readInt("What is " + x + sign + y + "? ");
 int attempts = 1;
 while ((answer != goodAnswer) && (attempts < 3)) {
 answer = this.readInt("That`s incorrect - try a different answer: ");
 attempts++;
 }
 if (answer == goodAnswer) {
 this.println("You got it!");
 } else {
 this.println ("No, the answer is " + goodAnswer + ".");
 }
 }
 }
 /**
 * @return a value {@code y} such that {@code y} and the correct answer to the mathematical
 * operation are both less than {@link BIGGEST_POSSIBLE_NUMBER}.
 */
 private int computeY(final int x, final String sign) {
 return sign.equals("+") ? this.rgen.nextInt(0, BIGGEST_POSSIBLE_NUMBER - x) : this.rgen.nextInt(0, x);
 }
 private int computeAnswer(final int x, final String sign, final int y) {
 return sign.equals("+") ? x + y : x - y;
 }
}

• Ankit Soni: IMO, goodAnswer should be renamed to correctAnswer too.

Answer 3

Great answers already. I add one point:

sign == "+"

Is a (potiential) bug. See String equals in Java

Better would be to store the sign in a boolean (isPlus), or better, use an explicit enum.

For example:

enum Sign
{
 PLUS, MINUS
}

Then you have an explicit type that is your sign, and you can use

if (sign == Sign.PLUS)

Answer 4

I have thoroughly analyzed your suggestions and I see reasons for most of them. However, I would be greatful for additional clarification in a couple of cases:

1. Use final aggressively to reduce the cognitive load of readers.

   Do you mean by that that I should use "final" agresively because if a reader sees "final" he does not have to worry that the variable will change? In other words: one thing less to keep track of in a program?

   And that, I presume, makes such a program easier to follow?

2. You have added a lot of "this" to the program. What is the reason for that?

3. sign == "+" Is a (potiential) bug. Better would be to store the sign in a boolean (isPlus), or better, use an explicit enum.

   Can`t I just change "==" to "equals"?

4. The refactored version of my code has got fewer methods that mine. For instance you have not created a method for int x but you have created one for int y.

After watching online lectures on programming methodology by prof. Mehren Sahami I was under the impression that the more methods the better. That by creating methods you hide unnecessary complexity from the reader.

Why than have you decided not to create a method for int x?

And what criteria do you use when you decide to create or not to create a method.