Gfrktyl

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

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/%dn", 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.");

    }

}

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.

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.

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 + %dn", 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");