7
\$\begingroup\$

I have written my first rock, paper, scissors project recently. I had to use a lot of if statements and System.out.println(); commands in the program. I am wondering what ways I could optimize my program to contain less lines of code/run faster. Are there ways to get the job done using more practical coding techniques like utilizing classes in this program?

import java.util.Random;
import java.util.Scanner;

public class Application {

    public static void main(String[] args) {

        System.out.println("Welcome to Rock, Paper, Scissors; Let's Play!");

        Scanner playerName = new Scanner(System.in);
        Scanner roundsToWin = new Scanner(System.in);

        System.out.println("Please enter your name: ");
        String text = playerName.nextLine();

        System.out.println("Hello, " + text + "\nHow many rounds would you like to play?");
        int rounds = roundsToWin.nextInt();

        int value = 0;
        int player = 0;
        int computer = 0;

        do {
            // System.out.println("Current rounds played: " + value);

            String[] rockPaperScissors = {"Rock", "Paper", "Scissors"};
            Random random = new Random();

            Scanner playerChoice = new Scanner(System.in);
            System.out.println("Please enter Rock, Paper or Scissors: \nCapitilization Matters!");
            String choice = playerChoice.nextLine();
            System.out.println();

            int select = random.nextInt(rockPaperScissors.length);

            System.out.println("Computer selection: " + rockPaperScissors[select]);
            System.out.println("Your selection: " + choice);
            System.out.println();

            if (choice.equals(rockPaperScissors[select])) {
                System.out.println("It is a Tie");
            }
            else {
                if(choice.equals("Rock")) {
                    if(rockPaperScissors[select].equals(rockPaperScissors[1])) {
                        System.out.println("Paper beats rock.");
                        computer++;
                    }
                    if(rockPaperScissors[select].equals(rockPaperScissors[2])) {
                        System.out.println("Rock beats scissors.");
                        player++;
                    }
                }
                if(choice.equals("Paper")) {
                    if(rockPaperScissors[select].equals(rockPaperScissors[0])) {
                        System.out.println("Paper beats rock.");
                        player++;
                    }
                    if(rockPaperScissors[select].equals(rockPaperScissors[2])) {
                        System.out.println("Scissors beat paper.");
                        computer++;

                    }

                }
                if(choice.equals("Scissors")) {
                    if(rockPaperScissors[select].equals(rockPaperScissors[0])) {
                        System.out.println("Rock beats scissors.");
                        computer++;

                    }
                    if(rockPaperScissors[select].equals(rockPaperScissors[1])) {
                        System.out.println("Scissors cuts paper.");
                        player++;
                    }
                }
            }

            System.out.println();
            System.out.println(text + ": " + player);
            System.out.println("Computer: " + computer);
            System.out.println();
            System.out.println();


            try {
                Thread.sleep(3000);                 //1000 milliseconds is one second.
            } catch(InterruptedException ex) {
                Thread.currentThread().interrupt();
            }

            value++;
        }
        while(value < rounds);

        System.out.println("Final Score!");
        System.out.println(text + ": " + player);
        System.out.println("Computer:" + computer);
        System.out.println();

        if(computer > player) {
            System.out.println("You Lose! Computer Wins");
        }
        if(player > computer) {
            System.out.println("Congratulations " + text + ", You won!");
        }


    }

}
\$\endgroup\$
  • \$\begingroup\$ Welcome to Code Review! I hope you get some great answers. \$\endgroup\$ – Phrancis Jun 1 '16 at 21:51
8
\$\begingroup\$

First up, I read carefully your question and found a string 'I have a lot of if statements'.

Perfomance Issue

Unnecessary if checks

Lets look deeply in your code.

if (choice.equals("Rock")) {
    // some code
}
if (choice.equals("Paper")) {
    // some code
}
if (choice.equals("Scissors")) {
    // some code
}

Using only if we will have (it's not a Java code, it's just an example).

choice = "Rock" => 3 check
choice = "Paper" => 3 checks
choice = "Scissors" => 3 checks

In all cases you will have 3 checks.

Solution

From if to if/else if.

if (choice.equals("Rock")) {
    // some code
} else if(choice.equals("Paper")) {
   // some code
} else if(choice.equals("Scissors")) {
  // some code
}

Using if/else if we will have (it's not a Java code, it's just an example).

choice = "Rock" => 1 check
choice = "Paper" => 2 checks
choice = "Scissors" => 3 checks

It's better than previous.

Memory Issue

You don't need to create new variables in each new round of your game. Create once at the beginning.

String[] rockPaperScissors = {"Rock", "Paper", "Scissors"};
Scanner playerChoice = new Scanner(System.in);
Random random = new Random();

do {
   // your code
} while(your_condition);

You don't need 2 scanners for reading user input. You can use one.

Instead of:

Scanner playerName = new Scanner(System.in);
Scanner roundsToWin = new Scanner(System.in);

You can use:

Scanner sc = new Scanner(System.in);
String playerName = sc.nextLine();
int rounds = sc.nextInt();

UPDATE

From if/else if to switch. Sometimes it's difficult to read a lot of if/else statements in the code. We can replace last if/else if statement with switch to improve readability. Also replace string with enum.

public enum GameChoice {
   Rock,
   Paperm,
   Scissors
}

switch (choice) {
   case GameChoice.Rock:
      //some code
      break;
   case GameChoice.Paper:
      // some code
      break;
   case GameChoice.Scissors:
      // some code
      break;
   default:
      // some code
      break;
}

Bug

There is a probability when (computer == player), but you don't handle it.

if (computer > player) {
   System.out.println("You Lose! Computer Wins");
} else if (computer == player) {
   System.out.println("It is a draw");
} else {
   System.out.println("Congratulations " + text + ", You won!");
}

Naming

Please, provide more intuitive name for variables, because when I first looked in your code I didn't understand variables names: computer and player variables.

computer can be replaced with computerScore

player can be replaced with playerScore

value can be replaced with roundIndex

text can be replaced with playerName

Design Issue

DRY ideology

Do not repeat your self

Look carefully in your code and find a part that repeat.

1) 2 times print the score of the game

System.out.println(playerName + ": " + player);
System.out.println("Computer: " + computer);
System.out.println();

Move this part to a separate function.

private static void printScore(String playerName, int playerScore, int computerScore) {
   System.out.println(playerName + ": " + playerScore);
   System.out.println("Computer:" + computerScore);
   System.out.println();
}

Break big function into small several functions

Do One Thing and Do It Well

1) Function for printing result of the game

private static void printGameResult(String playerName, int playerScore, int computerScore) {
   System.out.println("Final Score!");
   printScore(playerName, playerScore, computerScore);

   if (computerScore > playerScore) {
      System.out.println("You Lose! Computer Wins");
    } else if (computerScore == playerScore) {
      System.out.println("It is a draw");
    } else {
      System.out.println("Congratulations " + playerName + ", You won!");
    }
}
\$\endgroup\$
  • 2
    \$\begingroup\$ Readability has no metric as it is totally subjective. Use an enum instead of strings for "Choices". Then you got a point to improve because you limit the Elements possible in switch case and you get compiler support if you are missing one element. I agree with the rest. So one up. \$\endgroup\$ – oopexpert Jun 2 '16 at 16:33
  • \$\begingroup\$ @oopexpert, yes I agree with readability issue about using if/else if or switch. Thanks for pointing to enum. \$\endgroup\$ – dshil Jun 3 '16 at 11:37
5
\$\begingroup\$

Consider implementing a Map

You note worries about the nested and multiple conditionals. Let's break down how we can amend this.

In this game, We have three variables of importance: rock paper and scissors.

We also happen to only need three rules to encompass the essential game dynamic:

  1. Rock beats Scissors.
  2. Paper beats Rock
  3. Scissors beats Paper.

As it turns out We can frame this exact relationship using a Map and simply check if a given value matches the key.

So we would have:

Map<String, String> winningMap = new HashMap<>();
winningMap.put("Rock", "Scissors"); // Rock beats Scissors
winningMap.put("Paper", "Rock"); // Paper beats Rock
winningMap.put("Scissors", "Paper"); // Scissors beats Paper

and you would simply check for equivalence and whether or not the given input key matches the value in the map which significantly reduces your conditional statements.

if (userChoice.equals(computerChoice)) {
    System.out.println("It's a tie!");
} else if (computerChoice.equals(winningMap.get(userChoice))) {
     System.out.println("You win!");
} else {
     System.out.println("You lose!");
}

which we can even reduce to a rather succinct ternary statement:

System.out.println(
    userChoice.equals(computerChoice) ? "It's a tie!" :
    computerChoice.equals(winningMap.get(userChoice)) ? "You win!" : "You lose!"
);

On Case Sensitivity

You also comment about case sensitivity, rather than relying on the user to provide valid input pick a case and simply call the String class's built in toUpperCase() or toLowerCase() methods or make your comparisons with equalsIgnoreCase().

Input Validation

While on this end, it's a good opportunity to make a method to ensure the input is correct in the first place, a simple while loop and conditional check should suffice, you have an array of valid choices already so we can use that

private static boolean isValidChoice(String userChoice, String[] validChoices) {
    for (String choice : validChoices) {
        if (userChoice.equals(choice)) {
            return true;
        }
    }
    return false;
}

and simply ensure validation by evaluating with this method in a while loop, like so:

while (!isValidChoice(userChoice, choices)) {
    System.out.println("Please pick Rock, Paper, or Scissors");
    userChoice = input.nextLine(); // maybe here we set to lowercase?
}

Example Implementation

Taking all these ideas together, here's a runnable example:

import java.util.HashMap;
import java.util.Map;
import java.util.Scanner;
import java.util.Random;

public class RockPaperScissors {
    public static void main(String[] args) {
        Scanner input = new Scanner(System.in);
        Random random = new Random();
        String[] choices = {"rock", "paper", "scissors"};

        Map<String, String> winningMap = new HashMap<>();
        winningMap.put("rock", "scissors"); // Rock beats Scissors
        winningMap.put("paper", "rock"); // Paper beats Rock
        winningMap.put("scissors", "paper"); // Scissors beats Paper

        System.out.print("Do you choose Rock, Paper, or Scissors? ");
        String computerChoice = choices[random.nextInt(choices.length)];
        String userChoice = input.nextLine().toLowerCase();

        while (!isValidChoice(userChoice, choices)) {
            System.out.println("Please pick Rock, Paper, or Scissors");
            userChoice = input.nextLine().toLowerCase();
        }

        System.out.println(
            "You chose: " + userChoice
            + "\nThe computer chose: " + computerChoice
        );

        System.out.println(
            userChoice.equals(computerChoice) ? "It's a tie!" :
            computerChoice.equals(winningMap.get(userChoice)) ? "You win!" : "You lose!"
        );
    }

    private static boolean isValidChoice(String userChoice, String[] validChoices) {
        for (String choice : validChoices) {
            if (userChoice.equals(choice)) {
                return true;
            }
        }
        return false;
    }
}
\$\endgroup\$
  • \$\begingroup\$ The advice about toLowerCase(), toUpperCase() and equalsIgnoreCase() is great. In this context not yet relevant, but: Those methods have some nasty edge-cases involving unicode and Languages with glyphs that don't have an uppercase equivalent (e.g. German ß) That can result in funny behaviour. Scißors can equalsIgnoreCase to Scissors, IIRC :D \$\endgroup\$ – Vogel612 Jun 2 '16 at 8:53
  • 1
    \$\begingroup\$ I think this problem is bagging for an enum. \$\endgroup\$ – rdllopes Jun 3 '16 at 14:38
  • 1
    \$\begingroup\$ OP is a beginner and I elected against introducing this. It seems like overkill where a Map accomplishes the same and addresses the main concern. Using an Enum just for the sake of using enum -- without making use of the methods accompanying methods is rather pointless. \$\endgroup\$ – Legato Jun 3 '16 at 16:00
  • \$\begingroup\$ That said, I can see full on OOP easily encapsulating the gamechoices, gamestates as enumerations. \$\endgroup\$ – Legato Jun 3 '16 at 16:03
1
\$\begingroup\$

Since you have only 3 possible choices for the player I'd suggest this calls for an enum. Although you're new to programming it is still important to understand the features of the Java language if you're going to continue using it. So, here is the tutorial page for enum if you'd like to read in more detail. Below is your code modified to use an enum, a single Scanner object, single Random object, and separate methods for obtaining valid user input and printing scores.

I tried to make sure to write a comment for each change I made to the code. Please see the comments in the code below. Note I tried to maintain the output you had in your original code as closely as possible.

import java.util.Random;
import java.util.Scanner;

public class RockPaperScissors {

    /**
     * An enum representing all allowable inputs to the game.
     */
    private static enum Option{
        ROCK, PAPER, SCISSORS;

        private Option beatenBy;

        public Option getBeatenBy(){
            return beatenBy;
        }

        public void setBeatenBy(Option opt){
            beatenBy = opt;
        }

        /**
         * Initialize the options by setting their "beatenBy" fields.
         */
        public static void init(){
            Option.ROCK.setBeatenBy(Option.PAPER);
            Option.PAPER.setBeatenBy(Option.SCISSORS);
            Option.SCISSORS.setBeatenBy(Option.ROCK);
        }
    }

    /**
     * Converts a String input to an Option
     * @param input the String to convert to an Option
     * @return the Option matching the uppercase of the input String.
     */
    private static Option createOption(String input){
        input = input.toUpperCase();
        return Option.valueOf(input);
    }

    public static void main(String[] args) {
        Option.init();
        System.out.println("Welcome to Rock, Paper, Scissors; Let's Play!");

        Scanner inputScanner = new Scanner(System.in);
//        Removed second scanner, re-use the first one.
//        Scanner roundsToWin = new Scanner(System.in);

        System.out.println("Please enter your name: ");
//        renamed "text" to "playerName" to better represent what its purpose is.
        String playerName = inputScanner.nextLine();

        System.out.println("Hello, " + playerName + "\nHow many rounds would you like to play?");
        int rounds = inputScanner.nextInt();

//        renamed "value" to "currRound" to indicate that it represents the current round number.
        int currRound = 0;
        // renamed "player" and "computer" to add the suffix "Score" to indicate these represent the scores of the player and computer.
        int playerScore = 0;
        int computerScore = 0;

        Random random = new Random();
        do {
            // System.out.println("Current rounds played: " + value);

            //Removed the array that previously stored the options
//            String[] rockPaperScissors = {"Rock", "Paper", "Scissors"};
            //moved Random object outside the loop

//            Removed additional scanner, re-use the one defined earlier.
//            Scanner playerChoice = new Scanner(System.in);

//            Moved input scanning logic to separate method that also checks for valid input.
            Option playerSelection = getUserInput(inputScanner);

            int select = random.nextInt(Option.values().length);
            Option computerSelection = Option.values()[select];

            System.out.println();
            System.out.println("Computer selection: " + computerSelection);
            System.out.println("Your selection: " + playerSelection);
            System.out.println();

            if(playerSelection == computerSelection){
                System.out.println("It is a Tie");
            }else if(playerSelection == computerSelection.getBeatenBy()){
                System.out.println(playerSelection.name() + " beats "+computerSelection.name());
                playerScore++;
            }else if(playerSelection.getBeatenBy() == computerSelection){
                System.out.println(computerSelection.name() + " beats "+playerSelection.name());
                computerScore++;
            }else{
                System.err.println("This shouldn't happen! Something went terribly wrong!");
            }

//            Removed previous logic below
//            if (choice.equals(rockPaperScissors[select])) {
//                System.out.println("It is a Tie");
//            }
//            else {
//                if(choice.equals("Rock")) {
//                    if(rockPaperScissors[select].equals(rockPaperScissors[1])) {
//                        System.out.println("Paper beats rock.");
//                        computerScore++;
//                    }
//                    if(rockPaperScissors[select].equals(rockPaperScissors[2])) {
//                        System.out.println("Rock beats scissors.");
//                        playerScore++;
//                    }
//                }
//                if(choice.equals("Paper")) {
//                    if(rockPaperScissors[select].equals(rockPaperScissors[0])) {
//                        System.out.println("Paper beats rock.");
//                        playerScore++;
//                    }
//                    if(rockPaperScissors[select].equals(rockPaperScissors[2])) {
//                        System.out.println("Scissors beat paper.");
//                        computerScore++;
//
//                    }
//
//                }
//                if(choice.equals("Scissors")) {
//                    if(rockPaperScissors[select].equals(rockPaperScissors[0])) {
//                        System.out.println("Rock beats scissors.");
//                        computerScore++;
//
//                    }
//                    if(rockPaperScissors[select].equals(rockPaperScissors[1])) {
//                        System.out.println("Scissors cuts paper.");
//                        playerScore++;
//                    }
//                }
//            }

            System.out.println();
            printScores(playerName, playerScore, computerScore);
            System.out.println();
            System.out.println();


            try {
                Thread.sleep(3000);                 //1000 milliseconds is one second.
            } catch(InterruptedException ex) {
                Thread.currentThread().interrupt();
            }

            currRound++;
        }
        while(currRound < rounds);

        System.out.println("Final Score!");
        printScores(playerName, playerScore, computerScore);
        System.out.println();

        //modified second if by changing to else-if instead since no need to 
        // process the second condition if the first is true.
        if(computerScore > playerScore) {
            System.out.println("You Lose! Computer Wins");
        }else if(playerScore > computerScore) {
            System.out.println("Congratulations " + playerName + ", You won!");
        }

        //Added a call to close the scanner
        inputScanner.close();
    }

    private static void printScores(String playerName, int playerScore, int computerScore){
        System.out.println(playerName + ": " + playerScore);
        System.out.println("Computer: " + computerScore);
    }

    /**
     * Gets input from the user using the specified Scanner and attempts to 
     * parse that input into a valid game Option. If the input is not valid the 
     * user is prompted for input again until valid input is entered.
     * @param inputScanner the scanner to use for obtaining input
     * @return the Option 
     */
    public static Option getUserInput(Scanner inputScanner){
        Option playerSelection = null;
        while(true){
            System.out.println("Please enter Rock, Paper or Scissors: ");
            String choice = inputScanner.next();
            try{
                 playerSelection = createOption(choice);
                 return playerSelection;
            }catch(IllegalArgumentException e){
                //The user entered an invalid value, prompt again.
                System.out.println("Sorry, \""+choice+"\" is not a valid option.");
            }
        }
    }
\$\endgroup\$
  • \$\begingroup\$ DB you can accept some of the answers or make some feedback to reviewers. \$\endgroup\$ – dshil Jun 24 '16 at 3:17

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy