4
\$\begingroup\$

Any feedback on this code would be greatly appreciated! I am just trying to improve so I can move on to bigger projects. Thank you so much!

    import java.util.*;
   import java.util.Random;

   public class RockPaperScissors {
     public static void main(String [] args)
     {


    Random rd = new Random();
    Scanner kb = new Scanner(System.in);
    System.out.println("Welcome to Rock Paper Scisscors!");

    while(true)
    {
    int rock = 1;
    int paper = 2;
    int scissors = 3;

    int conditions [] = {rock, paper, scissors};
    int random = (conditions[new Random().nextInt(conditions.length)]);

    System.out.print("Please Enter: \n1 for Rock, \n2 for Paper, \n3 for Scissors: ");
    int player1 = kb.nextInt();



    if(player1 == rock && random == rock)
    {
        System.out.print("You have tied, computer rock, please try again\n");
    }
    else if(player1 == paper && random == paper)
    {
        System.out.print("You have tied, computer chose paper, please try again");
    }
    else if(player1 == scissors && random == scissors)
    {
        System.out.print("You have tied, computer chose scissors, please try again");
    }

    if(player1 == rock && random == paper)
    {
        System.out.print("Computer chose paper, computer has won");
        break;
    }
    else if(player1 == paper && random == rock)
    {
        System.out.print("You win!, computer chose rock");
        break;
    }
    else if(player1 == paper && random == scissors)
    {
        System.out.print("Computer chose scissors, computer has won");
        break;
    }
    else if(player1 == scissors && random == paper)
    {
        System.out.print("You win, computer chose paper");
        break;
    }
    else if(player1 == rock && random == scissors)
    {
        System.out.print("You win!, computer chose scissors");
        break;
    }
    else if(player1 == scissors && random == rock)
    {
        System.out.print("Computer chose rock, computer has won");
        break;
    }


    if(player1 != rock && player1 != paper && player1 != scissors )
    {
        System.out.print("Invalid response");
        break;
    }
    }

     }



   }
\$\endgroup\$

2 Answers 2

2
\$\begingroup\$

Welcome to Code Review, I have some suggestions for you:

Logic of the game and a possible implementation

Starting from your code:

int rock = 1;
int paper = 2;
int scissors = 3;

You already identified that rock, paper, and scissors can be represented as three consecutive numbers, a better choice of these three numbers is rock = 0, paper = 1 and scissors = 2. Why ? Because if player1 and random are the choices for the player and computer in the range of integers 0, 1, 2 you can determine who is the winner checking the conditions player1 == ((random + 1) % 3) and random == ((player1 + 1) % 3) . If the first one is true player wins, if the second one is true computer wins, if both are false there is a draw.

A possibility to reduce your code is given by creating an Enum Hand:

private enum Hand {ROCK, PAPER, SCISSORS};
//the method below will print for 0, 1, 2 values for random the Strings ROCK, PAPER, SCISSORS
String hand = Hand.values()[random].name();
//with lowercase ROCK, PAPER, SCISSORS strings will become rock, paper, scissors
String lower = hand.toLowerCase();

The good thing about Enum is that ROCK, PAPER, SCISSORS have the default values of 0, 1, 2 so you don't need to define them, so you can define a prototype for your class like this:

public class RockPaperScissors {

    private enum Hand {ROCK, PAPER, SCISSORS};

    public static void main(String[] args) { /*here your logic*/ }
}

Because you have a lot of repetitive messages about the game differing just for one word you can define String templates like these:

String messageTie = "You have tied, computer %s, please try again\n";
String messageWin = "You win, computer chose %s\n";
String messageLost = "Computer chose %s, computer has won\n";

Notice that %s can be substituted by the words rock, paper, scissors so the number of messages decreases to three.

You can write your class like this:

public class RockPaperScissors {

    private enum Hand {ROCK, PAPER, SCISSORS};

    public static void main(String[] args) {

        Random rd = new Random();
        String messageTie = "You have tied, computer %s, please try again\n";
        String messageWin = "You win, computer chose %s\n";
        String messageLost = "Computer chose %s, computer has won\n";

        try (Scanner kb = new Scanner(System.in)) {
            while (true) {
                System.out.print("Please Enter: \n0 for Rock, \n1 for Paper, \n2 for Scissors: ");
                int player1 = kb.nextInt();
                if(player1 < 0 || player1 > 2) {
                    System.out.print("Invalid response");
                    break;
                }
                //here the logic to determine the winner
        }
    }

}

My implementation differs from your's because I'm using the try with resources statement and legitimate choices for players are numbers 0, 1, 2, different number of another type of input will bring to the termination of the program.

The logic to determine the winner and print the appropriate the right message is the following:

int random = (rd.nextInt(3));
String messageGame = messageTie;
if (player1 == ((random + 1) % 3)) {
    messageGame = messageWin;
} 
if (random == ((player1 + 1) % 3)) {
    messageGame = messageLost;
}
String hand = Hand.values()[random].name();
System.out.printf(messageGame, hand.toLowerCase());

Initially I'm supposing the game will end with a draw result and it will be printed the draw message, if one of the condition is true the message will change. The method System.out.printf will take the computer hand result as a parameter.

Here the code complete of my version of the class RockPaperScissors:

RockPaperScissors.java

public class RockPaperScissors {

    private enum Hand {ROCK, PAPER, SCISSORS};

    public static void main(String[] args) {

        Random rd = new Random();
        String messageTie = "You have tied, computer %s, please try again\n";
        String messageWin = "You win, computer chose %s\n";
        String messageLost = "Computer chose %s, computer has won\n";

        try (Scanner kb = new Scanner(System.in)) {
            while (true) {
                System.out.print("Please Enter: \n0 for Rock, \n1 for Paper, \n2 for Scissors: ");
                int player1 = kb.nextInt();
                if(player1 < 0 || player1 > 2) {
                    System.out.print("Invalid response");
                    break;
                }
                int random = (rd.nextInt(3));
                String messageGame = messageTie;
                if (player1 == ((random + 1) % 3)) {
                    messageGame = messageWin;
                } 
                if (random == ((player1 + 1) % 3)) {
                    messageGame = messageLost;
                }
                String hand = Hand.values()[random].name();
                System.out.printf(messageGame, hand.toLowerCase());
            }
        }
    }

}
\$\endgroup\$
1
\$\begingroup\$

Suggestions

Unused variable

The variable rd is not used, you can remove it.

Put variables that don’t change out of the loop

In the while loop, the rock, paper and scissors get their value reassigned with the same value each time the loop restart, move them outside the loop.

I suggest that you put those as class constants, so you will be able to reuse them in other methods.

public class RockPaperScissors {
    public static final int ROCK = 1;
    public static final int PAPER = 2;
    public static final int SCISSORS = 3;
}

Validate the invalid choice first

In your code, you check if the chosen option is valid at the end of the loop; it will be more efficient if you check the invalid options first. This will prevent to check every other conditions and quit the loop faster.

System.out.print("Please Enter: \n1 for Rock, \n2 for Paper, \n3 for Scissors: ");
int player1 = kb.nextInt();

if (player1 != rock && player1 != paper && player1 != scissors) {
   System.out.print("Invalid response");
   break;
}

Extract some of the logic into methods

In your code, you can move some of the logic into a method to allow the code to be shorter and easier to read.

  1. I suggest make methods to check if the answer is a rock, paper or scissors; this will allow the conditions to be shorter.
public static void main(String[] args) {
   while (true) {
      //[...]
      if (isRock(player1) && isRock(random)) {
         //[...]
      }
      //[...]
   }
}

private static boolean isRock(int answer) {
   return answer == ROCK;
}

private static boolean isPaper(int answer) {
   return answer == PAPER;
}

private static boolean isScissor(int answer) {
   return answer == SCISSORS;
}
  1. I suggest that you extract the logic that read the value in a method.
public static void main(String[] args) {
   while (true) {
      //[...]
      int player1 = getPlayer1Answer(kb);
   }
}

private static int getPlayer1Answer(Scanner kb) {
   System.out.print("Please Enter: \n1 for Rock, \n2 for Paper, \n3 for Scissors: ");
   return kb.nextInt();
}

Bonus

While you are in the method, I suggest making another loop to verify the validity of the answer; this will make sure the provided choice is valid and ask again if not.

private static int getPlayer1Answer(Scanner scanner) {
   int value;

   while (true) {
      System.out.print("Please Enter: \n1 for Rock, \n2 for Paper, \n3 for Scissors: ");
      value = scanner.nextInt();

      if (isRock(value) || isPaper(value) || isScissor(value)) {
         break;
      } else {
         System.out.println("Invalid response");
      }
   }

   return value;
}
  1. I suggest that you extract the logic that generates the computer answer in a method.
    private static int getComputerAnswer() {
        int[] conditions = {ROCK, PAPER, SCISSORS};
        return conditions[new Random().nextInt(conditions.length)];
    }

Refactored code

public class RockPaperScissors {
    public static final int ROCK = 1;
    public static final int PAPER = 2;
    public static final int SCISSORS = 3;

    public static void main(String[] args) {
        Scanner kb = new Scanner(System.in);
        System.out.println("Welcome to Rock Paper Scissors!");

        while (true) {
            int random = getComputerAnswer();
            int player1 = getPlayer1Answer(kb);

            if (isRock(player1) && isRock(random)) {
                System.out.print("You have tied, computer rock, please try again\n");
            } else if (isPaper(player1) && isPaper(random)) {
                System.out.print("You have tied, computer chose paper, please try again");
            } else if (isScissor(player1) && isScissor(random)) {
                System.out.print("You have tied, computer chose scissors, please try again");
            }

            if (isRock(player1) && isPaper(random)) {
                System.out.print("Computer chose paper, computer has won");
                break;
            } else if (isPaper(player1) && isRock(random)) {
                System.out.print("You win!, computer chose rock");
                break;
            } else if (isPaper(player1) && isScissor(random)) {
                System.out.print("Computer chose scissors, computer has won");
                break;
            } else if (isScissor(player1) && isPaper(random)) {
                System.out.print("You win, computer chose paper");
                break;
            } else if (isRock(player1) && isScissor(random)) {
                System.out.print("You win!, computer chose scissors");
                break;
            } else if (isScissor(player1) && isRock(random)) {
                System.out.print("Computer chose rock, computer has won");
                break;
            }
        }
    }

    private static boolean isRock(int answer) {
        return answer == ROCK;
    }

    private static boolean isPaper(int answer) {
        return answer == PAPER;
    }

    private static boolean isScissor(int answer) {
        return answer == SCISSORS;
    }

    private static int getComputerAnswer() {
        int[] conditions = {ROCK, PAPER, SCISSORS};
        return conditions[new Random().nextInt(conditions.length)];
    }

    private static int getPlayer1Answer(Scanner scanner) {
        int value;

        while(true) {
            System.out.print("Please Enter: \n1 for Rock, \n2 for Paper, \n3 for Scissors: ");
            value = scanner.nextInt();

            if(isRock(value) || isPaper(value) || isScissor(value)) {
                break;
            } else {
                System.out.println("Invalid response");
            }
        }

        return value;
    }
}
```
\$\endgroup\$

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.