Rock-Paper-Scissors-Lizard-Spock game

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 getWinsAgainst() {
if (winsAgainst.isEmpty()) {
winsAgainst.add(SCISSORS);
winsAgainst.add(LIZARD);
}
return winsAgainst;
}
},
PAPER("paper") {
@Override
public List getWinsAgainst() {
if (winsAgainst.isEmpty()) {
winsAgainst.add(ROCK);
winsAgainst.add(SPOCK);
}
return winsAgainst;
}
},
SCISSORS("scissors") {
@Override
public List getWinsAgainst() {
if (winsAgainst.isEmpty()) {
winsAgainst.add(PAPER);
winsAgainst.add(LIZARD);
}
return winsAgainst;
}
},
LIZARD("lizard") {
@Override
public List getWinsAgainst() {
if (winsAgainst.isEmpty()) {
winsAgainst.add(SPOCK);
winsAgainst.add(PAPER);
}
return winsAgainst;
}
},
SPOCK("spock") {
@Override
public List getWinsAgainst() {
if (winsAgainst.isEmpty()) {
winsAgainst.add(ROCK);
winsAgainst.add(SCISSORS);
}
return winsAgainst;
}
};

private String keyword;

protected List winsAgainst;

private Choices(String keyword) {
this.keyword = keyword;

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

Choices

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

-
Your class is not safe. It is public, and has a public method public abstract List 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.

-
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 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)];

StackExchange Code Review Q#73369, answer score: 5