I'm a complete beginner to Java and we have just started learning objects and classes in school. I'm trying to create a very simple text-based Rock Paper Scissors game. I do realise that my code is a mess, so I'm asking for some suggestions on how could I improve it and if I'm even in the right direction with OOP.

Main class:

public class Main {

    public static void main(String args) {

        new Game();

    }

}

Player class

public abstract class Player {

    private String name;

    private String choice;



    public Player() {}

    public Player(String name) {this.name = name;}



    public void setName(String newName) {

        name = newName;

    }



    public String getName() {

        return name;

    }



    public String getChoice() {

        return choice;

    }



    public void setChoice(String newChoice) {

        choice = newChoice;

    }

    public abstract void selectChoice();

}

User class

import java.util.Scanner;



public class User extends Player {



    private Scanner input;



    public User() {

        input = new Scanner(System.in);

    }



    public void selectChoice() {

        System.out.println("Enter your choice: R - Rock, P - Paper, S - Scissors");

        setChoice(input.nextLine().toUpperCase());

    }

}

Computer class

import java.util.Random;



public class Computer extends Player {



    private Random rand;

    private final int MAX_NUMBER = 3;



    public Computer() {

        setName("Computer");

        rand = new Random();

    }



    public void selectChoice() {

        int randomNumber = rand.nextInt(MAX_NUMBER);

        switch(randomNumber) {

            case 0:

                setChoice("ROCK");

                break;

            case 1:

                setChoice("PAPER");

                break;

            case 2:

                setChoice("SCISSORS");

                break;

        }

    }

}

Game class

import java.util.Scanner;



public class Game {

    private User p;

    private Computer com;

    private int playerWins;

    private int playerLoses;

    private int ties;

    private boolean isRunning = false;

    private Scanner scan;



    public Game() {

        p = new User();

        com = new Computer();

        scan = new Scanner(System.in);

        start();

    }



    private void start() {

        isRunning = true;

        System.out.println("Please, enter your name:");

        p.setName(scan.nextLine());



        while(isRunning) {

            displayScore();

            p.selectChoice();

            com.selectChoice();

            displayChoices();

            displayWinner(decideWinner());

            updateScore(decideWinner());

            playAgain();

        }

    }



    private void displayScore() {

        System.out.println(p.getName());

        System.out.println("----------");

        System.out.println("Wins: " + playerWins);

        System.out.println("Loses: " + playerLoses);

        System.out.println("Ties: " + ties);

        System.out.println("----------");

    }



    private int decideWinner() {

        // 0 - User wins

        // 1 - Computer wins

        // 2 - tie



        if(p.getChoice().equals("ROCK") && com.getChoice().equals("SCISSORS"))

            return 0;

        else if(p.getChoice().equals("PAPER") && com.getChoice().equals("ROCK"))

            return 0;

        else if(p.getChoice().equals("SCISSORS") && com.getChoice().equals("PAPER"))

            return 0;

        else if(com.getChoice().equals("ROCK") && p.getChoice().equals("SCISSORS"))

            return 1;

        else if(com.getChoice().equals("PAPER") && p.getChoice().equals("ROCK"))

            return 1;

        else if(com.getChoice().equals("SCISSORS") && p.getChoice().equals("PAPER"))

            return 1;

        else

            return 2;

    }



    private void displayChoices() {

        System.out.println("User has selected: " + p.getChoice());

        System.out.println("Computer has selected: " + com.getChoice());

    }



    private void displayWinner(int winner) {

        switch(winner) {

            case 0:

                System.out.println("User has won!");

                break;

            case 1:

                System.out.println("Computer has won!");

                break;

            case 2:

                System.out.println("It is a tie!");

        }

    }



    private void playAgain() {

        String choice;

        System.out.println("Do you want to play again? Enter Yes to play again.");

        choice = scan.nextLine();

        if(!(choice.toUpperCase().equals("YES") ))

            isRunning = false;



    }



    private void updateScore(int winner) {

        if(winner == 0)

            playerWins++;

        else if(winner == 1)

            playerLoses++;

        else if(winner == 2)

            ties++;

    }

}

This code really isn't a mess! While there are a few relatively minor points I can make, your code is generally easy to read and understand, and there's nothing horribly wrong about it.

1. Watch out for user input! I can enter Z (or anything but rock/paper/scissors) and the result will always be a tie. You should tell the user when they enter something invalid and have them correct it.




2. Both Computer and Player use setName to set their name - once. I would prefer requiring the use of the Player constructor with the name parameter since you could then mark name as final.




3. Why is setChoice public? It is (and should) only used by selectChoice, so mark it protected.




4. What happens when I want to play a game between two computer opponents (or two people)? Right now I can't. It would be nice if Game let me pass the two Player objects in that will be opponents.




5. While I appreciate the MAX_NUMBER constant in the Computer class instead of just including a magic number, it's a better idea to store your choices in an array and then select a random element of the array.



6. 
   

   Passing around strings for choices isn't so bad when just dealing with Rock/Paper/Scissors, but it will quickly become unmanagable. Enum Types were introduced to fix this problem, and generally do a pretty good job of it! It wouldn't hurt to start using them now. Another good spot for an enumeration is in the decideWinner method.

   
   
   

   The Choice enum type could also provide a method winsAgainst(Choice other) that can be used to simplify decideWinner.

   
   



7. For the most part, your names are great... until we get to Game and see p and com.




8. It would be nice to use the user's name instead of User when displaying the winner and when displaying choices.

The following points may directly contradict what your teacher says, as they are more opinion based. Follow whatever your team's (or assignment's) guidelines say.

1. 
   

   Don't be afraid to include more than one line of code in main. If I wrote this program my main function would look something like this:

   
   
   

   Scanner scanner = new Scanner(System.in);
   
   System.out.println("Please enter your name:");
   
   
   
   Player user = new User(scanner.nextLine());
   
   Player computer = new Computer();
   
   Game game = new Game(user, computer);
   
   
   
   do {
   
       game.play();
   
       System.out.println("Play again? [Yes/No]");
   
   } while (scanner.nextLine().toUpperCase().equals("YES"));
   
   

   
   



2. Use braces on all if statements that aren't one line long. I would be fine with if (winner == 0) playerWins++;, but if (winner == 0)n playerWins++; is much easier to mess up, especially if you don't have automatic indentation.




3. Don't declare variables before initializing them, if possible. There's no reason to declare choice before printing out instructions in the playAgain method.

After writing this review, I noticed there is still a few things that there were a some improvements that would require more-advanced-than-beginner levels of Java syntax. This answers focuses mostly on language agnostic improvements. There are some other things that can be added, but I really wanted to focus on the idea of a "Game Loop" before getting into too many details.

Game loop

You don't follow a typical game loop. Here is an article about creating a game loop. Fortunately, you don't need to worry about Frames Per Second (FPS) interpolation which is a large part of the article I linked. Nevertheless, it is imperative to look at this:

bool game_is_running = true;



while( game_is_running ) {

    update_game();

    display_game();

}

This logic should be in you main method. In other words, something like:

public class Main {

    public static void main(String args) {

        Game game = new Game();

        game.start();

        while (game.isRunning()) {

             game.step();

             game.display();

        }

    }

}

Refactoring start

Thus, it is imperative that we refactor this start method you have. First, make it public so I can call it as such. (There are actually a couple of other things related to starting, ending, and restarting games that you should probably do, but I think there is already enough in this CodeReview.)

What is step?

The step method mentioned above is actually located in your start. It is almost:

public void step() {

    displayScore();

    p.selectChoice();

    com.selectChoice();

    displayChoices();

    displayWinner(decideWinner());

    updateScore(decideWinner());

    playAgain();

}

We need still change a couple things:

1. This is more of stepAndDisplay not step.


2. We will need to make some changes about the Player class (more on that later).

Let's break step and display up.

step should generate the data, and handle input. display should output the data. So:

public void display() {

    displayOldScore();

    displayChoices();

    displayWinner();

    displayNewScore();

    playAgain();

}

And step could be:

public void step() {

    oldScore = newScore;

    p.selectChoice();

    com.selectChoice();

    newScore += decideWinner();

}

Refactoring step even further.

Instead of what I just wrote, the selectChoice() method is a bit awkward, I would just want to do:

public void step() {

    oldScore = newScore;

    newScore += decideWinner(p.getChoice(), com.getChoice());

}

But, even this can worked on:

Player class

Now back to the idea of selectChoice: Just implement getChoice as what is currently selectChoice. For example the Computer method selectChoice becomes:

public String getChoice() {

    int randomNumber = rand.nextInt(MAX_NUMBER);

    switch(randomNumber) {

        case 0:

            return "ROCK";

        case 1:

            return "PAPER";

        case 2:

            return "SCISSORS";

    }

}

Handling different player configurations

Currently, you cannot have two human players play each other. I would imagine you want this option.

Here are some changes that would need to be made in order for this to happen:

// Each player should have type player, don't make it `User` and `Computer`.

private Player p1;

private Player p2;



public Game() { // Default...

    p1 = new User();

    p2 = new Computer();

    scan = new Scanner(System.in);

}



public Game(Player firstPlayer, Player secondPlayer) {

    p1 = firstPlayer;

    p2 = secondPlayer;

    scan = new Scanner(System.in);

}

(There are further changes that I leave to you as an exercise: How do you change displayScore?)

Computer class

Your computer class is a particular kind of AI. Your Computer implements a "random selection" so I would change the class name to RandomComputer or something like that. Bottom line: You may want other computers in the future.

^{1. However You need an end game function too as well as some other additions.}