12
\$\begingroup\$

I'm new to programming and have created a simple Rock, Paper, Scissors game. The entire program is under a single class and the main method. I hear that's probably not the best way to code.

How should I have written this to utilize multiple classes or to adhere to best practice?

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

public class RPS {
    public static void main(String args[]){

    String choice, choice1;
    int num = 0;
    Random r = new Random();
    num = r.nextInt(3);

    System.out.println("Choose: Rock, Paper or Scissors.");
    Scanner userChoice = new Scanner(System.in);
    choice1 = userChoice.next();
    choice = choice1.toLowerCase();



    switch (num){
    case 0:
        String num1 = "rock";
        System.out.println("System chose Rock"); 
        if (choice.matches(num1)){
            System.out.println("Its a tie!");

            }
        else if (choice.matches("paper")){
            System.out.println("You win!");

        }
        else if (choice.matches("scissors")){
            System.out.println("You lose!");
        }
        else {
            System.out.println();
            System.out.println("ERROR: Please choose Rock, Paper or Scissors");
        }
    break;
    case 1:
        String num2 = "paper";
        System.out.println("System chose Paper");
        if (choice.matches(num2)){
            System.out.println("Its a tie!");

        }
        else if (choice.matches("scissors")){
            System.out.println("You win!");

        }
        else if (choice.matches("rock")){
            System.out.println("You lose!");
        }
        else {
            System.out.println();
            System.out.println("ERROR: Please choose Rock, Paper or Scissors");
        }
    break;
    case 2: 
        String num3 = "scissors";
        System.out.println("System chose Scissors");
        if (choice.matches(num3)){
            System.out.println("Its a tie!");

        }
        else if (choice.matches("rock")){
            System.out.println("You win!");

        }
        else if (choice.matches("paper")){
            System.out.println("You lose!");
        }
        else {
            System.out.println();
            System.out.println("ERROR: Please choose Rock, Paper or Scissors");
        }
    break;


    }

}
}
\$\endgroup\$
2
  • \$\begingroup\$ See Rock-Paper-Scissors here for nice example of how to use an Enum to encapsulate the winning/losing logic. \$\endgroup\$ May 8 '13 at 16:44
  • 6
    \$\begingroup\$ For trivial programs such as your average CS class assignment, it's not as evil as we generally make it out to be. However, two things are central to good code design at any level: "DRY" (Don't Repeat Yourself; make loops or extract methods when you see the same thing being done many times), and "self-documenting code"; it should be obvious to any other competent programmer what your code is trying to do (meaning you should name classes, functions and variables descriptively, try to use I/O values that do the same, etc). \$\endgroup\$
    – KeithS
    May 8 '13 at 22:36
7
\$\begingroup\$

I would make some changes, although your code is quite simple.

First, get rid of the magic Strings for rock/paper/scissors, and create an enum.

I would possibly also move the logic of checking if it's a win/draw/loose inside that enum, so it would be much easier to add new entries (imagine you want to extend that to a Rock/Paper/Scissors/Lizard/Spock.

My main recomendation is to separate getting the input from the user, and the logic of the actual program.

If these two concepts are completely separated, you could reuse the logic to build, for instance, a web interface, or get the user input in any other way (or even get two inputs from two users, and use the same logic to decide who wins!).

Check this, for instance, for an enum representation of Rock/Paper/Scissors:

https://stackoverflow.com/a/9858163/432806

Overall I would advise you to have a structure like:

main:
    input = getUserInput
    enumValue = getEnumFromInput
    secondValue = getEnumFromComputerRoll
    testWhoWins(enumValue, secondValue) 

That last line could also be something like:

    enumValue.getResult(secondValue)

If you implement the logic inside the enum.

update

There's also another big downside to that monolithic approach, that hasn't been discussed yet: testing.

There is no simple way of writing unit tests for a program written in such a fashion.

To be able to have decent unit tests, you need to break up your programming logic into small pieces that can be tested independently from each other.

Think about:

  • How would you test transforming a String (user input), to the correct Enum/String?
  • How would you test generating a value in the right range, and converting into an Enum?
  • How would you test the logic of, given two "moves", choosing the result?

I think this is a good way of figuring out how to break up your logic: Think about how you would write tests for it.

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

There is nothing inherently wrong about writing monolithic programs. There are lots of downsides, thought, most of them related to maintainability.

Your particular example can be refactored to make it more compact and probably easier to the eye. It can be also a good exercise on structures (here I'm using a String array of arrays, also known as a matrix.) For example:

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

public class RPS {
    public static void main(String args[]) {
        // Arrays of moves: move name, beaten by 
        String[][] moves = { { "rock", "paper", },
                { "scissors", "rock", },
                { "paper", "scissors", },
        };
        String choice;
        int num = 0;
        Random r = new Random();
        num = r.nextInt(3);

        System.out.print("Choose: ");
        for (int i = 0; i < moves.length; i++ ) {
            if (i > 0)
                System.out.print(", ");             
            System.out.print(moves[i][0]);
        }
        System.out.println(".");
        Scanner userChoice = new Scanner(System.in);
        choice = userChoice.next().toLowerCase();

        System.out.println("System chose " + moves[num][0]);
        if (choice.matches(moves[num][0])) {
            System.out.println("Its a tie!");
        } else if (choice.matches(moves[num][1])) {
            System.out.println("You win!");
        } else {
            System.out.println("You lose!");
        }
    }
}

A nice side effect of refactoring your code like this is that you can easily extend it. For example, two small modifications and you have the game Sheldon plays in A Big Bang Theory:

public class RPS {
    public static void main(String args[]) {
        // Arrays of moves: move name, beaten by, also beaten by
        String[][] moves = { { "rock", "paper", "spock" },
                { "scissors", "rock", "spock" },
                { "paper", "scissors", "lizard" },
                { "lizard", "rock", "scissors" },
                { "spock", "lizard", "paper" }, };
        String choice;
        int num = 0;
        Random r = new Random();
        num = r.nextInt(moves.length);

        System.out.print("Choose: ");
        for (int i = 0; i < moves.length; i++) {
            if (i > 0)
                System.out.print(", ");
            System.out.print(moves[i][0]);
        }
        System.out.println(".");
        Scanner userChoice = new Scanner(System.in);
        choice = userChoice.next().toLowerCase();

        System.out.println("System chose " + moves[num][0]);
        if (choice.matches(moves[num][0])) {
            System.out.println("Its a tie!");
        } else if (choice.matches(moves[num][1])
                || choice.matches(moves[num][2])) {
            System.out.println("You win!");
        } else {
            System.out.println("You lose!");
        }
    }
}
\$\endgroup\$
3
  • \$\begingroup\$ In the second game shouldn't this be num = r.nextInt(3); 5 instead? \$\endgroup\$ May 8 '13 at 19:16
  • 1
    \$\begingroup\$ Actually, that should be r.nextInt(moves.length). Thanks for pointing it out. \$\endgroup\$ May 8 '13 at 19:59
  • \$\begingroup\$ Thanks for modifying my code, it'll make it easier for me to see what i need to do! \$\endgroup\$
    – Cody H
    May 9 '13 at 0:47

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.