8
\$\begingroup\$

There was a fairly strangely written RPSLS game on Stack Overflow here.

That inspired me (read: I was bored and figured it would be fun) to write a better one. But I'm not sure what else there is that could be done to make it prettier.

This is my latest version:

public class RockPaperScissors {
    public enum Choices {
        ROCK("rock") {
            @Override
            public List<Choices> getWinsAgainst() {
                if (winsAgainst.isEmpty()) {
                    winsAgainst.add(SCISSORS);
                    winsAgainst.add(LIZARD);
                }
                return winsAgainst;
            }
        },
        PAPER("paper") {
            @Override
            public List<Choices> getWinsAgainst() {
                if (winsAgainst.isEmpty()) {
                    winsAgainst.add(ROCK);
                    winsAgainst.add(SPOCK);
                }
                return winsAgainst;
            }
        },
        SCISSORS("scissors") {
            @Override
            public List<Choices> getWinsAgainst() {
                if (winsAgainst.isEmpty()) {
                    winsAgainst.add(PAPER);
                    winsAgainst.add(LIZARD);
                }
                return winsAgainst;
            }
        },
        LIZARD("lizard") {
            @Override
            public List<Choices> getWinsAgainst() {
                if (winsAgainst.isEmpty()) {
                    winsAgainst.add(SPOCK);
                    winsAgainst.add(PAPER);
                }
                return winsAgainst;
            }
        },
        SPOCK("spock") {
            @Override
            public List<Choices> getWinsAgainst() {
                if (winsAgainst.isEmpty()) {
                    winsAgainst.add(ROCK);
                    winsAgainst.add(SCISSORS);
                }
                return winsAgainst;
            }
        };

        private String keyword;

        protected List<Choices> winsAgainst;

        private Choices(String keyword) {
            this.keyword = keyword;
            this.winsAgainst = new ArrayList<>();
        }

        public String getKeyword() {
            return keyword;
        }

        public abstract List<Choices> getWinsAgainst();
    }

    public void printUserOptions() {
        StringBuilder stringBuilder = new StringBuilder();
        stringBuilder.append("Input your choice of one of the following:");
        for (Choices choice : Choices.values()) {
            stringBuilder.append(" ");
            stringBuilder.append(choice.getKeyword());
        }
        System.out.println(stringBuilder.toString());
    }

    public Choices getUserChoice() {
        boolean isUserChoiceValid = false;
        BufferedReader bufferedReader = null;
        Choices userChoice = null;
        try {
            bufferedReader = new BufferedReader(new InputStreamReader(System.in));
            do {
                String userChoiceString = bufferedReader.readLine();
                isUserChoiceValid = validateUserChoice(userChoiceString);
                if (!isUserChoiceValid) {
                    System.out.println("Please enter one of the valid options.");
                } else {
                    userChoice = Choices.valueOf(userChoiceString.toUpperCase());
                }
            } while (!isUserChoiceValid);
        } catch (IOException e) {
            e.printStackTrace();
            throw new RuntimeException("There was an error while reading from input.", e);
        } finally {
            if (bufferedReader != null) {
                try {
                    bufferedReader.close();
                } catch (IOException e) {
                    e.printStackTrace();
                }
            }
        }
        return userChoice;
    }


    public boolean validateUserChoice(String userChoice) {
        for (Choices choice : Choices.values()) {
            if (choice.getKeyword().equals(userChoice)) {
                return true;
            }
        }
        return false;
    }

    public Choices getComputerChoice() {
        return Choices.values()[new Random().nextInt(Choices.values().length)];
    }

    public void evaluateResult(Choices userChoice, Choices computerChoice) {
        if (userChoice == computerChoice) {
            System.out.println("It's a tie!");
            return;
        }

        if (userChoice.getWinsAgainst().contains(computerChoice)) {
            System.out.println("You won!");
        } else {
            System.out.println("The computer won.");
        }
    }

    public void execute() {
        printUserOptions();
        Choices userChoice = getUserChoice();
        Choices computerChoice = getComputerChoice();

        System.out.println("");
        System.out.println("You picked: " + userChoice.getKeyword());
        System.out.println("Computer picked: " + computerChoice.getKeyword());
        System.out.println("");
        evaluateResult(userChoice, computerChoice);
    }

    public static void main(String[] args) {
        RockPaperScissors rockPaperScissors = new RockPaperScissors();
        rockPaperScissors.execute();
    }
}

Current output:

Input your choice of one of the following: rock paper scissors lizard spock
scissors

You picked: scissors
Computer picked: spock

The computer won.

Apart from the boilerplate on the enums which I find that while despite being lengthy, it does make it easier to read the code on the long-run.

Is there anything specific that could be done to improve the app, make it more scalable, maybe make it easy to detach it from being console-only (look at those System.outs, those keep it from being easily improved from one framework to another)? What can you think of as an improvement?

\$\endgroup\$
5
\$\begingroup\$

Nice implementation. There are a few items I would criticise, but, overall I am impressed.

Choices

  1. Enums should have singlular names. Choices should be just Choice. This would eliminate problems like public Choices getUserChoice() {...} which returns just one thing despite the return value.

  2. Your class is not safe. It is public, and has a public method public abstract List<Choices> getWinsAgainst(); which returns a 'live' list. If I were a mean player, I would do:

    public static final void main (String[] args) {
        Choices[] opts = Choices.values();
        for (Choices c : opts) {
            c.getWinsAgains().clear();
        }
        Choices myChoice = opts[0];
        for (int i = 1; i < opts.length; i++) {
            myChoice.getWinsAgainst().add(opts[i]);
        }
    
        // .... now the user only ever chooses myChoice .... ;-) Happy Christmas!!! 
    }
    

    You need to encapsulate your data correctly. Private data should be private... what you should consider, instead of the getWinsAgainst() method instead have something like public boolean winsAgainst(Choice otherChoice); which will return true if the internal data says it should. This does not leak data.

  3. While we are looking there, the data is not thread safe. Enums are supposed to be safe to use concurrently... and yours is not. Two threads accessing the enums at the same time may overpopulate, or corrupt, the winning strategies. One option to solve this is to use a static initializer block:

    public enum Choice {
        ROCK("rock"),
        PAPER("paper"),
        SCISSORS("scissors"),
        LIZARD("lizard"),
        SPOCK("spock");
    
        static {
            ROCK.winsAgainst.add(SCISSORS);
            ROCK.winsAgainst.add(LIZARD);
            PAPER.winsAgainst.add(ROCK);
            PAPER.winsAgainst.add(SPOCK);
            SCISSORS.winsAgainst.add(PAPER);
            SCISSORS.winsAgainst.add(LIZARD);
            LIZARD.winsAgainst.add(SPOCK);
            LIZARD.winsAgainst.add(PAPER);
            SPOCK.winsAgainst.add(ROCK);
            SPOCK.winsAgainst.add(SCISSORS);
        }
    
        private final String keyword;
    
        private final List<Choice> winsAgainst = new ArrayList<>();
    
        private Choice(String keyword) {
            this.keyword = keyword;
        }
    
        public String getKeyword() {
            return keyword;
        }
    
        public boolean winsAgainst(Choice otherChoice) {
            return winsAgainst.contains(otherChoice);
        }
    }
    

    That should be almost the same bhaviour (except that it is called Choice, it is thread-safe, and that the enum fields are now also final, and private).

    Static initializer blocks are uncommon, but can be very, very useful.

Remaining code

  • Instead of creating a new Buffered reader each time you get user input, you should instead create the buffered reader immediately on System.in, and then pass that reader in to the getUserChoice method. While your exception handling in there is commendable, you would be better served by doing that outside the method in your execute method, and using a try-with-resources approach. I really like the use of the do-while loop though. Good choice... but, consider doing a while(true), and just returning immediately with a valid choice from inside the loop. No need for managing the cumbersome isUserChoiceValid variable.

  • Random - you create a new one each time the computer plays:

    return Choices.values()[new Random().nextInt(Choices.values().length)];
    

    There is no value in that (in fact, it's bad). You should simply create a static, or class field, and then reuse it:

     private static final Random rand = new Random();
    
     return Choices.values()[rand.nextInt(Choices.values().length)];
    
\$\endgroup\$
  • \$\begingroup\$ First of all, thank you for taking the time to look through the code! :) I'm actually most amazed by point #3. At first, I was trying to use instance initializers to add the right Choices to the lists, but instance initializers were forbidden for the enum - and I didn't think of the static initializer blocks - so it's shorter, more efficient, and actually safe! I honestly didn't even think of #2 though. :| now I will. Thank you again, this will help me be aware of these mistakes! :) \$\endgroup\$ – Zhuinden Dec 11 '14 at 20:57
  • 1
    \$\begingroup\$ +1 but... I think EnumSet<Choice> would be an improvement on List<Choice>. Also, everybody working with enums should be reminded to reread the related chapter in Effective Java, 2nd Edition. \$\endgroup\$ – VoiceOfUnreason Dec 11 '14 at 22:22
  • 1
    \$\begingroup\$ @VoiceOfUnreason - good point about the EnumSet ... hmmm. Why did I not take it that step further. Drat. \$\endgroup\$ – rolfl Dec 11 '14 at 22:24
  • \$\begingroup\$ @VoiceOfUnreason now that you mention it, I forgot about EnumSet<T> in this particular case even though I've used them before, after seeing the following answer on SO by Gexicide: stackoverflow.com/a/24651741/2413303 \$\endgroup\$ – Zhuinden Dec 11 '14 at 22:33
  • 1
    \$\begingroup\$ public and private have nothing to do with security. The player of your game is not writing code that you are executing. If they are, they can just not call your function and instead write their own function that always returns that they win, or modify your source code. Suggesting that private is a security feature is dangerous. \$\endgroup\$ – David Stone Dec 11 '14 at 23:28
1
\$\begingroup\$

I'd love an initialization like

ROCK(PAPER, SCISSORS), ...

enlisting all the "wins against". However, this is impossible as when the constructor gets executed, the enum literals don't yet exist. The strings would do, but this is not especially nice. Initial letters would be perfect but SCISSORS and SPOCK collide.

So I chose the second letters, which is probably good to show what can be done, but I surely would not recommend really doing it.

public enum Choice {
    ROCK("CI"), // C = sCissors, I =lIzzard, ...
    PAPER("OP"),
    SCISSORS("AI"),
    LIZARD("PA"),
    SPOCK("OC"),
    ;

    Choice(String winsAgainstString) {
        this.winsAgainstString = winsAgainstString;
        this.keyword = name().toLowerCase();
    }

    public boolean winsAgainst(Choice choice) {
        final char secondChar = choice.name().charAt(1);
        // there's no `contains` for chars
        return winsAgainstString.indexOf(secondChar) != -1;
    }

    private final String winsAgainstString;
    @Getter private final String keyword;
}

No need to specify the keyword in the initializer when it can be so trivially computed. The Lombok Getter annotation does exactly the expected thing.

\$\endgroup\$
  • \$\begingroup\$ this.keyword = name().toLowerCase(); smart. I'm surprised I didn't think of that. I don't really like the idea of specifying the relation in a string, though. Although it is a fine alternative. \$\endgroup\$ – Zhuinden Dec 12 '14 at 9:43
  • \$\begingroup\$ @Zhuinden The string itself is fine, but using the second letter is terrible. For things like King, Queen, Rook, ..., the initial letters are unique and well established and I'd use them. \$\endgroup\$ – maaartinus Dec 12 '14 at 9:47
  • \$\begingroup\$ if I want to specify them as strings, I may as well, just write out the whole thing and give the constructor a String... parameter \$\endgroup\$ – Zhuinden Dec 12 '14 at 9:48
  • \$\begingroup\$ that could actually work pretty well, though. \$\endgroup\$ – Zhuinden Dec 12 '14 at 9:56

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.