Does my Rock Paper Scissors game look good?

Question

I'm doing some homework and am just curious if everything I've done looks good and/or if you'd suggest I modify something to keep with "javaese."

import java.util.Scanner;
import java.util.Random;
public class rockpaperscissors 
{
    public static void main (String[] args) 
    {    
        int cChoice; // variable for computers choice (R/P/S)
        int pChoice = 0; // Holds converted choice (R=1, P=2, S=3)
        int cScore = 0, pScore = 0, tie = 0, rounds = 0; // Initialised variables for score keeping (c = computer, p = player) plus keeps track of number of rounds played
        String loop="yes"; // Starts our loop
        Scanner input = new Scanner(System.in); // Creates scanner object
        Random rgen = new Random();
        System.out.println("Hello, for this exercise we're going to be playing everyone's favourite game, Rock-Paper-Scissors!");

        while (loop.equals("yes")) // This loop keeps our game going only while our string.loop is equal to 'yes'
        { 
            cChoice=rgen.nextInt(3)+1;
            System.out.println("Please make your selection: R - Rock. P - Paper. S - Scissors");
            String hInput = input.nextLine();  // variable for players choice (R/P/S) 
            String hChoice = hInput.toUpperCase(); //Converts to Upper case in case user did not
            if (hChoice.equals("R") || hChoice.equals("P") || hChoice.equals("S")) // Ensures player has entered the correct choice for the game to continue
            {
                System.out.println("");

                if (hChoice.equals("R"))  // This converts pChoice to the numeric value for "Rock"
                {
                    pChoice = 1;
                }
                if (hChoice.equals("P"))  // This converts pChoice to the numeric value for "Rock"
                {
                    pChoice = 2;
                }
                if (hChoice.equals("S"))  // This converts pChoice to the numeric value for "Rock"
                {
                    pChoice = 3;
                }

                if (pChoice == cChoice) // Takes care of Ties
                { 
                    System.out.println("Tie Game!");
                    System.out.println("");
                    tie++;
                    rounds++;
                } else      // Accounts for scoring for non-tie scenarios
                {
                    if (cChoice==1 && pChoice==3) // Computer picks Rock and player has Scissors - adds point to score and rounds
                    {
                        System.out.println("Computer picked Rock!");
                        System.out.println("Rock beats Scissors!");
                        System.out.println("**Computer Wins!**");
                        System.out.println("");
                        cScore++;
                        rounds++;
                    } 

                    if (cChoice==1 && pChoice==2) // Computer picks Rock and player has Paper - adds point to score and rounds
                    {
                        System.out.println("Computer picked Rock!");
                        System.out.println("Paper beats Rock!");
                        System.out.println("**Player Wins!**");
                        System.out.println("");
                        pScore++;
                        rounds++;
                    } 

                    if (cChoice==2 && pChoice==3) // Computer picks Paper and player has Scissors - adds point to score and rounds
                    {
                        System.out.println("Computer picked Paper!");
                        System.out.println("Scissors beats Paper!");
                        System.out.println("**Player Wins!**");
                        System.out.println("");
                        pScore++;
                        rounds++;
                    } 

                    if (cChoice==2 && pChoice==1) // Computer picks Paper and player has Rock - adds point to score and rounds
                    {
                        System.out.println("Computer picked Paper!");
                        System.out.println("Paper beats Rock!");
                        System.out.println("**Computer Wins!**");
                        System.out.println("");
                        cScore++;
                        rounds++;
                    } 

                    if (cChoice==3 && pChoice==1)  // Computer picks Scissors and player has Rock - adds point to score and rounds
                    {
                        System.out.println("Computer picked Scissors!");
                        System.out.println("Rock beats Scissors!");
                        System.out.println("**Player Wins!**");
                        System.out.println("");
                        pScore++;
                        rounds++;
                    } 

                    if (cChoice==3 && pChoice==2) // Computer picks Scissors and player has Paper - adds point to score and rounds
                    {
                        System.out.println("Computer picked Scissors!");
                        System.out.println("Scissors beats Paper!");
                        System.out.println("**Computer Wins!**");
                        System.out.println("");
                        cScore++;
                        rounds++;
                    } 
                }                            
            } else  // end the game
            {
                System.out.println ("Sorry, you didn't pick Rock, Paper, or Scissors. The game will end now.");
                System.out.println ("Here are the final scores after " + rounds +" rounds:");
                System.out.println ("You\tComputer\tTies");
                System.out.println (" "+ pScore +"\t   " + cScore + "\t\t " + tie);
                loop = "no"; // terminates the while loop keeping the game going
            }
        }    
    }
}

Answer 1

For your perusal, I took the liberty to clean up and simplify your code a bit.

public class RockPaperScissors
{
  static int cScore, pScore, tie, rounds;

  enum RPS {
    R("Rock", "S"), P("Paper", "R"), S("Scissors", "P");
    private final String beats, name;
    RPS(String name, String beats) { this.beats = beats; this.name = name; }
    int compare(RPS other) { return other == this? 0 : other == valueOf(beats)? 1 : -1; }
    String fullName() { return name; }
  }

  public static void main (String[] args) {
    final Scanner input = new Scanner(System.in);
    final Random rgen = new Random();
    System.out.println("Hello, for this exercise we're going to be playing everyone's favourite game, Rock-Paper-Scissors!");
    while (true) {
      final RPS playerChoice, compChoice = RPS.values()[rgen.nextInt(3)];
      System.out.println("Please make your selection: R - Rock. P - Paper. S - Scissors");
      try { playerChoice = RPS.valueOf(input.nextLine().toUpperCase()); }
      catch (Exception e) {
        System.out.println(
            "Sorry, you didn't pick Rock, Paper, or Scissors. The game will end now.\n" +
            "Here are the final scores after " + rounds +" rounds:\nYou\tComputer\tTies\n" +
            " "+ pScore +"\t   " + cScore + "\t\t " + tie);
        return;
      }
      System.out.println("\nComputer picked " + compChoice.fullName() + "!");
      switch (playerChoice.compare(compChoice)) {
      case 0:
        System.out.println("Tie Game!\n");
        tie++;
        break;
      case -1:
        System.out.println(compChoice.fullName() + " beats " + playerChoice.fullName() + "!");
        System.out.println("**Computer Wins!**\n");
        cScore++;
        break;
      case 1:
        System.out.println(playerChoice.fullName() + " beats " + compChoice.fullName() + "!");
        System.out.println("**Player Wins!**\n");
        pScore++;
      }
      rounds++;
    }
  }
}

Answer 2

Once you have working code, it is time to refactor.

For example, once the computer makes a choice, print "Computer picked %s" to announce the choie. This is better than repeating the same code in multiple sections (because it makes the code shorter, easier-to-understand, easier-to-test, and easier-to-maintain).

Also, consider precomputing the comparison logic into a HashMap or a 3-by-3 table.

Answer 3

To determine the outcome of a round, you have logic that depends on values. The "logic" basically checks all of the values and then determines the outcome.

Such a strategy (while effective immediately) is a fragile one over time, any change in the data demands searching and updating any blocks of code that decide based on values of the data. So encapsulate the logic.

// this could be a class, abstract class, or enum, it doesn't need to be an interface
// but Choices should probably know if they beat other Choices.
public interface Choice {

  public boolean beats(Choice other) {
    ...
  }

}

So many people have commented on Choice that I'll just avoid adding extra comments past this; however, I do want to talk about the Score which is another "thing" that exists in your program, yet there is no corresponding object.

public class Score {

  private int games;

  private int wins;

  public Score() {
    games = 0;
    wins = 0;
  }

  public void update(Choice human, Choice computer) {
    games++;
    if (human.beats(computer)) {
      wins++;
    }
  }

}

reduces your "in the loop code to"

Score score = new Score();
while (playing) {
  Choice human = getChoice();
  Choice computer = Choices.getRandomChoice();
  score.update(human, computer);
  playing = getContinue();
}

revisting the intial complaint

To determine the outcome of a round, you have logic that depends on values. The "logic" basically checks all of the values and then determines the outcome.

You can see that the new structure (which doesn't functionally do anything differently than the old structure) lacks a top-level logic that checks based on the low level data. Choices now know if they beat each other, and Scores basically update themselves based on asking the Choices.

In the event that you needed to make a Rock-Paper-Scissor derivative, like Rock-Paper-Scissors-Lizard-Spock, you could simply modify the Choices and not need to touch how scoring works (or go searching through all of your code to see if it impacted external logic).

Answer 4

You're using the variable 'loop' which has a type of String as a boolean, so just make it a boolean.

By convention, class names should be uppercase.

You have the following comment:

// This converts pChoice to the numeric value for "Rock"

Remember, code is written for programmers to read, and anyone that knows how to read code understands what that code does; so try not to document the what, aim to document the why. Also you've repeated it erroneously two additional times.

You convert the input of 'R', 'P', or 'S' to ints: There's no reason to do that you could just compare the strings. A better way to do this all together would probably be to use an enum to represent player choices.

   public enum Choices {
        Rock("R"),
        Paper("P"),
        Scissors("S");

     // ....
    }

This opens up some other options for cleaning up your comparison cases further along in the program.

Answer 5

Naming can make code much more readable e.g if you use computerChoice and playerChoice rather than cChoice and pChoice, it's immediately more obvious what the variables are supposed to contain.

Answer 6

Refactoring parts into meaningful methods is also worthwhile, for example the line:-

if (hChoice.equals("R") || hChoice.equals("P") || hChoice.equals("S")) // Ensures player has entered the correct choice for the game to continue

Could be refactored to:-

if (isValidChoice(hChoice))
{
...            
}

private boolean isValidChoice(String choice)
{
    return choice.equals("R") || choice.equals("P") || choice.equals("S");
}

This makes the code more readable.

This type of refactoring is outlined in Clean Code, which incidentally is an excellent book.