Problem
I just made a Rock, Paper, Scissors game and I have read some of the previous threads about them. Just wondering if there was any advice to optimize or just suggestions for naming or doing something better for my code. I have learned Java for only about 2 weeks so bear with me if I don’t understand some of the functions.
I read somewhere in another thread that I could use a method to print out blocks of text if they were repetitive. I’m wondering if I could implement that (if it’s actually more efficient) in my game.
Pastebin link for better viewing: http://pastebin.com/RbpaA4q2
import java.util.Random;
import java.util.Scanner;
public class Application {
public static void main(String[] args) {
boolean validMove;
int playerScore = 0;
int computerScore = 0;
String newLineSpacing = "n---------------";
do {System.out.println("Do you pick rock, paper or scissors?");
Scanner choice = new Scanner(System.in);
// The next line that the player types is the move he is making
String playerChoice = choice.nextLine();
validMove = true;
if (!("rock".equals(playerChoice) ||
"paper".equals(playerChoice) ||
"scissors".equals(playerChoice))) {
System.out.println("Move not recognized. Try again by running the program again.");
validMove = false;
} else {
Random randomNumber = new Random();
// Random number from 0 - 2 (0, 1, 2)
int computerNumber = randomNumber.nextInt(2);
// Computer chooses a move based on a randomly generated integer
// 0 = rock
// 1 = paper
// 2 = scissors
String computerMoveChoice;
if (computerNumber == 0) {
computerMoveChoice = "rock";
} else if (computerNumber == 1) {
computerMoveChoice = "paper";
} else {
computerMoveChoice = "scissors";
}
// Printing out what the computer chose
System.out.println("The computer chose: " + computerMoveChoice);
// Comparing the choices
// Both choices are the same (tie)
if (playerChoice == computerMoveChoice) {
System.out.println("It's a tie!");
System.out.println("Computer score: " + computerScore + "nPlayer score: " + playerScore + newLineSpacing);
}
// Player choose rock --- Computer choose paper (computer wins)
else if ("rock".equals(playerChoice) && "paper".equals(computerMoveChoice)) {
System.out.println("Paper beats rock. The computer wins!");
computerScore++;
System.out.println("Computer score: " + computerScore + "nPlayer score: " + playerScore + newLineSpacing);
}
// Player choose rock --- Computer choose scissors (player wins)
else if ("rock".equals(playerChoice) && "scissors".equals(computerMoveChoice)) {
System.out.println("Rock beats scissors. Player wins!");
playerScore++;
System.out.println("Computer score: " + computerScore + "nPlayer score: " + playerScore + newLineSpacing);
}
// Player choose paper --- Computer choose rock (player wins)
else if ("paper".equals(playerChoice) && "rock".equals(computerMoveChoice)) {
System.out.println("Paper beats rock. Player wins!");
playerScore++;
System.out.println("Computer score: " + computerScore + "nPlayer score: " + playerScore + newLineSpacing);
}
// Player choose paper --- Computer choose scissors (computer wins)
else if ("paper".equals(playerChoice) && "scissors".equals(computerMoveChoice)) {
System.out.println("Scissors beats paper. The computer wins!");
computerScore++;
System.out.println("Computer score: " + computerScore + "nPlayer score: " + playerScore + newLineSpacing);
}
// Player choose scissors --- Computer choose rock (computer wins)
else if ("scissors".equals(playerChoice) && "rock".equals(computerMoveChoice)) {
System.out.println("Rock beats scissors. The computer wins!");
computerScore++;
System.out.println("Computer score: " + computerScore + "nPlayer score: " + playerScore + newLineSpacing);
}
// Player choose scissors --- Computer choose paper (player wins)
else if ("scissors".equals(playerChoice) && "paper".equals(computerMoveChoice)) {
System.out.println("Scissors beats paper. Player wins!");
playerScore++;
System.out.println("Computer score: " + computerScore + "nPlayer score: " + playerScore + newLineSpacing);
}
}
} while (validMove == true);
// Only loop this when validMove is true (one of the options were chosen)
}
}
Solution
Bugs
First of all, there are 2 bugs:
-
In the code determining the choice by the computer, you have
int computerNumber = randomNumber.nextInt(2);However,
nextInt(2)will not return a random number between 0 and 2 inclusive, but 0 and 2 exclusive. As such, this method will return either 0 or 1. You need to callnextInt(3)to return a random number in the interval[0,2]. -
If both choice are the same, you are comparing the two Strings with
if (playerChoice == computerMoveChoice) { // ... }As you have done in the rest (so I guess this is more a typo), Strings should be compared with
equalsand not==.
To the refactorings!
Computer choice
What you’re missing are methods. All of the code is inside the main method: it would be preferable to create dedicated methods to make the code a lot clearer. Let’s to this part by part.
The choice of the computer can be safely extracted into a method getComputerChoice(). Taking directly your code, it would look like:
private static String getComputerChoice() {
Random randomNumber = new Random();
// Random number from 0 - 2 (0, 1, 2)
int computerNumber = randomNumber.nextInt(3);
// Computer chooses a move based on a randomly generated integer
// 0 = rock
// 1 = paper
// 2 = scissors
String computerMoveChoice;
if (computerNumber == 0) {
computerMoveChoice = "rock";
} else if (computerNumber == 1) {
computerMoveChoice = "paper";
} else {
computerMoveChoice = "scissors";
}
return computerMoveChoice;
}
That can be improved a lot.
- This method is creating a new
Randomobject each time it is invoked. This is not a good idea and leads to very poor random numbers. ARandomobject should be created just once and shared by the rest of the application. In this case, it can be made a constant of the game withprivate static final Random RANDOM = new Random();. - To return the choice of the computer, an
if / else ifis performed on the possible random integers generated. Instead, consider having a list of the possible values, and returning a random element from the list.
With those two changes, you have:
private static final Random RANDOM = new Random();
private static final List<String> CHOICES = Arrays.asList("rock", "paper", "scissors");
private static String getComputerChoice() {
return CHOICES.get(RANDOM.nextInt(CHOICES.size()));
}
We were able to break it down into a very clear single line of code. Note also that we don’t hardcode the number of choice anymore (like we did before with nextInt(3)). If tomorrow, we add another choice, it will be an easy change to the CHOICES list.
Player choice
Just like the computer choice, the choice of the player should be extracted inside its own dedicated method.
private static String getPlayerChoice() {
System.out.println("Do you pick rock, paper or scissors?");
Scanner choice = new Scanner(System.in);
String playerChoice = choice.nextLine();
if (CHOICES.contains(playerChoice)) {
return playerChoice;
}
throw new InvalidChoiceException(playerChoice);
}
The method asks the player their choice. If it’s a correct choice, it returns it. To determine whether it is correct, we don’t rely on the hard-coded list of choices, we can reuse the CHOICES list made before, which now makes this method generic.
What happens in case it isn’t correct can be discussed: you can return null but I find clearer to throw a custom exception: the user entering garbage qualifies as an exceptional condition that getPlayerChoice shouldn’t need to handle. It is up to the game to decide what to do in that case. The exception takes the incorrect choice as parameter, in case we want to log it later on.
With this, the rest of the code needs to be changed accordingly. First, validMove doesn’t make sense anymore: this was effectively replaced by the exception InvalidChoiceException. We can now have:
while (true) {
String playerChoice, computerMoveChoice;
try {
playerChoice = getPlayerChoice();
computerMoveChoice = getComputerChoice();
} catch (InvalidChoiceException e) {
System.out.println("Move not recognized. Try again by running the program again.");
break;
}
// .. rest of code here
}
Notice that InvalidChoiceException could, in theory, be thrown by the player or the computer, so it makes sense to try both of the calls.
The winner / loser logic
So far, the logic to determine who wins the game (which relies on a lot of comparison between Strings), the logic to print the correct message and the logic to update the scores, are intermingled.
The first issue is comparing raw Strings to determine who is winning. This is error-prone and it is better to use an enum for all the possible moves:
private static enum Choice {
ROCK, PAPER, SCISSORS;
public boolean beats(Choice choice) {
switch (this) {
case ROCK:
return choice == SCISSORS;
case PAPER:
return choice == ROCK;
case SCISSORS:
return choice == PAPER;
default:
return false;
}
}
}
Another important method was added: it is the responsiblity of a choice to determine if it beats another one, so a method beats is added. There are several approach to implemeting it, but a switch over the possible values is the simplest. Another would be building a map of the values before-hand and re-using this map.
This implies some other changes:
- both
getPlayerChoiceandgetComputerChoicewould now return aChoice. -
our previous list of choices now becomes:
private static final List<Choice> CHOICES = Arrays.asList(Choice.ROCK, Choice.PAPER, Choice.SCISSORS);with no hard coded Strings.
Now that we have this, let’s consider the choice of both players are in a Choice variable. In order to do that, we can call Choice.valueOf on the value entered by the player (we know it’s valid, an exception would have been thrown earlier otherwise). The whole block of code comes down to:
if (playerChoice == computerMoveChoice) {
System.out.println("It's a tie!");
} else if (playerChoice.beats(computerMoveChoice)) {
System.out.println(playerChoice + " beats " + computerMoveChoice + ". Player wins!");
playerScore++;
} else {
System.out.println(computerMoveChoice + " beats " + playerChoice + ". The computer wins!");
computerScore++;
}
System.out.println("Computer score: " + computerScore + "nPlayer score: " + playerScore + newLineSpacing);
which is very simple and quite clear. You can argue that the messages aren’t strictly the same as before, but you could add a getName() to each Choice in order to return a more friendly String instead of relying on the name() of the enum.
Printing the current score
The line
System.out.println("Computer score: " + computerScore + "nPlayer score: " + playerScore + newLineSpacing);
should be extracted into its own method. The variable newLineSpacing is awkward because it is in the middle of the rest of the code, and not separated, as it should since it is only relevant for this part. With the following method
private static void printCurrentScores(int playerScore, int computerScore) {
System.out.println("Computer score: " + computerScore + "nPlayer score: " + playerScore + "n---------------");
}
the concern is separated and the rest of the code can simply invoke this method.
Putting this all in
With all of those changes, the main method now looks like:
public static void main(String[] args) {
int playerScore = 0;
int computerScore = 0;
while (true) {
Choice playerChoice, computerMoveChoice;
try {
playerChoice = getPlayerChoice();
computerMoveChoice = getComputerChoice();
} catch (InvalidChoiceException e) {
System.out.println("Move not recognized. Try again by running the program again.");
break;
}
if (playerChoice == computerMoveChoice) {
System.out.println("It's a tie!");
} else if (playerChoice.beats(computerMoveChoice)) {
System.out.println(playerChoice + " beats " + computerMoveChoice + ". Player wins!");
playerScore++;
} else {
System.out.println(computerMoveChoice + " beats " + playerChoice + ". The computer wins!");
computerScore++;
}
printCurrentScores(playerScore, computerScore);
}
}
Can we do better?
Yes, we could! In the last snippet, the logic to determine who wins the game, to print the message and to update the scores, are still intermingled. There should be distinct methods:
- Given each player’s choice, determine who is the winner.
- Given the winner, update the scores.
- Given the winner (and each player’s choice), print the correct message.
It is clear that those methods will need to share some data and that is what the code is missing: an object capable of representing the current state of the game. That data can be safely encapsulated inside objects.
-
Consider adding a
Playerobject that holds its current score. In the previous code, we can see this type is needed. Look atint playerScore = 0; int computerScore = 0;Generally, this shows a missing object, encapsulating the score, and two instances holding their respective score.
-
Condider adding a
Gameclass that hold the current state of the game.
There are several ways to implement that, but as a first example, the player could have a method returning its next choice (or throwing an exception of an incorrect choice); the game could have a method taking 2 players and determining the winner for each turn, etc.