mjhjmtu

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++;
 

Good job so far!

The previous answers already cover a lot, I just want to add this:

Don't use String for state!

Currently you use String to encode the Players Choice. This is hard to refactor and it is difficult to add functionality. Prefer its own Class or Enum.

For example

 enum Choice ROCK, PAPER, SCISSORS

Then, you can have a variable containing the choice, for example:

 Choice choice = Choice.ROCK;

You can add behaviour to the enum:

enum Choice 
 ROCK, PAPER, SCISSORS

 public boolean beats(Choice other)
 
 switch (this)
 
 case ROCK:
 return other == SCISSORS;
 case PAPER:
 return other == ROCK;
 case SCISSORS:
 return other == PAPER;
 

 

Now, your other code can just call myChoice.beats(otherChoice).

Also, you can implement random() in the enum, as well as parseFromUserInput(String s). If you do this correctly, you can change the enum, adding different choices, without needing to change any other code in your application.

So, if you want to implement RockPaperScissorsLizardSpock , you just extend the Enum, and be done!

Also:

Use Enum instead of magic int values

Instead of returning int that encodes the result of the round, use a explicit enum as well, replacing the boolean beats() method above:

enum Result WIN, LOSS, DRAW 

public Result fights(Choice other)
 
 switch (this)
 
 case ROCK:
 if (other == ROCK)
 return Result.DRAW
 return other == SCISSORS ? Result.WIN : Result.LOSS;
 ....
 

 

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.