How to change my code to make it more efficient?

Question

in this code is playing Rock Paper Scissors, between the computer and the user. My code is all working great, however, I'm trying to think of a better way to make it ask the user if they would want to play again. If yes, then it would start the program again, if no, then it would stop. My "yes" seems to work but the no will stop and not go through all the way. Any suggestions or tips on how to do this? I will trying to incorporate a different while loop, but wasn't working. Would a do loop be good for this scenario? Thanks!

//import scanner
import java.util.Scanner;
import java.util.*;

//declare variables and main methods
class Rock {
  Scanner scan = new Scanner(System.in);
  Random generator = new Random();
  String response, name;
  char choice;
  int rounds, computerChoice, userScore, computerScore;
  boolean playIntro = true;
  boolean playGame = true;

  //this method will run the entire progrma
  public void playRPS(){
  
    //while loop for beginning of game
    while(playIntro){
      System.out.println("This is a game of Rock Paper Scissors!");
      System.out.println("Please enter your name: ");
      name = scan.nextLine();

      //while loop for the actual part of the game
      while(playGame){
        System.out.println("Type R (Rock), P (Paper), or S (Scissors): ");
        choice = scan.nextLine().charAt(0);
        computerChoice = generator.nextInt(3)+1;

        //using switch and case for each choice
        switch (choice){
          //case for Rock
          case 'R':
            if(computerChoice==1){
             System.out.println("Tie between you and the computer! Go again.");
             break;
            }
            else{
              if(computerChoice==2){
                System.out.println("The computer beat you this round");
                computerScore++;
                break;
              }
              else{
                System.out.println("You won this round");
                userScore++;
                break;
              }
            }
          //case for Paper
          case 'P':
            if(computerChoice==2){
             System.out.println("Tie between you and the computer! Go again.");
             break;
            }
            else{
              if(computerChoice==3){
                System.out.println("The computer beat you this round");
                computerScore++;
                break;
              }
              else{
                System.out.println("You won this round");
                userScore++;
                break;
              }
            }
          //case for Scissors
          case 'S':
            if(computerChoice==3){
             System.out.println("Tie between you and the computer! Go again.");
             break;
            }
            else{
              if(computerChoice==1){
                System.out.println("The computer beat you this round");
                computerScore++;
                break;
              }
              else{
                System.out.println("You won this round");
                userScore++;
                break;
              }
            }
        }
        System.out.println("You have "+userScore+" points and the computer has "+computerScore+" points");
        if (userScore==5){
          System.out.println("\nOut of 5 rounds, You beat the computer!");
          playGame = false;
        }
        else if (computerScore==5){
          System.out.println("\nOut of 5 rounds, The computer beat you.");
          playGame = false;
        }
      }
      askUser();
    }  
  }

  public void askUser(){

    System.out.println("\nDo you want to play this Rock Paper Scissors again? Type yes: ");
    response = scan.nextLine();
    if (response.equalsIgnoreCase("yes")){
      playGame = true;
      userScore=0;
      computerScore=0;
    }
    else{
      playGame = false;
      scan.nextLine();
    }
  }

  public static void main() {
    Rock prog = new Rock();
    prog.playRPS();
  }
}

Answer 1

I wouldn't say this is necessarily more efficient or even better but it is a little more concise. It's major elements are.

• use Lambdas to decide the winner based on the chosen move.
• use a map to call the proper Lambda based on the user's move. The lambda then evaluates the two moves to decide the outcome.
• for simplicity, moves are selected by number

Of course, the important thing is that your code works.

import java.util.List;
import java.util.Map;
import java.util.Random;
import java.util.Scanner;
import java.util.Set;
import java.util.function.Function;

public class RockPaperScissors {
    
    final static int PAPER = 1;
    final static int ROCK = 2;
    final static int SCISSORS = 3;
    
    // the next two declarations allows the previous three to take on any values.
    private Set<Integer> allowedMoves = Set.of(PAPER, ROCK, SCISSORS);
    private List<String> moves = List.of("PAPER", "ROCK", "SCISSORS");
    private String ROCK_WINS_MSG = "Rock crushes scissors";
    private String SCISSORS_WINS_MSG = "Scissors cuts paper";
    private String PAPER_WINS_MSG = "Paper covers rock";
    private String COMPUTER_WINS = ", computer wins!";
    private String YOU_WIN = ", you win!";
    
    private Function<Integer, String> CHECK_PAPER =
            (c) -> c == PAPER ? "It's a tie!" :
                    c == ROCK ? PAPER_WINS_MSG + YOU_WIN :
                    SCISSORS_WINS_MSG + COMPUTER_WINS;
    private Function<Integer, String> CHECK_ROCK =
            (c) -> c == ROCK ? "It's a tie!" :
                    c == SCISSORS ? ROCK_WINS_MSG + YOU_WIN :
                    PAPER_WINS_MSG + COMPUTER_WINS;
    private Function<Integer, String> CHECK_SCISSORS =
            (c) -> c == SCISSORS ? "It's a tie!" :
                    c == PAPER ? SCISSORS_WINS_MSG + YOU_WIN :
                    ROCK_WINS_MSG + COMPUTER_WINS;
    
    private Map<Integer, Function<Integer, String>> evalUser =
            Map.of(PAPER, CHECK_PAPER, ROCK, CHECK_ROCK, SCISSORS,
                    CHECK_SCISSORS);
    
    public static void main(String[] args) {
        new RockPaperScissors().play();
    }
    
    public void play() {
        Random r = new Random();
        Scanner scan = new Scanner(System.in);
        
        while (true) {
            System.out.printf("%n%d : %s%n%d : %s%n%d : %s%n%s%n",
                    PAPER, "PAPER", ROCK, "ROCK", SCISSORS,
                    "SCISSORS", "Any other integer to quit.");
            System.out.print("Your move! ");
            String str = scan.nextLine();
            int move;
            try {
                move = Integer.parseInt(str);
                if (!allowedMoves.contains(move)) {
                    break;
                }
            } catch (IllegalArgumentException ie) {
                System.out.println("Only integers permitted.");
                continue;
            }
            System.out.println("\nYou chose " + moves.get(move - 1));
            int cmove = r.nextInt(3);
            
            System.out.println(
                    "The computer chooses " + moves.get(cmove));
            System.out.println(evalUser.get(move).apply(cmove + 1));
            
        }
        System.out.println("\nGame over!");
        
    }
}

Once suggestion for your code would be to look at the switch cases. The code for each case is practically identical. I would look for similarities and make the evaluation a single method (something I didn't really do in my code). Then in each case, call that method with the appropriate arguments. One such argument would be either "computer" or "you" based on the context.

Answer 2

There's nothing that ever sets playIntro false, and therefore the outer loop will never terminate.

When askUser() sets playGame false the inner loop terminates, and you fall into the outer loop, which keeps on looping.

I don't see any reason for the outer loop to exist at all. You only want to print the introduction and ask the player's name once.

This isn't so much a matter of 'efficiency' as of correctness.

---

Incidentally, it would be better to make askUser() return a true/false value rather than set a member variable. Then you can use it directly in a 'while' expression.

The overall structure of playRPS() then looks like:

public void playRPS() {
   ... print intro, ask name ...
   do {
       ... play one game ...
   } while (askUser());
}