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();
 }
 }
 }
}

• Imus: Looks decent enough for a simple toy program. Too small to make any useful suggestions other than "you're doing the formatting correctly" (like which letters upper/lower case, brackets on same line, ...). Everything static is a bit non-java like though. But using instances only starts making sense if your entire program doesn't fit into a single class file anymore.
• Jakub: This is what I was asking for. Thank you. Now I just need to carry on that way, learn more Java and improve more.

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

• Jakub: Thank you for review. One thing, I don't translate it from one language to another. They are the same exercises but I don't even open python or cpp file. Doing it from the scratch every time. Agree with your point about extending it.. but I am not going to extend this ever. My question was about conventions and syntax, not about the logic.
• Mark Bluemel: I didn't say that you were translating from one language to another. However, I see what you're doing as simply treating Java as just another procedural language, using the same sort of thought processes that you probably used for C++ or Python and not actually getting any understanding of what makes Java what it is, and makes it different from C++ and Python.
• Jakub: I think you just confusing things. A simple terminal game does not need all of that stuff, especially if it was created just to get an opinion about code formatting etc. You can't say "another procedural language" as Python is object orientated and C++ really is more of a hybrid (can do, but don't have to at all). In all fairness, this conversation is pointless as I didn't ask for an opinion about how the program is written, I've asked for an opinion about style, syntax etc, not logic. As to the logic, the simples solutions that work are the best anyway.
• Timothy Truckle: @Jakub "A simple terminal game does not need all of that stuff, especially if it was created just to get an opinion about code formatting etc." -- That's true, but this is (as you stated) not just a simple terminal program but an exercise in a new programming language. And what Mark is writing about is really the style of how programs should be written in an OO language. Formatting and naming does not make the difference between languages.
• Jakub: What Mark said is: "you appear to be making the minimal effort to translate a procedural approach to this problem into a variety of computer language syntaxes." and that is not true. Considering I wrote this on the first day of learning java and all I wanted is to know is if my formatting etc is correct. And yes it is an exercise.. exercise for learning syntax, because unless you just start learning you would not consider this as a challenge. When I get a bit better in Java, then I will drop another program and you will tell me what you think about it ;) But first I have some learning to do.

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.

• Jakub: Thank you for your review. Will implement them all (advice) in my next code.

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;
 }
 }

• Jakub: Should of said, class is not public only because name of my file it's different than a name of a class. Comes up with some error, public class should be in its own file.
• Imus: @Jakub Java does require the class name and file name to be the same. In this case you should rename the file to match the classname. If for some reason the filename is something specific (only for school assignments for example) you have to name the class the same as the file.
• Jakub: Yes I know, the reason that one isn't it's because it is in my learning folder. So I have a set of exercises and programs and every time I learn new thing I am doing them all again. So I have this files named the same but with .py .cpp extensions and now with .java too.