Rock, Paper, Scissors Terminal Game in Java

Question

Recently started to learn Java.

I have created a few little things in Java already, but before I will start doing anything more serious I would like to know your opinion about my style.

Any conventions I don't follow? (trying to follow all of them, but I don't know everything yet).

Anything that doesn't look right?

I know it's not much here to show, but any advice and or opinion will be much appreciated. I am just trying to catch and stop bad habits at the early stage.

import java.util.Random;
import java.util.Scanner;

class RockPaperScissors {
    static int wins = 0;
    static int losses = 0;
    static int ties = 0;

    public static String getUserInput() {
        System.out.print(">> ");
        Scanner sc_input = new Scanner(System.in);
        String input = sc_input.nextLine();
        return input;
    }

    public static void getResult(String player, String computer) {
        if (player.equals(computer)) {
            System.out.println("It's a tie.");
            ties += 1;
        } else if (player.toLowerCase().equals("r") && computer.equals("s")) {
            System.out.println("You win.");
            wins += 1;
        } else if (player.toLowerCase().equals("p") && computer.equals("r")) {
            System.out.println("You win.");
            wins += 1;
        } else if (player.toLowerCase().equals("s") && computer.equals("p")) {
            System.out.println("You win.");
            wins += 1;
        } else {
            System.out.println("You loose.");
            losses += 1;
        }
    }

    public static String getComputerMove() {
        String[] hand = { "r", "p", "s" };
        Random r = new Random();
        int random_index = r.nextInt(hand.length);
        return hand[random_index];
    }

    public static void printChoices(String usr_choice, String cmp_choice) {
        if (usr_choice.equals("s")) {
            System.out.println("SCISSORS VS... ");
        } else if (usr_choice.equals("p")) {
            System.out.println("PAPER VS... ");
        } else if (usr_choice.equals("r")) {
            System.out.println("ROCK VS... ");
        } else {
            System.out.println("Are you trying to cheat?");
        }
        if (cmp_choice.equals("r")) {
            System.out.println("ROCK");
        } else if (cmp_choice.equals("s")) {
            System.out.println("SCISSORS");
        } else if (cmp_choice.equals("p")) {
            System.out.println("PAPER");
        }
    }
    
    public static void main(String[] args) {
        System.out.println("Enter your move: (r)ock, (p)aper, (s)cissors or (q)uit.");
        boolean run = true;
        while (run == true) {
            System.out.println(wins + " Wins. " + losses + " Losses. " + ties + " Ties. ");
            String usr_choice = getUserInput().toLowerCase();
            String cmp_choice = getComputerMove();
            if (usr_choice.equals("q")) {
                run = false;
            }
            if (run == true) {
                printChoices(usr_choice, cmp_choice);
                getResult(usr_choice, cmp_choice);
                System.out.println();
            }
        }
    }
}

Answer 1

Just a few arbitrary points.

• The arguments to getResults are poorly named. They don't represent a player and a computer, they represent the choices made by those entities.
• You use Strings to represent single characters. Not necessary or really sensible.

More significantly, this is barely Java in any meaningful sense. From your comment about .py and .cpp, you appear to be making the minimal effort to translate a procedural approach to this problem into a variety of computer language syntaxes.

Your approach scales poorly to additional conditions - consider how you would extend it to cover the famous "Rock, Paper, Scissors, Lizard, Spock" variant...

The point of Java is that it is object-oriented. You could consider having objects to represent the choices. Each instance of a Choice object would know its type (Rock, Paper, etc) together with the Choices it can defeat and how (Rock defeats Paper by wrapping, etc.). Player and computer Choices can then decide who wins.

See (for example) the discussions here

Answer 2

Thanks for sharing your code.

I am just trying to catch and stop bad habits at the early stage.

General approach

Your code is a procedural approach to the problem.

There is nothing wrong with procedural approaches in general, but Java is an object oriented (OO) programming language and if you want to become a good Java programmer then you should start solving problems in an OO way.

But OOP does not mean that you split up your code into random classes.

The ultimate goal of OOP is to reduce code duplication, improve readability and support reuse as well as extending the code.

Doing OOP means that you follow certain principles which are (among others):

• information hiding / encapsulation
• single responsibility
• separation of concerns
• KISS (Keep it simple (and) stupid.)
• DRY (Don't repeat yourself.)
• "Tell! Don't ask."
• Law of Demeter ("Don't talk to strangers!")
• replace branching with polymorphism

A first step would be to actually create an Object of your class and get rid of all the static keywords. The only method in your entire Project that really needs to be static is the main() method, which is called when you start your program from the command line.

_{Of cause all the code in this method should move to a new member method, that would be called from here...}

Always use the static keyword with care having a good reason to do so.

Example for an OOish approach

In the Method printChoices() you have an if/else cascade. That could be simplified if you'd hold the "translations" from the user/computer input to the display strings in a map like this:

private void printChoices(String userChoice, String computerChoice) {
    Map<String,String> translations = new HasMap<>();
    translations.put("s","SCISSORS");
    translations.put("r","ROCK");
    translations.put("p","PAPER");
   
    if(!translations.containsKey(userChoice)) {
        System.out.println("Are you trying to cheat?");
    } else {
        System.out.print(translations.get(userChoice));
        System.out.println(" VS ");
        System.out.print(translations.get(computerChoice));
    }
}

At least in my view this reads much nicer. ;o)
And after all you do something similar in your method getComputerMove() already, only based on an array instead of a collection...

Naming

Finding good names is the hardest part in programming. So always take your time to think about your identifier names. Remember that they are the best way to communicate your intentions to a complete stranger reading your code, and that this complete stranger is you in five months...

Conventions

Please read (and follow) the Java Naming Conventions (http://www.oracle.com/technetwork/java/codeconventions-135099.html).

Method names

Especially you mess with the method names. In java get as the first syllable of a method name is reserved for getter methods that do nothing else then returning the value of a member variable.

Variable names

In Java we rarely use the underscore (_) in identifier names. We use camel case names instead.

Also avoid single letter variable names or abbreviations. There is no penalty for long names. Its always better to be a bit "wordy" then being to short leaving room for interpretations. But beware of writing "novels" on the other hand...

Variables holding (and methods returning a) boolean should start with is, has, can or alike so that they read nice when used:

while(isRunning)
if(object.hasAbility())

Beside that variable names start with a noun or an adjective while method names start with a verb. Words that can be either one (like sum *or level) use with care.

Best practices

Visibility

Reduce visibility of methods and member/class variables as much as possible. Your class should have approximately one public method, the one that is meant to be called by other classes.
In your example this is the main() method only.

Member variables with visibility other then private usually indicate a violation of the most important OOP pattern: information hiding.

About if and else

In your main you have:

    while (run == true) {
        // ...
        if (usr_choice.equals("q")) {
            run = false;  
        }
        if (run == true) {
            printChoices(usr_choice, cmp_choice);
            getResult(usr_choice, cmp_choice);
            System.out.println();
        }

This has two issues:

1. Do not compare boolean variables to true. Give the variable a better name instead:

          while (isRunning) {
              // ...
              if (isRunning) {
   

   In case you want to check if it is false use the negation operator (!).

          while (!isStopped) {
              // ...
              if (!isRunning) {
   

   Don't use negation in names like isNotRunning

2. At this the second if the variable run can never hold false unless the the condition of previous if was true. Therefore you implicitly checking the opposite condition of the previous if here.

   Consequently this could (and should) be the else block of the previous if:

              if (userChoice.equals("q")) {
                  run = false;  
              } else {
                  printChoices(usr_choice, cmp_choice);
                  getResult(usr_choice, cmp_choice);
                  System.out.println();
              }
   

Variable scopes

Some of your variables have wrong sope.

The scope of a variable depends on some criteria:

• will this variable be modified by more than one method?
  -> member variable

• will this variable accessed by more than one method but never modified?
  -> final member variable

• will this variable be accessed by more that one instance and provide the same value to all?
  -> final static member variable (aka Constant)

In your code Random r, Scanner sc_input String[] hand and Map<String,String> translations (introduced by me earlier) might better be constants.

Answer 3

Try to use switch case and default to compare user's choice and random choices, the code would look much cleaner. Also make the class public if possible since "Error - At least one public class is required in main file java" might occur

    //example switch case
    public static void printChoices(String usr_choice, String cmp_choice) {
        switch (usr_choice) {
            case "s":
                System.out.println("SCISSORS VS... ");
                break;
            case "p":
                System.out.println("PAPER VS... ");
                break;
            case "r":
                System.out.println("ROCK VS... ");
                break;
            default:
                System.out.println("ERROR INPUT VS... ");
                break;
        }
    }