Rock Paper Scissors Lizard Spock Revisited

Question

Continuing the spirit of the recent Weekend Challenge, here is a revised version of the RPSLS game.

This is a follow up to my previous submission:

Rock Paper Scissors Lizard Spock as a code-style and challenge

This version is revised to accommodate the following suggestions:

1. Model revised to abstract the display mechanism from the game engine.
2. Removed static method calls where appropriate
3. fixed bugs with binary search
4. included the 'verb' component of 'Rock crushes Scissors'
5. tracked the scoring inside each player rather than in the main method.
6. removed magic number abuse.

I have implemented both a console-based and GUI (Swing) based interface for the game. This is an image of the GUI. The fine-tuning of the layout is not complete, but that is not a priority for me in terms of a review (The icons here (CC-attribution) come from Wikipedia ):

To accomodate the suggestions above I have separated the logic in to a number of classes. The following are the core classes (does not include the GUI and Console interfaces). If there's any cool ideas in here that you may feel have been copied from other submissions, then you are probably right...

Weapon.java

package rpsls;

import java.util.HashMap;
import java.util.Map;

/**
 * Set up the rules for the game.
 * What are the moves, and what beats what (and how)!
 */
public enum Weapon {
    Rock, Paper, Scissors, Lizard, Spock;

    static {
        /*
         * Scissors cuts paper, paper covers rock, rock crushes lizard, lizard poisons Spock,
         * Spock smashes scissors, scissors decapitate lizard, lizard eats paper,
         * paper disproves Spock, Spock vaporizes rock. And as it always has, rock crushes scissors.
         *      Rd. Cooper
         */
        Scissors.will("cut", Paper);
        Paper.will("covers", Rock);
        Rock.will("crushes", Lizard);
        Lizard.will("poisons", Spock);
        Spock.will("smashes", Scissors);
        Scissors.will("decapitate", Lizard);
        Lizard.will("eats", Paper);
        Paper.will("disproves", Spock);
        Spock.will("vaporizes", Rock);
        Rock.will("smashes", Scissors);
    }

    // what will this move beat - populated in static initializer
    // cannot use enummap... recursive constructors otherwise.
    private final Map<Weapon, String> ibeat = new HashMap<>();
    private void will(String verb, Weapon move) {
        ibeat.put(move, verb);
    }

    /**
     * Return a non-null 'verb' if this Move will beat the supplied move
     * @param move the move we hope to beat
     * @return the verb if we beat that move i.e. if we are Rock, we will return 'crushes' if the input move is 'Scissors'.
     */
    public String beats(Weapon move) {
        // use binary search in case someone wants to set up crazy rules.
        return ibeat.get(move);
    }

}

PlayerListener.java

package rpsls;

public interface PlayerListener {

    void wonGame(Player abstractPlayer, Weapon mine, String verb, Weapon theirs);

    void wonSet(Player abstractPlayer);

    void resetGames(Player abstractPlayer);

}

Player.java

package rpsls;

import java.util.ArrayList;

public abstract class Player {
    private final String name;
    private int games = 0;
    private int sets = 0;
    private final ArrayList<PlayerListener> listeners = new ArrayList<>();

    public Player(String name) {
        this.name = name;
    }

    public void addListener(PlayerListener listener) {
        listeners.add(listener);
    }

    @Override
    public String toString() {
        return String.format("%s Games %d Sets %d", name, games, sets);
    }

    public final void wonGame(Weapon mine, String verb, Weapon theirs) {
        games++;
        for (PlayerListener pl : listeners) {
            pl.wonGame(this, mine, verb, theirs);
        }
    }

    public final void wonSet() {
        sets++;
        for (PlayerListener pl : listeners) {
            pl.wonSet(this);
        }
    }

    public String getName() {
        return name;
    }

    public int getGames() {
        return games;
    }

    public int getSets() {
        return sets;
    }

    public void resetGames() {
        games = 0;
        for (PlayerListener pl : listeners) {
            pl.resetGames(this);
        }
    }

    public abstract Weapon nextMove();


}

ComputerPlayer.java

package rpsls;

import java.util.Random;

public class ComputerPlayer extends Player {

    private final Random random = new Random();
    private Weapon[] weapons = Weapon.values();
    public ComputerPlayer() {
        super("Computer");
    }

    @Override
    public Weapon nextMove() {
        return weapons[random.nextInt(weapons.length)];
    }

}

HumanPlayer.java

package rpsls;

public class HumanPlayer extends Player {

    private final HumanInteraction input;
    public HumanPlayer(String name, HumanInteraction input) {
        super(name);
        this.input = input;
    }

    @Override
    public Weapon nextMove() {
        return input.getWeapon();
    }

}

HumanInteraction.java

package rpsls;

public interface HumanInteraction {
    public Weapon getWeapon();
    public boolean playAgain();
    public Player getHumanPlayer();
    public void draw(Weapon weapon);
    public void exiting();
}

and the main class

RPSLS.java

package rpsls;


/**
 * Rock Paper Scissors Lizard Spock
 * <p>
 * http://en.wikipedia.org/wiki/Rock-paper-scissors-lizard-Spock
 * <p>
 * Interface for a human to play against the computer.
 * 
 * @author rolf
 *
 */
public class RPSLS {

    /**
     * Run the game.... Good Luck!
     * @param args these are ignored.
     */
    public static void main(String[] args) {

        final boolean gui = args.length == 0 || "gui".equals(args[0]);

        final int firstto = 2; // winner is the first to this.
        final Player computer = new ComputerPlayer();
        final HumanInteraction interaction = gui ? new HumanGUI(computer) : new HumanConsole(computer);

        // human is not created by default because the interaction level can give them their name.
        final Player human = interaction.getHumanPlayer();
        do {
            // like tennis, with games, and sets.
            // best of 3 (first to 2) games wins a set.
            human.resetGames();
            computer.resetGames();

            setloop: do {
                // The computer chooses their weapon.
                final Weapon compweap = computer.nextMove();
                // the human chooses thir weapon
                final Weapon humanweap = human.nextMove();
                if (humanweap == null) {
                    // the user quits.
                    break setloop;
                }
                String verb = null; 
                if ((verb = humanweap.beats(compweap)) != null) {
                    human.wonGame(humanweap, verb, compweap);
                } else if ((verb = compweap.beats(humanweap)) != null){
                    computer.wonGame(compweap, verb, humanweap);
                } else {
                    interaction.draw(humanweap);
                }
                // play until someone scores 2.... best-of-three
            } while (human.getGames() != firstto && computer.getGames() != firstto);
            if (human.getGames() == firstto) {
                human.wonSet();
            } else {
                // including if human quits.
                computer.wonSet();
            }

        } while (interaction.playAgain());

        interaction.exiting();

    }

}

Answer 1

• Love the way you're using HashMap to provide the strings for the items. But in the UI, how would it look if you pick Spock and computer picks Lizard? Does it swap the positions for you? Or should it say "Spock gets poisoned by Lizard"? (I'm not sure of what's the best way is to solve this myself, but it is something to think about).

• It is more preferable to declare Lists by interface instead of implementation. So that, if you want to switch implementation, it's enough to swap at one place (the new ArrayList) instead of two.

  private final ArrayList<PlayerListener> listeners = new ArrayList<>(); // Bad
  
  private final List<PlayerListener> listeners = new ArrayList<>(); // Good
  

• Your RPSLS class only contains one big main method. And I still think that it does too much. RPSLS could instead be the game class, and the main method splitted into a couple of methods within the class.

Overall, I'd say your code looks great. Well done. Good use of interfaces. Good use of different classes and methods. It's been a pleasure reading your code. A great improvement compared to the previous version.

Answer 2

I agree with Simon in that you should extract game logic in a separate class, which would be your Model component of your applications the MVC.

// The computer chooses their weapon.
final Weapon compweap = computer.nextMove();
// the human chooses thir weapon
final Weapon humanweap = human.nextMove();
if (humanweap == null) {
    // the user quits.
    break setloop;
}

The game logic shouldn't care whether the players are human or computer (violating Lyskov's substitution principle). For example setting the game up with two computer players should not be just easy, it should be trivial. Also that Player.nextMove() can return null cannot be inferred by looking at the Player class only. A better way is to make nextMove() throw an exception, such as PlayerQuitsException, which not only make the contract explicit, it would also obviate the // the user quits. comment and the labeled break statement with a catch block. Moreover, a Player quitting when we expect his nextMove also sounds like an exceptional situation to me.

The following part doesn't read well:

String verb = null; 
if ((verb = humanweap.beats(compweap)) != null) {
    human.wonGame(humanweap, verb, compweap);
} else if ((verb = compweap.beats(humanweap)) != null){
    computer.wonGame(compweap, verb, humanweap);
} else {
    interaction.draw(humanweap);
}

Note how data flow of the variable verb is complicated. And doesn't translate to human language. Because it stands in for some other concept left implicit in this snippet, namely a turn outcome.

Also note the repetition in the first two if clauses.

Also note that we're passing arguments to the player class that it does not care.

compare:

Outcome outcome = getOutCome(player1, move1, player2, move2);
raiseEvent(new TurnEnded(outcome)); // update view etc

// you need not call this explicitly 
// if you make the Player a listener of outcome events
// and handle it there
if (outcome.isNotDraw()) {
   outcome.getWinner().wonGame();
}

similar problems here:

} while (human.getGames() != firstto && computer.getGames() != firstto);
if (human.getGames() == firstto) {
    human.wonSet();
} else {
    // including if human quits.
    computer.wonSet();
}

Player class[es] do both interaction with outside world (IO or random number generator) and score keeping (violating Single responsibility principle).

compare:

} while (scoreKeeper.gameHasNotEnded());

raiseEvent(new GameEnded(scoreKeeper.getGameOutcome())); // update view etc