11
\$\begingroup\$

I enjoyed the many headaches this project gave me as I learned a lot! I have been studying Java for less than a year and am new to programming in general. I read over the comments rules and hope I followed them correctly. I don't know many people that code and would love any feedback. This was a school project I worked very hard on, but I know I have a long way to go. Please, let me know if you can help me get better...

import java.util.Scanner;
    import java.util.Random;
    
    public class RPS {
        public static void main(String[] args) {
            Boolean playAgain = true;
            Scanner input = new Scanner(System.in);
            String play;
            
            while (playAgain = true) {      
            WelcomeP1(); //  Welcome & basic rule example
            
            // Final Result & Replay Option
            System.out.print("*********************" );
            System.out.print("-------------> You " +Game()+ "!!!" );
            System.out.println("\n*********************" );
            System.out.println("Play Again?!");
            System.out.println("Please press y for (Y)es or n for (N)o: ");
            play = input.nextLine().trim();
            while (play.isEmpty()){
                System.out.println("Do you want to play Rock, Paper, Scissors?");
                System.out.println("Please press y for (Y)es \nor n for (N)o: ");
                play = input
                        .nextLine().trim();
                } // end while ~ validate/catch error ~ for empty space
            switch ( play.charAt(0) ){
                    case 'n' : case 'N':
                        playAgain = false;
                        System.exit(0);
                    case 'y': case 'Y':
                        System.out.println("\n*********************" );
                        System.out.print("Excellent;\nGlad you enjoyed playing!"); 
                        System.out.println("\nJust checking again...");
              } // end while 
            } // end switch 
        } // end main
        
        
            // Welcome Validation Start
            public static void WelcomeP1(){
                Scanner stdIn= new Scanner(System.in);
                Boolean validGame = false;
                String userPlay;
                
                while( validGame == false ){
                System.out.println("Do you want to play Rock, Paper, Scissors?");
                System.out.println("Please press y for (Y)es \nor n for (N)o: ");
                userPlay = stdIn.nextLine().trim();
                while (userPlay.isEmpty()){
                    System.out.println("Do you want to play Rock, Paper, Scissors?");
                    System.out.println("Please press y for (Y)es \nor n for (N)o: ");
                    userPlay = stdIn.nextLine().trim();
                } // end while ~ validate/catch error ~ for empty space
                // below valiates user entry    
                switch ( userPlay.charAt(0) ){
                    case 'n' : case 'N':
                        System.exit(0);
                    case 'y': case 'Y':
                        validGame = true;
                        System.out.println("*************************************");
                        System.out.println("Excellent! Thank you...\nLet's Play!");
                        break;
                    default: 
                } // end switch
            } // end while loop 
            } // end class Welcome ~ End Welcome Validation
            
            
            // User Input Validation Start
            public static int P1Choice(){
                Scanner userIn = new Scanner(System.in);
                Boolean validUserIn = false;
                String userInput; 
                int userPiece = 0;
            
                while (validUserIn == false){
                    System.out.println("*************************************");
                    System.out.println("Choose (R)ock, (P)aper, or (S)cissors ");
                    System.out.println("Please type the first letter only.");
                    System.out.println("Your Choice: ");
                    userInput = userIn.nextLine();
                while (userInput.isEmpty()){
                    System.out.println("Choose (R)ock, (P)aper, or (S)cissors ");
                    System.out.println("Please type the first letter only.");
                    System.out.println("Your Choice: ");
                    userInput = userIn.nextLine();
                } // end while ~ validate/catch error ~ for empty space
                  // below valiates user entry further   
                    userPiece = userInput.charAt(0);
                if (userInput.equalsIgnoreCase("r")){
                    userPiece = 0;
                    System.out.println("You Chose: Rock");
                    validUserIn = true;
                }
                    else if (userInput.equalsIgnoreCase("p")){
                        userPiece = 1;
                        System.out.println("You Chose: Paper");
                        validUserIn = true;
                    }
                        else if (userInput.equalsIgnoreCase("s")) {
                            userPiece = 2;
                            System.out.println("You Chose: Scissors");
                            validUserIn = true;
                        }  
                        else {
                            System.out.println("Invalid Choice.");
                            validUserIn = false;
                        } 
                } // end while loop 
            return userPiece;
            } // end class UserPiece ~ End User Input Validation & Return
            
            
            // Computer Choice Calculation & Print ~ Start
            public static int ComputerChoice(){
            Random random = new Random();
            int choiceStart;
            choiceStart = random.nextInt(3); // max number 3 ~ 0-2
            int computerInput = 0;
            String computerPiece = null;
            
            switch (choiceStart) {
                case 0:
                    computerInput = 0; 
                    computerPiece = "Rock";
                    break;
                case 1:
                    computerInput = 1; //Paper
                    computerPiece = "Paper";
                    break;
                case 2:
                    computerInput = 2; //Scissors
                    computerPiece = "Scissors";
                    break;
            } // end switch
            System.out.println("*************************************");
            System.out.println("*************************************");
            System.out.println("The computer has decided as well...");
            System.out.println("|||||||||||||||||||||||||||||||||||||");
            System.out.println("|||||||||||||||||||||||||||||||||||||");
            System.out.println("The computer's choice was " +computerPiece+ "...");
            System.out.println("*************************************");
            System.out.println("*************************************");
            return computerInput;    
            } // end class ComputerChoice ~ End Computer Choice Calculation & Print
            
            
            // Calculate Game & Return Win/Loss/Tie ~ Calculations
            public static String Game(){
                int user = P1Choice();
                int computer = ComputerChoice();
                String error = "*** If you are reading this, there has been a"
                        +" system error. Please restart the program. ***";
                
                // 0 = rock
                // 1 = paper
                // 2 = scissors
                if (user == 0) {
                    if (computer != 1){
                        if (computer != 0){
                            return "Win";
                        }
                        return "Tie";
                    }
                    return "Lose";
                } // end first nested for loop logic
                if (user == 1){
                    if (computer != 2){
                        if (computer != 1) {
                            return "Win";
                        }
                        return "Tie";
                    }
                    return "Lose";       
                } // end second nested for loop logic
                if (user == 2){
                    if (computer != 0){
                        if (computer != 2) {
                            return "Win";
                        }
                        return "Tie";
                    }
                    return "Lose";       
                } // end third nested for loop logic
                return error; // null  -- Needed to return something 
                // -- better practice teacher?
            } // end class Game ~ End Calculate Game & Return     
    } // end class RPS

```
\$\endgroup\$

3 Answers 3

10
\$\begingroup\$

Code formatting

You have some formatting and indentation problems. Please run your code through a formatter, for example this one

https://www.tutorialspoint.com/online_java_formatter.htm

to make it more readable and follow standards.

Logic

while (playAgain = true) {

This is wrong, it's supposed to be == , not = . It works anyway since what you do is just set the playAgain variable to true in the first run, which is the same value as it already has, and since during the while loop it checks the truth value of playAgain.

while(playAgain) is equivalent to while(playAgain == true)

Comments

    } // end while 
} // end switch 

These comments are in the wrong order. The switch is inside the while , so it ends first. But you should not need these comments at all. If you use a good code editor and proper indentation, you will easily see (and get auto-highlight) on which bracket matches which. I suggest using an editor such as IntelliJ or Visual studio code.

Logic 2

The win/lose/tie code is very long and messy.

First, for ties you can just check

if (user == computer) {
    return "Tie";
}

That covers all combinations that create a tie. With those cases out of the way, the remaining logic gets simpler.

Since paper beats rock (1 beats 0) and scissors beats paper (2 beats 1), with your convenient number definitions you can then check user == computer + 1, but we also need the case for rock beats scissors.

if (user == computer + 1 || user == 0 && computer == 2) {
    return "Win";
}

Since we have now covered all cases of Tie or Win, any other case will lose

else {
    return "Lose";
}
\$\endgroup\$
8
\$\begingroup\$

Java naming convention

The method name should always start with a lower case.

  • WelcomeP1 -> welcomeP1
  • P1Choice -> p1Choice
  • ComputerChoice -> computerChoice
  • Game -> game

Use java.io.PrintStream#printf instead of java.io.PrintStream#println when you have to concatenate

java.io.PrintStream#printf offer you to use patterns to build the string without concatenating it manually. The only downside is you will be forced to add the break line character yourself; in java you can use the %n to break the line (portable between various platforms) or uses the traditional \n / \r\n.

Before

System.out.print("-------------> You " + game() + "!!!");

After

System.out.printf("-------------> You %s!!!",  game());

Before

System.out.println("The computer's choice was " + computerPiece + "...");

After

System.out.printf("The computer's choice was %s...%n", computerPiece); //With a new line

Always use the primitives when possible

When you know that it's impossible to get a null value with the number, try to use the primitives; this can prevent the unboxing of the value in some case.

In your code, you can replace the Boolean to boolean since you have only the true and false values.

Extract some of the logic to methods.

When you have logic that does the same thing, you can generally move it into a method and reuse it.

You can extract some of the logic that asks if the user wants to play Rock, Paper or Scissors into a method and reuses it.

private static String askIfWantToPlay(Scanner scanner) {
   String userPlay = "";
   while (userPlay.isEmpty()) {
      System.out.println("Do you want to play Rock, Paper, Scissors?");
      System.out.println("Please press y for (Y)es \nor n for (N)o: ");
      userPlay = scanner.next().trim();
   }
   return userPlay;
}

This method will remove some duplication.

If you want to, you could centralize the logic of exiting in this new method; check if the user want to exit or not.

private static void askIfWantToPlayAndExitOtherwise(Scanner scanner) {
   String userPlay = "";

   while (!"n".equalsIgnoreCase(userPlay) && !"y".equalsIgnoreCase(userPlay)) {
      System.out.println("Do you want to play Rock, Paper, Scissors?");
      System.out.println("Please press y for (Y)es \nor n for (N)o: ");
      userPlay = scanner.next();
   }

   if ("y".equalsIgnoreCase(userPlay)) {
      System.exit(0);
   } else {
      System.out.println("*************************************");
      System.out.println("Excellent! Thank you...\nLet's Play!");
   }
}

With this method, the code is easier to read and don’t propagate logic everywhere in the application; you can do the same thing with the user choice for the (R)ock, (P)aper, or (S)cissors and return only a valid choice.

\$\endgroup\$
5
\$\begingroup\$

I can agree with all of in two previous answers, however I have to address the issue of implementation of game logic itself. I just so happens that we understand the game logic of the game because we all know the game, but if we look at the code it is difficult to extract the game logic from the code. It is not written in clean, easy to understand way, especially with deep if statement nesting.

public static String Game(){
                int user = P1Choice();
                int computer = ComputerChoice();
                String error = "*** If you are reading this, there has been a"
                        +" system error. Please restart the program. ***";
                
                // 0 = rock
                // 1 = paper
                // 2 = scissors
                if (user == 0) {
                    if (computer != 1){
                        if (computer != 0){
                            return "Win";
                        }
                        return "Tie";
                    }
                    return "Lose";
                } // end first nested for loop logic
                if (user == 1){
                    if (computer != 2){
                        if (computer != 1) {
                            return "Win";
                        }
                        return "Tie";
                    }
                    return "Lose";       
                } // end second nested for loop logic
                if (user == 2){
                    if (computer != 0){
                        if (computer != 2) {
                            return "Win";
                        }
                        return "Tie";
                    }
                    return "Lose";       
                } // end third nested for loop logic
                return error; // null  -- Needed to return something 
                // -- better practice teacher?
            } // end class Game ~ End Calculate Game & Return     
    } // end class RPS

I found a way to implement this game logic in more clear and easy to read and understand way.

Use enumerations

Enumerations can help greatly with code clarity

public enum PlayedMove
{
    ROCK,
    PAPER,
    SCISSORS;
}

They are self-explanatory

public enum Winer
{
    DRAW,
    PLAYER,
    COMPUTER;
}

The new game logic

As if you are explaining it to a man and not a computer

public class GameLogic
{
    public static Winer getWiner(PlayedMove player, PlayedMove computer)
    {
        if (player == computer)
        {
            return Winer.DRAW;
        }
        else if (isPlayerWiner(player, computer))
        {
            return Winer.PLAYER;
        }
        else
        {
            return Winer.COMPUTER;
        }
    }
    
    private static boolean isPlayerWiner(PlayedMove player, PlayedMove computer)
    {
        return ((player == PlayedMove.ROCK && computer == PlayedMove.SCISSORS)
             || (player == PlayedMove.SCISSORS && computer == PlayedMove.PAPER)
             || (player == PlayedMove.PAPER && computer == PlayedMove.ROCK));
    }
}
\$\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.