10
\$\begingroup\$

I made a Rock, Paper, Scissors game and would like the code to be reviewed.

Main class:

public class Main {



    public static void main(String args[]){
        Game game = new Game();
        game.gameLoop();

    }

}

Game class:

import java.util.Scanner;

public class Game {

String yesOrNo;

Input inputClass = new Input();
Computer comp = new Computer();
Checking checker = new Checking();
Score score = new Score();
Scanner input = new Scanner(System.in);

    public void gameLoop() {
        while (true) {
            inputClass.setUserInput();
            comp.setComputerInput();
            comp.displayComputerInout();
            if (checker.check(inputClass.getUserInput(),
                    comp.getComputerInput()) == 0) {
                System.out.println("Tie!");
                score.scoreBoard(0);
            } else if (checker.check(inputClass.getUserInput(),
                    comp.getComputerInput()) == 1) {
                System.out.println("Win!");
                score.scoreBoard(1);
            } else if (checker.check(inputClass.getUserInput(),
                    comp.getComputerInput()) == 2) {
                System.out.println("Lose");
                score.scoreBoard(2);
            } else if (checker.check(inputClass.getUserInput(),
                    comp.getComputerInput()) == -1) {
                break;
            }

            System.out.println("Go to score menu? y/n");

            yesOrNo = input.nextLine();
            if(yesOrNo.equals("y")){
                score.ScoreMenu();
            }else if(yesOrNo.equals("n")){

            }else{
                System.out.println("Invalid command! Continuing without going to score menu!");
            }

            System.out.println("Want to play again? y/n");
            yesOrNo = input.next();
            if(yesOrNo.equals("y")){
            }else if(yesOrNo.equals("n")){
                break;
            }else{
                System.out.println("Invalid command! Continuing!");
            }
        }
    }
}

Input Class:

import java.util.Scanner;

public class Input {

    private Scanner input = new Scanner(System.in);

    private int uInput;

    public void setUserInput(){
        System.out.println("Rock: 1, Paper: 2, Scissor: 3");
        uInput = input.nextInt();
        if(uInput<1 || uInput>3){
            System.out.println("Invalid value!\nYou get Rock");
            uInput=1;
        }
    }

    public int getUserInput(){
        return uInput;
    }

}

Computer Class:

 public class Computer {

    private int computerGesture;

    public void setComputerInput(){
        computerGesture = (int) (Math.random()*4);
        if(computerGesture == 0){
            computerGesture = 1;
        }

    }

    public int getComputerInput(){
        return computerGesture;
    }

    public void displayComputerInout(){
        if(computerGesture == 1)
        {
            System.out.println("Computer chooses rock");
        }else if(computerGesture == 2){
            System.out.println("Computer chooses paper");
        }else if(computerGesture == 3){
            System.out.println("Computer chooses scissor");
        }
    }

}

Checking Class:

public class Checking {

    private final int TIE = 0;
    private final int WIN = 1;
    private final int LOSE = 2;
    private final int ERROR = -1;

    public int check(int pGesture, int cGesture) {

        if (pGesture == cGesture) {
            return TIE;
        } else if (pGesture == 1 && cGesture == 2) {
            return LOSE;
        } else if (pGesture == 2 && cGesture == 1) {
            return WIN;
        } else if (pGesture == 1 && cGesture == 3) {
            return WIN;
        } else if (pGesture == 3 && cGesture == 1) {
            return LOSE;
        } else if (pGesture == 2 && cGesture == 3) {
            return LOSE;
        } else if (pGesture == 3 && cGesture == 2) {
            return WIN;
        } else {
            return ERROR;
        }
    }

}

Score Class:

import java.util.Scanner;

public class Score {

    Scanner input = new Scanner(System.in);

    private int wins = 0;
    private int loses = 0;
    private int ties = 0;

    private String choise;

    public void scoreBoard(int winOrLoseOrTie){
        if(winOrLoseOrTie == 1){
            wins++;
        }else if(winOrLoseOrTie == 2){
            loses++;
        }else if(winOrLoseOrTie == 0){
            ties++;
        }
    }

    public void displayScore(){
        System.out.println("Wins Loses Ties\n"+wins+"    "+loses+"    "+ties);
    }

    public void resetScoreBoard(){
        wins = 0;
        loses = 0;
        ties = 0;
    }

    public void ScoreMenu(){
        System.out.println("Display Score: display");
        System.out.println("Reset Score: reset");
        System.out.println("Exit score menu: exit");

        choise = input.next();
        if(choise.equals("display")){
            displayScore();
        }else if(choise.equals("reset")){
            resetScoreBoard();
        }else if(choise.equals("exit")){

        }else{
            System.out.println("Invalid command!");
            ScoreMenu();
        }
    }
}
\$\endgroup\$
1
  • 2
    \$\begingroup\$ One little comment, the class Random comes with the method nextInt() so you could do Random.nextInt(4) instead of multiplying a floating point. Please someone correct me if I'm wrong, but I believe the numbers should be distributed more randomly than by multiplying a random floating point number. \$\endgroup\$
    – ljacqu
    Commented Jul 17, 2014 at 14:51

3 Answers 3

11
\$\begingroup\$

I will not cover the style convention, but do know that there is work to do !

Empty if

You should never use empty if. This would to someone thinking that the if is not necessary, remove it and bang your program doesn't work anymore.

    if(choise.equals("display")){
        displayScore();
    }else if(choise.equals("reset")){
        resetScoreBoard();
    }else if(choise.equals("exit")){

    }else{
        System.out.println("Invalid command!");
        ScoreMenu();
    }

In this case you could add a return, since that is basically what you want to do.

Enums

When you have a set of predefined values with a meaning, normally you can make a Enum to give them the special meaning they deserve and enhance the readability of the code.

private final int TIE = 0;
private final int WIN = 1;
private final int LOSE = 2;
private final int ERROR = -1;

This could be

public enum GameEnding {
    TIE , WIN , LOSE , ERROR
}

The name is really bad, but with an enum your method to check will be far more readable :

    if(winOrLoseOrTie == GameEnding.WIN){
        wins++;
    }else if(winOrLoseOrTie == GameEnding.LOSE){
        loses++;
    }else if(winOrLoseOrTie == GameEnding.TIE){
        ties++;
    } 

Now instead of having magic numbers, you have a readable way to express the outcome of a game.

OO structure

I didn't read for a long time your code, but I can say that you're on the good way. You've encapsulated your Game, Scoring and concept like that. You still use some magic numbers, which is an indication that there are some concept of your game that you have not abstracted in code. The GameEnding enum is one of those concept. It has a specific meaning in your application and will be used everywhere in your code, that should have been there, but this will come with more experience.

This is a bit advance for you (I guess), but your output should be independent of your logic code. This will help to separate your concepts , because when you'll move from the console to a GUI, all your classes will be impacted.

There is still an Enum missing in your code, I will leave it as an exercise to you, but some tips look at magic numbers and think of what is the base of Rock, Paper, Scissors!

\$\endgroup\$
7
  • \$\begingroup\$ If someone has a better name to propose please do so! I have no inspiration! \$\endgroup\$
    – Marc-Andre
    Commented Jul 17, 2014 at 14:07
  • \$\begingroup\$ Thank you for you answer! I will try to fix all of the things you mentioned. Do you think i have got the OO thinking or am i doing it wrong? \$\endgroup\$
    – Svante
    Commented Jul 17, 2014 at 14:09
  • \$\begingroup\$ @Svante I've edited what I think of your OO structure! \$\endgroup\$
    – Marc-Andre
    Commented Jul 17, 2014 at 14:17
  • \$\begingroup\$ @Svante To add my two cents, your OO structure had me smiling a little bit because of the "sticking to what I learned" feel it had! But it's great! ^^ \$\endgroup\$
    – ljacqu
    Commented Jul 17, 2014 at 14:50
  • \$\begingroup\$ @Svante Ping me on this answer when you will find the missing enums in your code ;). \$\endgroup\$
    – Marc-Andre
    Commented Jul 17, 2014 at 14:57
7
\$\begingroup\$

Since Marc-Andre has covered empty ifs, enums, and OO structure very thoroughly, I'll add a bit about style.

public class Game {

String yesOrNo;

Input inputClass = new Input();
Computer comp = new Computer();
Checking checker = new Checking();
Score score = new Score();
Scanner input = new Scanner(System.in);

    public void gameLoop() {

Since the rest of your indents seem good, I'm going to assume this is just down to Stack Exchange's kinda weird Markdown. However, since it seems like none of these are used outside of gameLoop, they should all be variables of gameLoop, not of Game. It doesn't matter too much in this case because Game only has one method, but in general we like to only give variables the scope they need (if you haven't come across variable scope, you can just ignore this until you do). Some of these names are strong (input is pretty standard, and score makes a lot of sense). Others could do with some work. yesOrNo, in particular, is used in two semantically different contexts: it is used both as "does the user want to see the score menu?" and as "does the user want to play again?". Separate these out into two variables, and suddenly their names can be specific: openScoreMenu and playAgain.

On spacing, I suggest adding more, or at the very least being consistent. People have different styles, and it doesn't make sense to say that others are invalid, but it is important to find a style that works well and stick to it. If you don't use spaces between the if and the (, then never use spaces between the if and the (. (Exception: when editing someone else's code, try to keep their style as much as possible.)

For example, here use use lots of whitespace:

if (pGesture == cGesture) {
            return TIE;
        } else if (pGesture == 1 && cGesture == 2) {
            return LOSE;

And here you use very little:

if(yesOrNo.equals("y")){
    score.ScoreMenu();
}else if(yesOrNo.equals("n")){

Personally, I like to use lots of whitespace in my Java, but I know some people like to use very little.

Finally, in Java it's very important to pay attention to capitalization. Sure, it'd be nice if the ScoreMenu method was OpenScoreMenu instead (general rule: make method names verbs, and object names nouns), but it's much better if it's scoreMenu. Methods and variables are always camelCase, class names are always NormalCaps, and constants are always ALLCAPS.

(also, one irrelevant point because I'm super pedantic: choice is spelled with two c's and no s's)

\$\endgroup\$
2
  • \$\begingroup\$ About the whitespaces: Ctrl+Shift+F all the way! It standardizes the code which is always nice for version control such as Git or SVN \$\endgroup\$ Commented Jul 17, 2014 at 21:12
  • \$\begingroup\$ True, if you're using a nice IDE like Eclipse that will standardize the white spacing, and it's configurable. \$\endgroup\$ Commented Jul 17, 2014 at 21:28
4
\$\begingroup\$

Couple of minor points:

  1. Expanding on Marc-Andre enums suggestion, consider an array of results, so instead of

    if(winOrLoseOrTie == GameEnding.WIN){
        wins++;
    }else if(winOrLoseOrTie == GameEnding.LOSE){
        loses++;
    }else if(winOrLoseOrTie == GameEnding.TIE){
        ties++;
    }
    

    you'd have a single line:

    results[winOrLoseOrTie]++;
    
  2. Checking.check could be significantly simplified. Notice how

    (pGesture - cGesture) % 3
    

    immediately determines an outcome. 1, 0, -1 mean win, tie, lose respectively.

\$\endgroup\$
0

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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