# Maths test program

I have written this simple program that is a basically a maths test. I would like to know is there any areas I can improve on and if I have any bad habits that would be useful to break now. I realise I could have written this in the main class, but I was trying to practice implementing other classes, so I tried to use a few classes.

Also, are there good practices that I am using that I should try to keep?

P.S. I don't usually use the comments I just added them to tell you what my intention is for a particular piece of code.

import java.util.Scanner;
import java.util.Random;

class Machine {
int num1, num2, ans, att;
int score = 0;
Random rand = new Random();
Scanner in = new Scanner(System.in);

public void sumGenerator(){
num1 = rand.nextInt(10);
num2 = rand.nextInt(10);
ans = num1 + num2;

System.out.println(num1 +  " + " + num2 );
}//sumGenerator Method

att = in.nextInt();
if(att == ans){
score = score + 1;
System.out.println("Correct");
System.out.println("Score is currently: " + score + "/5");
}else{
score = score - 1;
System.out.println("Incorrect");
System.out.println("Score is currently: " + score + "/5");
}//else
}//machine class

public class calcu {
public static void main(String[] args) {
Machine machine1 = new Machine();

for(int i=5; i>0; i--){
machine1.sumGenerator();
}
System.out.println("Thanks for taking the test.");
}//main method


### Naming and Problem Decomposition

• Class names should be nouns; method names should be verbs. If you have a method named as a noun like sumGenerator(), that's a red flag.
• Each method should do one thing only; its name should reflect its purpose. If you have a method named answerGetter_score(), that's a red flag. Is it getting an answer and keeping score? It also breaks the standard interCapsNaming() convention.
• Each class should do one thing only; its name should reflect its purpose. I have no idea what a Machine or a calcu is. The latter also breaks the standard capitalization convention.
• Separate your input/output routines from the calculation routines. Forcing yourself to separate the two helps you come up with clean designs.
• Keep your main() function minimal. The temptation to stuff a lot of functionality into main() leads to sloppy design.
• Avoid hard-coding assumptions in more than one place. What if you want to change the length of the quiz? Currently, 5 is hard-coded in several places. It's easy to introduce a bug if someone fails to make the same change everywhere.

For this problem, I think that there should be two classes: SumGenerator and ArithmeticQuiz. SumGenerator is responsible for making random addition questions. ArithmeticQuiz "drives" it.

What if you want to change the quiz to do multiplication? Just swap out the SumGenerator for a ProductGenerator. To ease that transition, it would be helpful to define a QuestionGenerator interface.

Consistent indentation is very important for readability. Do that, and discard the noisy //end comments.

Also, add some horizontal spacing around punctuation, such as comparison operators.

The one-point penalty for an incorrect answer deserves a comment, since not everyone expects that behaviour.

### Proposed Solution

QuestionGenerator.java:

interface QuestionGenerator {
void next();
String getQuestion();
}


SumGenerator.java:

import java.util.Random;

class SumGenerator implements QuestionGenerator {
private int maxAddend, num1, num2, ans;
private Random rand = new Random();

this.next();
}

@Override
public void next() {
ans = num1 + num2;
}

@Override
public String getQuestion() {
return num1 + " + " + num2;
}

@Override
return ans;
}
}


ArithmeticQuiz.java:

import java.util.Scanner;

public class ArithmeticQuiz {
private int length;
private QuestionGenerator questions;

public ArithmeticQuiz(int length, QuestionGenerator q) {
this.length = length;
this.questions = new SumGenerator();
}

public void run() {
// Closing the Scanner after use is a good habit.
// Automatically closing the Scanner using the
// try-with-resources feature of Java 7 is even better.
try (Scanner in = new Scanner(System.in)) {
int score = 0;
for (int i = this.length; i > 0; i--) {
System.out.println(this.questions.getQuestion());
score++;
System.out.println("Correct");
}else{
score--;    // Penalty for incorrect answer
System.out.println("Incorrect");
}
System.out.printf("Score is currently: %d/%d\n", score, this.length);
this.questions.next();
}
}
}

public static void main(String[] args) {
ArithmeticQuiz quiz = new ArithmeticQuiz(5, new SumGenerator(9));
quiz.run();
System.out.println("Thanks for taking the test.");
}
}


Member Names - don't use too short names, answer is better than ans; what exactly is att?

Method Names - method names should be verbs describing what the method does - generateNewSum() is better than sumGenerator(). Also use Java method conventions - CamelCase (no underscore in method names - answerGetterScore(), see below for more about this method)

Class Names - follow Java Class naming conventions - they should start with a capital letter, and don't be lazy - write the whole word - Calculator. The name of the class should denote its part in the program - Machine tells you nothing about the class, perhaps something more in the lines of SumExercise?

Responsibilities - object oriented programming is all about division of responsibility - one object is responsible for interacting with the user, another represents an item in a list, etc.

This means that a single class should be responsible for interacting with the user - getting input, printing out results. A single class should be responsible for managing the exercise - how many iterations there are, how you calculate score...

In your code, for example, the number of iterations (5) appears both in the main class and the Machine class - tomorrow you'll want to make it 10 iterations - you are bound to forget to change it inside the Machine class.

Same goes for method names - remember answerGetterScore()? The name that came out is awkward, because it tries to convey that the method does at least two things which are apparently unrelated - see if the answer is correct, and calculate the score (actually it does three - it also requests the answer from the user). You should split the method to its parts - (1) get the answer from the user; (2) check if it is correct; (3) calculate the new score; (4) notify the user. I'll leave it to you to decide which of those methods should go to which class.

Comments - you said you added comments for our behalf, to show your intention, and sometimes comments are really needed (not too often though!), but these comments do not convey any information, they simply say where a block ends (//else). Indentation should do that.

• Thank you so much. This is exactly what I was looking for. I will look to improve on these areas as the videos I am learning from don't really specify these conventions. Thanks again. Feb 21 '14 at 19:44
• I've rolled back the edit you suggested. Whitespace is a matter to be reviewed, not silently changed. Feb 21 '14 at 20:33

Scope - Objects and variables should be defined with the most restrictive scope possible, considering their use. Properties and methods that are only going to be used in the class they're defined in should be marked private. For instance, all of the properties you define for the class Machine should be private. Properties that are only used in a single method should be declared in that method. For instance, num1, num2 and att can all have their declarations moved into the methods that use them.

And a style point about scope. Although some people don't like it, consider using the this keyword for all object properties. For instance, you would write this.ans instead of just ans. Whoever reads your code will immediately see that ans is not a local variable. Although most IDEs will color your object properties differently from local variables, some places (like CodeReview) won't.

• Thank you I will take this advice on board. I only hope I don't develop more bad habits. Kind of makes me think about waiting until college to learn but I enjoy it so much. Feb 21 '14 at 19:48
• @user3117051: codereview.stackexchange.com/q/31/7076 Feb 21 '14 at 20:20
• I agree with the first paragraph and disagree with the second. Use a modern IDE, Eclipse does it fairly well: "Since modern IDEs colour code member fields, the unnecessary use of this is background noise to the intent you're trying to convey in the code." (Source: stackoverflow.com/a/725852/843804) Feb 21 '14 at 20:23
• Modern IDEs do it, but your code isn't always viewed in an IDE. The code user3117051 posted doesn't have such highlighting because Code Review doesn't understand code deeply enough to do that. It helps to add a this if you view your code anywhere else, and it doesn't feel like excessive noise, at least to me. It's a matter of personal preference, or better, whatever your organizations style guide says. Feb 22 '14 at 20:29

Some notes which was not mentioned earlier:

1. Machine machine1 = new Machine();


The variable name could be simply machine. I don't see any reason for the 1 postfix.

2. I think the usual style for a for loop is the following and the most developer is more familiar with it than a countdown loop:

for (int i = 0; i < 5; i++) { ... }


Using a well-known pattern makes maintenance easier.

3. I've found good practice to have a separate class for the main method as you did.

4. I'd put the variable declarations to separate lines. From Code Complete, 2nd Edition, p759:

With statements on their own lines, the code reads from top to bottom, instead of top to bottom and left to right. When you’re looking for a specific line of code, your eye should be able to follow the left margin of the code. It shouldn’t have to dip into each and every line just because a single line might contain two statements.

5. Having one Random instance and using it multiple times is good. (Initializing it every time could be slow and unnecessary.)

6. Using System.out.format instead of System.out.println with long string concatenations is easier to read:

System.out.format("%d + %d\n", num1, num2);

7. I guess using ++ and -- is a little bit easier to read than

score = score + 1;

8. I agree with @Uri Agassi, you should separate the responsibilities. Aside from that you could move the last System.out after the if-else condition:

if (att == ans) {
score = score + 1;
System.out.println("Correct");
System.out.println("Score is currently: " + score + "/5");
} else {
score = score - 1;
System.out.println("Incorrect");
System.out.println("Score is currently: " + score + "/5");
}


It would remove some duplication:

if (att == ans) {
score++;
System.out.println("Correct");
} else {
score--;
System.out.println("Incorrect");
}
System.out.println("Score is currently: " + score + "/5");