A TDD Exercise on Snakes and Ladders

Question

I finished the Snakes and Ladders kata in the Agile Katas and I would like to know if there's a way to improve my code (specifically the part where the turn order is determined).

Game

public class Game {
    private Map<Player, Token> playerTokenMap;
    private Player winner;
    private final int lastSquareLocation = 101;
    private Space[] board;
    private final Player computer = new Player("COMPUTER");
    private LinkedList<Player> turnOrder;
    private int turnNumber;

    public Game(SpecialEnds specialEnds) {
        playerTokenMap = new HashMap<Player, Token>();
        turnOrder = new LinkedList<Player>();
        board = new Space[lastSquareLocation];
        for(int squareLocation = 1; squareLocation < lastSquareLocation; squareLocation++){
            Space space = new Space();
            if(specialEnds.leadsToAnotherSquareLocation(squareLocation))
                space.setDestination(specialEnds.getDestination(squareLocation));
            board[squareLocation] = space;
        }
        this.winner = Player.NO_ONE;
    }

    public void addPlayer(Player player) {
        turnOrder.clear();
        playerTokenMap.put(player, new Token());
    }

    public int getPositionOfTokenOf(Player tokenOwner) {
        return playerTokenMap.get(tokenOwner).getLocation();
    }

    public void turnPlayerMove(int spaces) {
        Player turnPlayer = getTurnPlayer();
        Token token = playerTokenMap.get(turnPlayer);
        if(tokenWillGoPastTheLastSquare(token, spaces))
            return;
        token.move(spaces);
        int tokenLocation = token.getLocation();
        if(hasSpecialEnd(tokenLocation))
            token.move(this.board[tokenLocation].getDestination() - tokenLocation);
        if(hasWonTheGame(token))
            this.winner = turnPlayer;
        turnNumber++;
    }

    private boolean hasSpecialEnd(int squareLocationOfToken) {
        return board[squareLocationOfToken].isYieldingToAnotherDestination();
    }

    private boolean hasWonTheGame(Token token) {
        return token.getLocation() == lastSquareLocation - 1;
    }

    private boolean tokenWillGoPastTheLastSquare(Token token, int numberOfSpacesToMove) {
        return token.getLocation() + numberOfSpacesToMove >  lastSquareLocation - 1;
    }

    public Player getWinner() {
        return winner;
    }

    public Player getTurnPlayer() {
        if(turnOrder.isEmpty())
            return Player.NO_ONE;
        return turnOrder.get(turnNumber % playerTokenMap.size());
    }

    public void determinePlayOrder() {
        SortedMap<Integer, Player> playOrderPlayerMap = new TreeMap<Integer, Player>();
        for(Player player : playerTokenMap.keySet()){
            int dieRollResult = player.rollDice();
            if(!playOrderPlayerMap.containsKey(dieRollResult))
                playOrderPlayerMap.put(dieRollResult, player);
            else{
                while(playOrderPlayerMap.containsKey(dieRollResult)){
                    dieRollResult = player.rollDice();
                }
                playOrderPlayerMap.put(dieRollResult, player);
            }
        }
        turnOrder.addAll(playOrderPlayerMap.values());
        Collections.reverse(turnOrder);
    }

    public void addComputerToGame() {
        addPlayer(computer);
    }
}

Space

public class Space {
    private int destination;

    public Space() {
        this.destination = -1;
    }

    public void setDestination(int destination) {
        this.destination = destination;
    }

    public boolean isYieldingToAnotherDestination() {
        return destination != -1;
    }

    public int getDestination() {
        return destination;
    }
}

SpecialEnds

public class SpecialEnds {
    private Map<Integer, Integer> mapOfSpecialEnds;

    public SpecialEnds() {
        mapOfSpecialEnds = new HashMap<Integer, Integer>();
    }

    public void add(int source, int destination){
        mapOfSpecialEnds.put(source, destination);
    }

    public boolean leadsToAnotherSquareLocation(int squareLocation) {
        return mapOfSpecialEnds.containsKey(squareLocation);
    }

    public int getDestination(int squareLocation) {
        return mapOfSpecialEnds.get(squareLocation);
    }
}

---

GameTest

public class GameTest {
    private Game game;
    private Player player1;

    @Before
    public void setUp() throws Exception {
        SpecialEnds specialEnds = new SpecialEnds();
        specialEnds.add(12, 2);
        specialEnds.add(3, 13);
        game = new Game(specialEnds);
        player1 = new Player("Rome");
        game.addPlayer(player1);
        game.determinePlayOrder();
    }

    @Test
    public void tokenShouldStartOnSquareOne() throws Exception {
        int expectedSquareLocation = 1;
        assertPlayerIsIn(player1, expectedSquareLocation);
    }

    @Test
    public void tokenShouldBeInSquare4AfterMoving3SpacesFromSquare1() throws Exception {
        int expectedSquareLocation = 4;
        int numberOfSpacesToMove = 3;
        turnPlayerMove(numberOfSpacesToMove);
        assertPlayerIsIn(player1, expectedSquareLocation);
    }

    @Test
    public void tokenShouldBeInSquare8AfterMoving3SpacesThen4SpacesFromSquare1() throws Exception {
        int expectedSquareLocation = 8;
        int firstNumberOfSpacesToMove = 3;
        int secondNumberOfSpacesToMove = 4;
        turnPlayerMove(firstNumberOfSpacesToMove);
        turnPlayerMove(secondNumberOfSpacesToMove);
        assertPlayerIsIn(player1, expectedSquareLocation);
    }

    private void turnPlayerMove(int spaces) {
        game.turnPlayerMove(spaces);
    }

    @Test
    public void numberOfMovesShouldBeDecidedByDieRolls() throws Exception {
        int dieResult = 1;
        int expectedSquareLocation = 5;
        while (dieResult != 4) {
            dieResult = player1.rollDice();
        }
        turnPlayerMove(dieResult);
        assertPlayerIsIn(player1, expectedSquareLocation);
    }

    @Test
    public void playerShouldWinTheGameWhenTheirTokenReachesThe100thSquare() throws Exception {
        turnPlayerMove(96);
        int numberOfSpacesToMove = 3;
        turnPlayerMove(numberOfSpacesToMove);
        assertEquals(player1, game.getWinner());
    }

    @Test
    public void playerShouldNotYetWinTheGameWhenTheirTokenMovementWillExceedThe100thSquare() throws Exception {
        turnPlayerMove(96);
        int numberOfSpacesToMove = 4;
        turnPlayerMove(numberOfSpacesToMove);
        assertNotEquals(player1, game.getWinner());
    }

    @Test
    public void tokenThatLandsOnASnakeHeadShouldGoDownOnTheTail() throws Exception {
        int expectedSquareLocation = 2;
        turnPlayerMove(11);
        assertPlayerIsIn(player1, expectedSquareLocation);
    }

    @Test
    public void tokenThatLandsOnASnakeTailShouldStayThere() throws Exception {
        int expectedSquareLocation = 2;
        turnPlayerMove(1);
        assertPlayerIsIn(player1, expectedSquareLocation);
    }

    @Test
    public void tokenThatLandsOnLowerEndOfTheLadderShouldGoUpTheLadder() throws Exception {
        int expectedSquareLocation = 13;
        turnPlayerMove(2);
        assertPlayerIsIn(player1, expectedSquareLocation);
    }

    @Test
    public void tokenThatLandsOnUpperEndOfTheLadderShouldStayThere() throws Exception {
        int expectedSquareLocation = 13;
        turnPlayerMove(12);
        assertPlayerIsIn(player1, expectedSquareLocation);
    }

    @Test
    public void player1ShouldBeFirstIfPlayer1RollsHigherThanPlayer2() throws Exception {
        Player player2 = new Player("Ein");
        game.addPlayer(player2);
        while (isNotFirstToRoll(player1))
            game.determinePlayOrder();
        assertTurnPlayerIs(player1);
    }

    @Test
    public void player2ShouldBeFirstIfPlayer2RollsHigherThanPlayer1() throws Exception {
        Player player2 = new Player("Ein");
        game.addPlayer(player2);
        while (isNotFirstToRoll(player2))
            game.determinePlayOrder();
        assertTurnPlayerIs(player2);
    }

    private void assertTurnPlayerIs(Player player2) {
        assertEquals(player2, game.getTurnPlayer());
    }

    @Test
    public void player2ShouldGoNextAfterPlayer1() throws Exception {
        Player player2 = new Player("Ein");
        game.addPlayer(player2);
        while (isNotFirstToRoll(player1))
            game.determinePlayOrder();
        turnPlayerMove(2);
        assertTurnPlayerIs(player2);
    }

    @Test
    public void player1ShouldGoNextAfterPlayer2GoingAfterPlayer1() throws Exception {
        Player player2 = new Player("Ein");
        game.addPlayer(player2);
        while (isNotFirstToRoll(player1))
            game.determinePlayOrder();
        turnPlayerMove(2);
        turnPlayerMove(2);
        assertTurnPlayerIs(player1);
    }

    private boolean isNotFirstToRoll(Player player) {
        return !player.equals(game.getTurnPlayer());
    }

    @Test
    public void computerShouldGoFirstIfComputerRollsHigherThanPlayer1() throws Exception {
        Player expectedTurnPlayer = new Player("COMPUTER");
        game.addComputerToGame();
        while (isNotFirstToRoll(expectedTurnPlayer))
            game.determinePlayOrder();
        assertTurnPlayerIs(expectedTurnPlayer);
    }

    @Test
    public void computerShouldBeAbleToRollTheDie() throws Exception {
        int expectedSquareLocation = 2;
        Player computer = new Player("COMPUTER");
        game.addComputerToGame();
        while (isNotFirstToRoll(computer))
            game.determinePlayOrder();
        turnPlayerMove(1);
        assertPlayerIsIn(computer, expectedSquareLocation);
    }

    private void assertPlayerIsIn(Player computer, int expectedSquareLocation) {
        assertTrue(game.getPositionOfTokenOf(computer) == expectedSquareLocation);
    }
}

Answer 1

Unit Tests

I downloaded the code from the question and then stubbed the classes that weren't present (Token and Player). When I ran the unit tests, they hung. If you're treating the whole code base as a unit (which you appear to be doing), then they should all be included in the question. However, it seems like a red flag that the test hung, rather than failing. This suggests to me that they might be doing too much / be a bit brittle, so I've mostly looked at some of them.

DAMP vs DRY

DAMP vs DRY often has polarised views. I like my unit tests to be descriptive enough that the relevant bits of any setup and assertions are present in the individual tests. This might lead to a bit of repeated code, however only a small amount of code should need to be repeated and it can make the tests clearer.

So, looking at your first test:

@Test
public void tokenShouldStartOnSquareOne() throws Exception {
    int expectedSquareLocation = 1;
    assertPlayerIsIn(player1, expectedSquareLocation);
}

I think this is too DRY focused. The game is setup in the fixtures setup, which includes putting player1 on the board. Some of this initial setup seems relevant to the thing actually being tested here. The assertPlayerIsIn method hides how we tell if a player is in a given location. It's actually a wrapper for this single assertion:

assertTrue(game.getPositionOfTokenOf(computer) == expectedSquareLocation);

A few things to note, the player name parameter is actually 'computer' on the method signature which is misleading. assertTrue will also only tell you if the equality is correct or not, passing the two parameters into assertEquals on the other hand outputs both values if the test fails which is far more helpful for diagnosing the problem. So, a couple of minor changes and the first test looks like:

@Test
public void tokenShouldStartOnSquareOne() throws Exception {
    Player newPlayer = new Player("player2");
    game.addPlayer(newPlayer);
    assertEquals(1, game.getPositionOfTokenOf(newPlayer));
}

It's an extra line, but I think it's more descriptive. There are other helper methods in your tests, which I probably wouldn't have created, for example turnPlayerMove. Part of what I'm looking for when I use TDD is to get a feel for how easy the objects under test are to work with. If too much of this interaction gets hidden in private methods that make the interface more usable then I don't think you get this benefit. You don't get the same feeling that actually you need to refactor something.

Another example of how 'DRY'ing it up can be confusing is:

public void playerShouldWinTheGameWhenTheirTokenReachesThe100thSquare() throws Exception {
    turnPlayerMove(96);
    int numberOfSpacesToMove = 3;
    turnPlayerMove(numberOfSpacesToMove);
    assertEquals(player1, game.getWinner());
}

At first glance, I'm thinking 96 + 3 = 99, why is it on square 100... but of course, rather than putting the token on square 97, your putting it on 1, then moving it 96 to get to 97, before moving it 3 for the test. This seems quite confused/convoluted and again suggests a possible refactoring...

Tokens and Players

The Kata talks about Tokens and Players in a way that suggests they are distinct things. However, the way your tests are written this can be confused. 'tokenShould...for the test name andassertPlayerIs...` for the validation. Again, this is a flag that maybe the modelling isn't quite right and some refactoring could be useful.

turnPlayerMove

Tests like tokenShouldBeInSquare8AfterMoving3SpacesThen4SpacesFromSquare1 use turnPlayerMove to validate that if a token moves N spaces, then the player will be in space currentPosition + N. However, turnPlayerMove rotates the player that it moving. So, if I add another player, then instead of player 1 moving 3 + 4 spaces, player 1 moves 3 and player 2 moves 4. If it's relevant, then consider adding critical test dependencies to the test method signature as part of the scenario being tested. So, singlePlayerOnSquare1Moves3Then4ShouldBeAt8 for example.

Hanging Dependencies

@Test
public void numberOfMovesShouldBeDecidedByDieRolls() throws Exception {
    int dieResult = 1;
    int expectedSquareLocation = 5;
    while (dieResult != 4) {
        dieResult = player1.rollDice();
    }
    turnPlayerMove(dieResult);
    assertPlayerIsIn(player1, expectedSquareLocation);
}

This is a strange test. It's largely testing the same move logic that's been tested previously, however it's also testing that eventually a 4 can be rolled by the player. This isn't what the test name suggests. It was also one reason why my stubbed version of Player hung, since I just had rollDice returning 1. There are similar while loops in other tests, which hang if the underlying code doesn't behave as expected. Test should fail fast if there's an issue with the code, tests that hang are bad and can result in manual intervention being required on build servers.

determinePlayOrder

I'm fairly sure the logic here doesn't match the Kata description. It says, that if both players roll the same number, then they have to roll again. It looks to me like your logic is, player 1 rolls and reserves that number, then player 2 has to keep rolling until they get something other than the value player 1 rolled. This also wouldn't scale if more than 6 people played.