Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

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

public void answerGetter_score(){
    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
}//answer Getter method
}//machine class


public class calcu {
public static void main(String[] args) {
Machine machine1 = new Machine();
System.out.println("***Welcome to addition Math test***");

for(int i=5; i>0; i--){
machine1.sumGenerator();
machine1.answerGetter_score();
}
System.out.println("Thanks for taking the test.");
}//main method
share|improve this question
add comment

4 Answers 4

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.

share|improve this answer
1  
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. –  user3117051 Feb 21 at 19:44
    
I've rolled back the edit you suggested. Whitespace is a matter to be reviewed, not silently changed. –  200_success Feb 21 at 20:33
    
Agreed. Thanks. –  Uri Agassi Feb 21 at 20:49
add comment

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.

Code Formatting (only because you asked about good habits)

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();
    int getAnswer();
}

SumGenerator.java:

import java.util.Random;

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

    public SumGenerator(int maxAddend) {
        this.maxAddend = maxAddend;
        this.next();
    }

    @Override
    public void next() {
        num1 = rand.nextInt(this.maxAddend + 1);
        num2 = rand.nextInt(this.maxAddend + 1);
        ans = num1 + num2;
    }

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

    @Override
    public int getAnswer() {
        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());
                int answer = in.nextInt();
                if (this.questions.getAnswer() == answer) {
                    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) {
        System.out.println("***Welcome to addition Math test***");
        ArithmeticQuiz quiz = new ArithmeticQuiz(5, new SumGenerator(9));
        quiz.run();
        System.out.println("Thanks for taking the test.");
    }
}
share|improve this answer
    
+1. Note that the two spaces was an edit to the question by not the OP. –  palacsint Feb 21 at 20:27
    
@palacsint Thanks for pointing that out. I've rolled back the invalid edit. –  200_success Feb 21 at 20:30
    
Thank you very much. –  user3117051 Feb 22 at 3:59
    
You're welcome. Please remember to upvote all answers that you find useful. –  200_success Feb 22 at 4:10
add comment

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.

share|improve this answer
    
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. –  user3117051 Feb 21 at 19:48
    
@user3117051: codereview.stackexchange.com/q/31/7076 –  palacsint Feb 21 at 20:20
1  
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) –  palacsint Feb 21 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. –  David Stanley Feb 22 at 20:29
add comment

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");
    
share|improve this answer
    
I appreciate the help and advice. –  user3117051 Feb 22 at 4:00
add comment

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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