Simple game of Chutes and Ladders

Question

I recently made a new project for Chutes and Ladders (the Hasbro version of Snakes and ladders) and it is located here: https://github.com/vchittar/ChutesAndLadders

BoardLogic (implements an interface):

@Service
public class BoardLogic implements IBoardLogic {

    private static final Logger LOG = LoggerFactory.getLogger(BoardLogic.class);

    @Autowired
    GameSpinner gameSpinner;

    @Autowired
    PlayerLogic playerLogic;

    @Autowired
    PlayerUtils playerUtils;

    /**
     * This method deals with updating the player positions that are on the board
     *
     * @param playerList - The list of players that are currently in play and that need to be updated
     * @throws CLException - Custom Chutes and Ladder Exception
     */
    @Override
    public void runGame(List<Player> playerList) throws CLException {
        boolean isWinner = false;
        GameBoard gb = new GameBoard();
        try {
            while (!isWinner) {
                for (Player player : playerList) {

                    // Positional Logic
                    int position = player.getPosition() + gameSpinner.spinner();
                    int prevPosition = player.getPosition();

                    if (position > 100) {
                        System.out.println(player.getName() + " got " + position + " " +
                                "which is more than 100, so they will stay put!");
                        continue;
                    }

                    // Move Logic for Players
                    playerLogic.nextTurn(player, position, gb);

                    //Print and Cleanup Logic
                    System.out.println(playerUtils.printBoard(player, prevPosition, position, gb));
                    resetPlayerStatus(player);

                    //Declare winner
                    if (player.getPosition() == 100) {
                        isWinner = true;
                        System.out.println("The winner is: " + player.getName());
                        break;
                    }
                }
            }
        } catch (Exception e) {
            LOG.error("Run Game has thrown an error! Please check your values", e);
            throw new CLException("Run game has thrown an exception. Please check the logs", e);
        }
    }

    /**
     * Reset the Player Direction status so their correct statuses are printed
     *
     * @param p1 - Player object whose status is reset
     */
    private void resetPlayerStatus(Player p1) {
        p1.setPlayerDirection(PlayerDirection.EMPTY);
    }

}

PlayerLogic:

@Service
public class PlayerLogic implements IPlayerLogic {

    private static final Map<Integer, Integer> ladderMap = new HashMap<>();
    private static final Map<Integer, Integer> chuteMap = new HashMap<>();
    private static final Logger LOG = LoggerFactory.getLogger(ApplicationDriver.class);

    /**
     * Method that initializes the ladder and chute map,
     * to set up the logic of the game
     */
    private void initializeBoard() {
        ladderMap.put(1, 38);
        ladderMap.put(4, 14);
        ladderMap.put(9, 31);
        ladderMap.put(21, 42);
        ladderMap.put(28, 84);
        ladderMap.put(36, 44);
        ladderMap.put(51, 67);
        ladderMap.put(71, 91);
        ladderMap.put(80, 100);

        chuteMap.put(16, 6);
        chuteMap.put(47, 26);
        chuteMap.put(49, 11);
        chuteMap.put(56, 53);
        chuteMap.put(62, 19);
        chuteMap.put(64, 60);
        chuteMap.put(87, 24);
        chuteMap.put(93, 73);
        chuteMap.put(95, 75);
        chuteMap.put(98, 78);
    }

    /**
     * This method deals with updating the chutes and ladder logic for our game
     * and updates the player object with either chutes or ladder or
     * returns the player object if no valid key is found in the map
     *
     * @param p1       - Player object
     * @param position - integer value that holds the current value of the dice roll
     * @param gb       - GameBoard object that holds values on the board
     * @throws CLException - Custom Exception for Chutes and Ladders
     * @return Player object that has been updated for either ladder or chute or neither/
     */
    @Override
    public Player nextTurn(Player p1, int position, GameBoard gb) throws CLException {
        try {
            // invalid input check
            if (p1.getPosition() == null || p1.getPosition() < 0 || gb == null) {
                LOG.error("Invalid position! Please check your values!");
                throw new CLException("Invalid position! Please check your values");
            }

            //initialize
            initializeBoard();
            p1.setPosition(position);
            if (ladderMap.containsKey(p1.getPosition())) {
                return goUpLadder(p1, gb);
            } else if (chuteMap.containsKey(p1.getPosition())) {
                return goDownChute(p1, gb);
            }
        } catch (Exception e) {
            LOG.error("There was an error generating the next turn correctly. Please check!", e);
            throw new CLException("Error getting the next turn for the player: " + p1.getName(), e);
        }
        return p1;
    }

    /**
     * This method deals with updating the ladder logic
     * that takes place within the game
     *
     * @param p1  - Player object that gets updated
     * @param gb  - GameBoard object that holds values on the board
     * @return Player object that has been updated with ladder logic
     */
    private Player goUpLadder(Player p1, GameBoard gb) {
        gb.setLadderDown(p1.getPosition());
        p1.setPosition(ladderMap.get(p1.getPosition()));
        p1.setPlayerDirection(PlayerDirection.LADDER);
        gb.setLadderTop(p1.getPosition());
        return p1;
    }

    /**
     * This method deals with updating the chute logic
     * that takes place within the game
     *
     * @param p1  - Player object that gets updated
     * @param gb  - GameBoard object that holds values on the board
     * @return Player object that has been updated with chute logic
     *
     */
    private Player goDownChute(Player p1, GameBoard gb) {
        gb.setChuteDown(chuteMap.get(p1.getPosition()));
        gb.setChuteTop(p1.getPosition());
        p1.setPosition(chuteMap.get(p1.getPosition()));
        p1.setPlayerDirection(PlayerDirection.CHUTE);
        return p1;
    }
}

Utils classes:

PlayerUtils:

@Service
public class PlayerUtils {

    private static final Logger LOG = LoggerFactory.getLogger(PlayerUtils.class);
    private static int turnCounter = 1;

    @Autowired
    GameSpinner gameSpinner;

    /**
     * This method deals with deciding the turn order amongst the users that are playing
     *
     * @param playerList - The List of players that want to play
     * @return List of players that have been organized into their proper turn order
     */
    public List<Player> deriveTurnOrder(List<Player> playerList) throws CLException {
        try {
            LOG.info("Time to get the player order!");
            for (Player player : playerList) {
                player.setInitialRoll(gameSpinner.spinner());
                System.out.println(player.getName() + " rolled a " + player.getInitialRoll());
            }
            // calculation that checks if any players need re-rolls
            setRerollCalcs(playerList);

            // get the highest index of the roll in the player list
            // made it into a variable here for additional clarity
            int maxIndex = getMaxRollIndex(playerList);

            // this calc basically gets all the elements from left side of the highest element, as stated in the reqs
            // for ex: {3 2 5 4} -> {5 2 3 4}
            Collections.reverse(playerList);
            Collections.rotate(playerList, maxIndex + 1);
        } catch (Exception e) {
            LOG.error("There was a problem deriving the turn order! Please check the exception");
            throw new CLException("Error in the deriving turn order method", e);
        }
            return playerList.stream().map(i -> {
                        i.setTurnOrder(turnCounter);
                        turnCounter++;
                        System.out.println(i.getName() + " will go " + i.getTurnOrder() + " and they rolled a " + i.getInitialRoll());
                        return i;
                    }).sorted(Comparator.comparingInt(Player::getTurnOrder))
                    .collect(Collectors.toList());

    }

    /**
     * This method handles the reroll calculations, if there are any to be
     * @param playerList - List of players that need to be rerolled
     */
    public void setRerollCalcs(List<Player> playerList) {
        List<Player> reRollListPlayers = new ArrayList<>();

        int max = 0;
        for (Player player : playerList) {
            if (player.getInitialRoll() > max) {
                max = player.getInitialRoll();
                reRollListPlayers.clear();
                reRollListPlayers.add(player);
            } else if (max == player.getInitialRoll()) {
                reRollListPlayers.add(player);
            }
        }
        if (reRollListPlayers.size() > 1) {
            reRollPlayers(reRollListPlayers, max);
            reRollListPlayers.clear();
        }
    }

    /**
     * This method works to get the maximum index of whoever got the highest roll
     * @param playerList - The list that we will traverse to get the highest index
     * @return Return an integer that holds the highest value for the index
     */
    private int getMaxRollIndex(List<Player> playerList) {
        AtomicInteger randomInt = new AtomicInteger();
        IntStream.range(0, playerList.size()).reduce((a, b) ->
                        playerList.get(a).getInitialRoll() < playerList.get(b).getInitialRoll() ? b : a)
                .ifPresent(randomInt::set);
        return randomInt.get();
    }

    /**
     * Method that basically rerolls till there are no two highest rolls
     * @param players - List of players that are to be rerolled
     * @param maxRoll - Max Roll contains the highest roll value in the player list
     */
    private void reRollPlayers(List<Player> players, int maxRoll) {
        players.stream().collect(Collectors.groupingBy(Player::getInitialRoll)).forEach((initialRoll, initialRollList) -> {
            if (initialRoll >= maxRoll) {
                initialRollList.forEach(player -> {
                    System.out.println("Will need to reroll for: " + player.getName());
                    player.setInitialRoll(gameSpinner.spinner());
                    System.out.println(player.getName() + " rerolled: " + player.getInitialRoll());
                });
            }
        });
        setRerollCalcs(players);
    }


    /**
     * Method that creates the players based on the arg names that were given
     *
     * @param names - List of names that are given in the CLI
     * @return List of player objects that contain the same names provided in the CLI
     */
    public List<Player> createPlayers(List<String> names) {

        return names.stream().map(Player::new)
                .collect(Collectors.toCollection(ArrayList<Player>::new));
    }

    /**
     * This method deals with printing out the output of the game to standard output
     *
     * @param player       - Player object
     * @param prevPosition - Previous position that the player was on
     * @param position     - The current position that the player is on
     * @param gb           - The gameboard object that keeps track of the game in progress
     */
    public String printBoard(Player player, int prevPosition, int position, GameBoard gb) throws CLException {

        if(player.getPlayerDirection() == null) {
            throw new CLException("Player direction is null, please double check your values");
        }
            switch (player.getPlayerDirection()) {
                case LADDER:
                    return String.format("%s: %d --> %d -- %s --> %d", player.getName(), prevPosition, position,
                            player.getPlayerDirection(), gb.getLadderTop());
                case CHUTE:
                    return String.format("%s: %d --> %d -- %s --> %d", player.getName(), prevPosition, position,
                            player.getPlayerDirection(), gb.getChuteDown());
                case EMPTY:
                    return String.format("%s: %d --> %d", player.getName(), prevPosition, position);
                default:
                    LOG.error("A valid path wasn't found!");
                    return "A valid path wasn't found";
            }
    }
}

I would love it if someone was able to give me any recommendations on what I could do to improve or what I have done well, basically a code review.

I have about 3.5 years or so of exp. So please let me know how I am progressing based on that.

I just want to understand if this is the best way to tackle the reroll logic and if the board logic was set up properly and if I set up my classes, logic correctly according to OOP standards and what not.

Answer 1

thanks for sharing your code.

What I like

• Dependency Injection
• Naming according to the conventions
• use of JVM provided fuctionality (Collections.*())

what I'd improve

OOP stuff

Obsessive Interface creation

At least the classe you shared here have Interfaces but only one implementation. On Top their Names only differ by the 'I' as first letter. This indicates, that this interfaces don't really have a use. This impression is supported by the fact, that you don't use this interfaces do declare variables anywhere in your own code.

Custom Exception

To Exceptions the same OOP rules appl as to any other class: we only create sub classes if this subclass introduces new behavior (not only configuration). Speaking of Exceptions this means that there should be some behavior inside the exception or the code that processes it, that cannot be done using a predefined exception, eg. if the catcher had to parse the message to do something special or if the custom exception simplifies the construction of the error message.

In general a custom exception is only useful if some catcher in your program (or thread) is able to recover safely without user interaction and can continue runig.

Language use

Variable declaration out of scope

In BoardLogic.runGame() You declare two local variables. Both seam to be out of scope.

Both are not used outside the try block. gb on the other hand is only used inside the loop, but needs to be the same object in each iteration. Therefore it should be a member variable of the BoardLogic object initialized in the constructor or injected via DI.

Instructions behind catch block

In some methods you placed the return statement behind the catch block. The consequence is that some of the variables used in the expression constructing the return value need to be visible outside the try block.

Misuse of Java8 stream

The Streams API is Javas functional programming facility. Functional programming in turn adopts the mathematical definition of a function which is: project the elements of one group to the elements of another group. The point here is that this does not include state change of the elements processed.

The following misuses the Streams API by changing the state of the elements processed.

      return playerList.stream().map(i -> {
                    i.setTurnOrder(turnCounter);
                    turnCounter++;
                    System.out.println(i.getName() + " will go " + i.getTurnOrder() + " and they rolled a " + i.getInitialRoll());
                    return i;
                }).sorted(Comparator.comparingInt(Player::getTurnOrder))
                .collect(Collectors.toList());

This I'd rather do in an ordinary foreach loop calling Collections.sort() on it afterwards.

The nails and hammer are not useless just because you have a screwdriver now... ;o)

general coding

Unaligned scope of variables and methods working on them

In PlayerLogic you have defines constants. But you have the member method initializeBoard() to process them. Even worse you call initializeBoard() on each players turn. The only reason that you didn't run into troubles is, that the HashMap collections does not allow duplicated keys.

The method initializeBoard() should better have been a static initializer. No code should be placed behind a catch block. In case the catch block does not (re-) throw an exception it shout have its own return statement since it almost ever returns a special "something went wrong" or "default" value in that case.

Mixing IO with logic

You do IO directly using System.out. That should be encapsulated so that your Game could get a graphical UI easily.

DI via field injection

The injected dependencies are tore in package private mutable fields. At least the mutability might be required by the DI framework. This could be avoided when using Constructor Injection.

I know that you most likely opted for field injection to avoid the constructor at all and that my point of view will raise arguments. But I feel much more uncomfortable with the violation of Encapsulation/Information Hiding you introduced by exposing mutable fields instead.

Clean Code

abbreviated variable names

There is no need to abbreviate variable names like gb or p1.

"Structural" comments

You placed lots of inline comments that somehow separate your long methods into smaller sections. I'd rather put these sections into methods of their own with names derived from the comments above them.

Deep block nesting

BoardLogic.runGame() has 4 levels of nested blocks. I'd extract at least 2 blocks to separate methods: The content of the try block and the content of the loop.

---

Answer to comment

Could you also elaborate on how to throw proper exceptions vs try-catch works like? I always get a little confused on when to properly throw an exception vs try-catch? If catch an error, can i wrap that error and throw a custom exception. Not always but on high level methods like boardlogic. Thanks! – smarty_pants

Exception handling is a science on its own so here is what I dicoverd. ;o)

Your programs main loop and any threads run() method should have a try/catch block covering the complete logic that shows any (unchecked) exception to the user thats not yet handled way down the call stack.

You should trow checked exceptions when you expect the program can recover from that exception at runtime by any chance. If that error is so severe that the program needs to be changed or at least restarted to solve the problem you should throw an unchecked exception.

Sometimes it is useful to catch checked exceptions from third party code and convert them into unchecd exceptions to comply with the rules above.

As already written custom exceptions should be used when the enable specific behavior of your program that would not (easy) possible with build in exceptions.

---

Sorry for the long writing but I had no time to make it shorter. ;o)

Hope it helps anyway...