12
\$\begingroup\$

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
            }
        }    
    }
}
\$\endgroup\$
  • \$\begingroup\$ Looks good could be shortened but if you are just starting and learning this is fine. \$\endgroup\$ – JonH May 3 '12 at 18:57
  • \$\begingroup\$ truth - didn't know that existed. I'll check that out next time. JonH - Still learning, yeah :) Just for curiosities sake, how would you shorten it? Thanks! \$\endgroup\$ – Numpty May 3 '12 at 19:00
  • \$\begingroup\$ @Numpty - a lot of if conditions could be shortened up, also make use of switch statements. \$\endgroup\$ – JonH May 3 '12 at 19:01
  • 1
    \$\begingroup\$ Remember Java Coding Conventions for class nameing... and the while has a boolean condition for continuation, think, what will be a shorter booleaan expresion than equals() ? \$\endgroup\$ – PeterMmm May 3 '12 at 19:03
  • \$\begingroup\$ Use else if for tests against hChoice. Your way makes unnecessary checks and is more error-prone in general. \$\endgroup\$ – Marko Topolnik May 3 '12 at 19:17
8
\$\begingroup\$

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++;
    }
  }
}
\$\endgroup\$
  • \$\begingroup\$ Hah, I have some learnin' to do apparently. That's significantly shorter (and easier to follow). Neat :) \$\endgroup\$ – Numpty May 3 '12 at 20:12
4
\$\begingroup\$

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.

\$\endgroup\$
4
\$\begingroup\$

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).

\$\endgroup\$
  • \$\begingroup\$ Thanks for taking the time to go over all of that. Also, nice RPSLS reference :) \$\endgroup\$ – Numpty May 3 '12 at 22:07
3
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ Yeah, that seems like a better way (per Strawberry). I've fixed the class name :) \$\endgroup\$ – Numpty May 3 '12 at 19:13
  • \$\begingroup\$ re: your edits - I've never used that before. Need to do some more reading. Thanks again \$\endgroup\$ – Numpty May 3 '12 at 19:32
1
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ I figured it was pretty evident, and I've also got: int cChoice; // variable for computers choice (R/P/S) int pChoice = 0; // Holds converted choice (R=1, P=2, S=3) At the top, is that not sufficient? \$\endgroup\$ – Numpty May 3 '12 at 19:07
  • \$\begingroup\$ Comments are fine but good variable names are important too \$\endgroup\$ – cvanes May 3 '12 at 19:14
  • 1
    \$\begingroup\$ @Numpty names like cChoice force you to think or check the comment, introducing a tiny mental burden. These burdens add up, making code harder to understand. For instance, hInput confuses me - I couldn't figure out what the h stands for... furthermore cChoice may confuse some people into thinking it is a char (type information used to be encoded into the first characters of a variable name in the past). It's worth taking the time to choose good names. \$\endgroup\$ – Adam May 3 '12 at 20:21
1
\$\begingroup\$

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.

\$\endgroup\$

Your Answer

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

Not the answer you're looking for? Browse other questions tagged or ask your own question.