3
\$\begingroup\$

I recently finished a Java application and I was wondering if you could code-review it just for the sake of understanding what I could have done better :)

Since the code is long, I figure it would be best to post a link to my github repo.

Structure

enter image description here

Code

Just as a heads up, I took out the imports of all the classes except the console ones which use a third party library just for the sake of making the snippet shorter.

TicTacToeException

public class TicTacToeException extends Exception {
    public TicTacToeException(String message){
        super(message);
    }
}

Player

public abstract class Player {
    private String symbol;

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

    public String getSymbol(){
        return symbol;
    }

    public abstract void makeMove(Board board) throws TicTacToeException;

    @Override
    public boolean equals(Object player){
        return this.symbol == ((Player) player).getSymbol();
    }
}

Human

public class Human extends Player {
    private int inputRow;
    private int inputCol;

    public Human(String symbol){
        super(symbol);
    }

    @Override
    public void makeMove(Board board) throws TicTacToeException {
        board.checkTile(inputRow, inputCol, this);
    }

    public void setMoveInput(int row, int column){
        inputRow = row;
        inputCol = column;
    }
}

Computer

public class Computer extends Player {
    private ComputerAI internalAI;

    public Computer(String symbol, ComputerAI ai){
        super(symbol);
        internalAI = ai;
    }

    @Override
    public void makeMove(Board board){
        internalAI.makeBestMove(board, this);
    }
}

Tile

/**
 * Represents a tile of the tic-tac-toe board.
 */
public class Tile {
    /**
     * The player who has checked the tile.
     */
    private Player player;

    public void check(Player player){
        this.player = player;
    }

    public void uncheck(){
        player = null;
    }

    public Boolean isChecked(){
        return player != null;
    }

    public String getCheck(){
        return isChecked() ? player.getSymbol() :  " ";
    }

    public Player getCheckingPlayer() {
        return player;
    }
}

Board

public class Board {
    private static final int ROW_COUNT = 3;
    private static final int COLUMN_COUNT = 3;

    private Tile[][] board;

    public Board(){
        board = new Tile[ROW_COUNT][COLUMN_COUNT];

        for(int i = 0; i < ROW_COUNT; i++){
            for(int j = 0; j < COLUMN_COUNT; j++) {
                board[i][j] = new Tile();
            }
        }
    }

    public Tile getTile(int row, int column){
        return board[row][column];
    }

    public void checkTile(int row, int column, Player player) throws TicTacToeException {
        Tile tileToCheck = board[row][column];

        if(tileToCheck.isChecked()){
            throw new TicTacToeException("The chosen tile [" + (row+1) + ", " + (column+1) + "] is already checked.");
        }

        tileToCheck.check(player);
    }

    public Tile[] getAvailableTiles(){
        List<Tile> availableTiles = new ArrayList<Tile>();
        for(int i = 0; i < ROW_COUNT; i++){
            for(int j = 0; j < ROW_COUNT; j++){
                if(!board[i][j].isChecked()){
                    availableTiles.add(board[i][j]);
                }
            }
        }

        return availableTiles.toArray(new Tile[0]); // Just for the sake of returning always arrays.
    }

    public Tile[] getRow(int row){
        return board[row];
    }

    public Tile[] getColumn(int column){
        Tile[] tiles = new Tile[ROW_COUNT];

        for(int row = 0; row < ROW_COUNT; row++){
            tiles[row] = board[row][column];
        }

        return tiles;
    }

    public Tile[] getMainDiagonal(){
        return new Tile[]{ board[0][0], board[1][1], board[2][2] };
    }

    public Tile[] getAntiDiagonal(){
        return new Tile[]{ board[0][2], board[1][1], board[2][0] };
    }

    public int getRowCount(){
        return ROW_COUNT;
    }

    public int getColumnCount(){
        return COLUMN_COUNT;
    }

    public List<Player> getPlayers(){
        List<Player> players = new ArrayList<Player>();
        for(int i = 0; i < ROW_COUNT; i++){
            for(int j = 0; j < COLUMN_COUNT; j++){
                Player player = board[i][j].getCheckingPlayer();
                if(player != null && !players.contains(player)){
                    players.add(player);
                }
            }
        }

        return players;
    }

    public Boolean isBoardComplete(){
        for(int i = 0; i < ROW_COUNT; i++){
            for(int j = 0; j < COLUMN_COUNT; j++){
                Tile tile = board[i][j];
                if(!tile.isChecked()){
                    return false;
                }
            }
        }
        return true;
    }

    public Boolean isEmpty(){
        return getAvailableTiles().length == ROW_COUNT*COLUMN_COUNT;
    }

    public Boolean hasWon(Player player){
        Tile[] diagonal = getMainDiagonal();
        Tile[] antiDiagonal = getAntiDiagonal();
        if(areCheckedBySamePlayer(diagonal,player) || areCheckedBySamePlayer(antiDiagonal, player)){
            return true;
        }

        for(int i = 0; i <= 2; i++){
            Tile[] row = getRow(i);
            Tile[] column = getColumn(i);
            if(areCheckedBySamePlayer(row, player) || areCheckedBySamePlayer(column, player)){
                return true;
            }
        }

        return false;
    }

    /**
     * Util method to re-use the logic. //TODO: does it really belong here?
     * @param tiles tiles to check
     * @return Boolean
     */
    private static Boolean areCheckedBySamePlayer(Tile[] tiles, Player player){
        String marker = player.getSymbol();
        return marker == tiles[0].getCheck() && tiles[0].getCheck() == tiles[1].getCheck() && tiles[0].getCheck() == tiles[2].getCheck();
    }
}

Game

public class Game {
    private Board board;
    private Player player1;
    private Player player2;
    private Player currentPlayer;

    public Game(Player player1, Player player2){
        this.player1 = player1;
        this.player2 = player2;

        // Default - player 1 goes first unless changed later.
        chooseInitialPlayer(player1);

        board = new Board();
    }

    public void chooseInitialPlayer(Player player){
        currentPlayer = player;
    }

    public void chooseInitialPlayer(String symbol) throws TicTacToeException {
        if(player1.getSymbol().equalsIgnoreCase(symbol)){
            chooseInitialPlayer(player1);
        }
        else if(player2.getSymbol().equalsIgnoreCase(symbol)){
            chooseInitialPlayer(player2);
        }
        else{
            throw new TicTacToeException("The input symbol doesn't correspond to any current player."
                    + "\n Player 1 is set as the default starting player.");
        }
    }

    public void changeTurn(){
        if(currentPlayer == player1){
            currentPlayer = player2;
        }
        else{
            currentPlayer = player1;
        }
    }

    public void makeMove(int row, int column) throws TicTacToeException {
        if(row > 3 || row < 1 || column > 3 || column < 1){
            throw new TicTacToeException("The row and column don't correspond to any coordinates in the board.");
        }

        ((Human) currentPlayer).setMoveInput(row-1, column-1);
        currentPlayer.makeMove(board);
    }

    public void makeMove() throws TicTacToeException {
        currentPlayer.makeMove(board);
    }

    public Boolean isBoardComplete(){
        return board.isBoardComplete();
    }

    public Boolean hasCurrentPlayerWon(){
        return board.hasWon(currentPlayer);
    }

    public Boolean isTie(){
        return isBoardComplete() && !hasCurrentPlayerWon();
    }

    public Player getCurrentPlayer() {
        return currentPlayer;
    }

    public Player[] getPlayers(){
        return new Player[]{ player1, player2 };
    }

    public Board getBoard() {
        return board;
    }
}

public interface ComputerAI {
    public void makeBestMove(Board board, Player player);
}

RulesAI

/**
 * Implements a rule-based algorithm to make the best possible move in a Tic-Tac-Toe board.
 * The approach taken is an implementation of the Chain-of-responsibility pattern.
 */
public class RulesAI implements ComputerAI {
    /**
     * Set of rules/strategies to decide the next move in priority order.
     */
    protected static final MoveStrategy[] STRATEGIES = {
            new WinStrategy(),
            new BlockWinStrategy(),
            new ForkStrategy(),
            new BlockForkStrategy(),
            new RandomMoveStrategy()
    };

    /**
     * Apply the different tile-checking strategies available in priority order in order to make the first
     * available move.
     * @param board the board in its current status.
     * @param currentPlayer the player to move next.
     */
    public void makeBestMove(Board board, Player currentPlayer){
        if(board.isEmpty()){
            // This would only be "the best move" in a 3x3 board.
            board.getTile(1,1).check(currentPlayer);
            return;
        }

        for(MoveStrategy strategy : STRATEGIES) {
            if (strategy.checkTile(board, currentPlayer)) {
                break;
            }
        }
    }
}

MoveStrategy

public abstract class MoveStrategy {
    public abstract boolean checkTile(Board board, Player player);

    protected boolean isWinningMove(Board board, Tile tileToCheck){
        Player currentPlayer = tileToCheck.getCheckingPlayer();

        if (board.hasWon(currentPlayer) || board.isBoardComplete()) {
            return true;
        }

        return false;
    }

    protected Player getOpponent(Board board, Player player){
        for(Player iteratedPlayer : board.getPlayers()){
            if(!iteratedPlayer.equals(player)){
                return iteratedPlayer;
            }
        }

        return null; //TODO - NotAPlayer class? throw exception?
    }
}

WinStrategy

/**
 * Strategy to mark an available tile only if it secures a win.
 */
public class WinStrategy extends MoveStrategy {
    @Override
    public boolean checkTile(Board board, Player player){
        Tile[] availableTiles = board.getAvailableTiles();

        for (Tile availableTile : availableTiles){
            availableTile.check(player);
            if(isWinningMove(board, availableTile)){
                return true;
            }

            availableTile.uncheck();
        }

        return false;
    }
}

RandomMoveStrategy

/**
 * Strategy to mark the first available tile in the board.
 * //TODO - It serves its purpose, but it's not really random.
 */
public class RandomMoveStrategy extends MoveStrategy{
    @Override
    public boolean checkTile(Board board, Player player) {
        Tile[] availableTiles = board.getAvailableTiles();

        for (Tile availableTile : availableTiles){
            availableTile.check(player);
            return true;
        }

        return false;
    }
}

ForkStrategy

/**
 * Strategy to mark an available tile if it opens a fork win condition.
 * def. Fork: opens two different win paths.
 */
public class ForkStrategy extends MoveStrategy {
    @Override
    public boolean checkTile(Board board, Player player) {
        Tile[] availableTiles = board.getAvailableTiles();
        int winConditions = 0;

        for (Tile availableTile : availableTiles){
            availableTile.check(player);
            Tile[] futureAvailableTiles = board.getAvailableTiles();

            for(Tile futureAvailableTile : futureAvailableTiles){
                futureAvailableTile.check(player);

                if(isWinningMove(board, availableTile)){
                    winConditions++;
                    if(winConditions > 1){
                        futureAvailableTile.uncheck();
                        return true;
                    }
                }

                futureAvailableTile.uncheck();
            }

            availableTile.uncheck();
            winConditions = 0;
        }

        return false;
    }
}

BlockWinStrategy

/**
 * Strategy to mark an available tile only if it blocks an opponent win.
 */
public class BlockWinStrategy extends MoveStrategy {
   @Override
    public boolean checkTile(Board board, Player player){
        Player opponent = getOpponent(board, player);
        if(opponent == null) return false;

        Tile[] availableTiles = board.getAvailableTiles();
        for (Tile availableTile : availableTiles){
            availableTile.check(opponent);
            if(isWinningMove(board, availableTile)){
                availableTile.check(player);
                return true;
            }
            availableTile.uncheck();
        }

        return false;
    }
}

BlockForkStrategy

/**
 * Strategy to mark an available tile only if it blocks an opponent fork option.
 * def. Fork: opens two different win paths.
 */
public class BlockForkStrategy extends MoveStrategy {
    @Override
    public boolean checkTile(Board board, Player player) {
        Player opponent = getOpponent(board, player);
        if(opponent == null) return false;

        Tile[] availableTiles = board.getAvailableTiles();
        int winConditions = 0;

        for (Tile availableTile : availableTiles){
            availableTile.check(opponent);
            Tile[] futureAvailableTiles = board.getAvailableTiles();

            for(Tile futureAvailableTile : futureAvailableTiles){
                futureAvailableTile.check(opponent);

                if(isWinningMove(board, availableTile)){
                    winConditions++;
                    if(winConditions > 1){
                        futureAvailableTile.uncheck();
                        availableTile.check(player);
                        return true;
                    }
                }

                futureAvailableTile.uncheck();
            }

            availableTile.uncheck();
            winConditions = 0;
        }

        return false;
    }
}

CliOptionsParser

import core.Game;
import core.ai.RulesAI;
import core.players.Computer;
import core.players.Human;
import core.players.Player;
import org.apache.commons.cli.*;

public class CliOptionsParser {
    public static Game parseOptions(String[] args) throws ParseException {
        CommandLineParser parser = new DefaultParser();
        Options cmdOptions = getCmdOptions();
        CommandLine commandLine = parser.parse(cmdOptions, args);

        String p1Marker = commandLine.getOptionValue("p1");
        Player player1 = commandLine.hasOption("p1IsComputer") ?
                new Computer(p1Marker, new RulesAI()) :
                new Human(p1Marker);

        String p2Marker = commandLine.getOptionValue("p2");
        Player player2 = commandLine.hasOption("p2IsComputer") ?
                new Computer(p2Marker, new RulesAI()) :
                new Human(p2Marker);

        return new Game(player1, player2);
    }

    private static Options getCmdOptions(){
        Options options = new Options();

        //CLI options.
        options.addRequiredOption("p1", "player1Token", true, "Player1's token/marker.");
        options.addRequiredOption("p2", "player2Token", true, "Player2's token/marker.");
        options.addOption("p1IsComputer", false, "Sets player1 as controlled by the AI.");
        options.addOption("p2IsComputer", false, "Sets player2 as controlled by the AI.");

        return options;
    }
}

Application

import core.Game;
import core.ai.RulesAI;
import core.board.Board;
import core.exceptions.TicTacToeException;
import core.players.Computer;
import core.players.Human;
import core.players.Player;
import org.apache.commons.cli.*;

import java.util.Scanner;
import java.util.regex.Matcher;
import java.util.regex.Pattern;

public class Application {
    /**
     * Color constants to display the player symbols with different colours.
     */
    public static final String ANSI_RED = "\u001B[31m";
    public static final String ANSI_GREEN = "\u001B[32m";
    public static final String ANSI_RESET = "\u001B[0m";

    private static Scanner input = new Scanner(System.in);

    public static void main(String[] args) {
        Game game;

        // Initialize the game with its players and symbols from the CLI.
        try{
            game = CliOptionsParser.parseOptions(args);
        }
        catch (ParseException ex){
            System.out.println("An error has been found with the arguments: " + ex.getMessage());
            return;
        }

        chooseInitialPlayer(game);

        printGameBoard(game.getBoard());

        do {
            try {
                makeNextMove(game);
            }
            catch (Exception ex) {
                System.out.println(ex.getMessage());
                continue;
            }

            clearConsole();
            printGameBoard(game.getBoard());

            // Print end-game results.
            if(isGameEnded(game)) break;

            game.changeTurn();
        } while (!game.isBoardComplete());

        input.close();
    }

    private static void chooseInitialPlayer(Game game){
        System.out.print("Choose starting player: ");
        String symbol = input.nextLine();

        try {
            game.chooseInitialPlayer(symbol);
        }
        catch (TicTacToeException ex){
            System.out.println(ex.getMessage());
        }
    }

    private static int[] getHumanInput(){
        Scanner scanner = new Scanner(System.in);

        Matcher matcher;

        do{
            String input = scanner.next();

            String patternString = "^([0-9]),([0-9])$"; // NOTE: The number range (1-3) is being validated in the backend.
            Pattern pattern = Pattern.compile(patternString);
            matcher = pattern.matcher(input);

            if(!matcher.matches()){
                System.out.println("Invalid input. Please give the coordinates of the tile you want to check with the format \"row,column\"");
            }

        }while (!matcher.matches());

        return new int[]{ Integer.parseInt(matcher.group(1)), Integer.parseInt(matcher.group(2)) };
    }

    private static void makeNextMove(Game game) throws Exception{
        if (game.getCurrentPlayer() instanceof Human) {
            System.out.println("Enter the row and column of the tile you want to check:\n");
            int[] move = getHumanInput();
            game.makeMove(move[0], move[1]);

        } else {
            System.out.println("Please wait - the computer is thinking its next move.");
            game.makeMove();
            Thread.sleep(2000); // UX :-)
        }
    }

    private static boolean isGameEnded(Game game){
        if(game.hasCurrentPlayerWon()){
            System.out.println("Player " + game.getCurrentPlayer().getSymbol() + " has won!");
            return true;
        }
        if(game.isTie()){
            System.out.println("The game is tied!");
            return true;
        }

        return false;
    }

    private static void printGameBoard(Board board){
        String firstPlayer = board.getTile(0,0).getCheck();

        System.out.println(ANSI_RESET + "-------------");
        for(int i = 0; i < board.getRowCount(); i++){
            for(int j = 0; j < board.getColumnCount(); j++){
                System.out.print(ANSI_RESET + "| ");
                if(board.getTile(i, j).getCheck() == firstPlayer){
                    System.out.print(ANSI_GREEN + board.getTile(i, j).getCheck());
                }
                else{
                    System.out.print(ANSI_RED + board.getTile(i, j).getCheck());
                }
                System.out.print(" ");
            }
            System.out.print(ANSI_RESET + "|\n");
            System.out.println("-------------");
        }
    }

    private static void clearConsole() {
        System.out.print("\033[H\033[2J");
        System.out.flush();
    }
}
\$\endgroup\$
7
  • 1
    \$\begingroup\$ It's nice to give a link to github but please provide code as well here. Links are not eternal and it's nice to see the complete question and the response on the same page. Thank you. (Or provide only the code you want a review for if you find it may be too long) \$\endgroup\$ – Julien Rousé Jan 5 '18 at 11:05
  • \$\begingroup\$ Wouldn't it be a problem if the code in its complete form is like 3000 lines? :S \$\endgroup\$ – Javier García Manzano Jan 5 '18 at 11:07
  • \$\begingroup\$ Please take a look at the help center. The character limit is 65536, would that be enough? \$\endgroup\$ – Mast Jan 5 '18 at 11:15
  • \$\begingroup\$ @Mast fair enough :-P \$\endgroup\$ – Javier García Manzano Jan 5 '18 at 11:26
  • \$\begingroup\$ Can you cut your code into the files you actually use? Did you run into the character limit? If not, please add at least a list of the imports used. I can imagine doing this for all files would look noisy, but even then, it's usually preferred to be complete. \$\endgroup\$ – Mast Jan 5 '18 at 11:29
2
\$\begingroup\$

Thanks for sharing your code.

public Game(Player player1, Player player2){
    this.player1 = player1;
    this.player2 = player2;

    // Default - player 1 goes first unless changed later.
    chooseInitialPlayer(player1);

    board = new Board();
}

public void chooseInitialPlayer(Player player){
    currentPlayer = player;
}

Why is chooseInitialPlayer public?

It is dangerous to call a method from a constructor which is not private or at least final. It could be overwritten by a subclass an in this overridden subclass method you may access members of that subclass not yet initialized. That leads to hard to understand NPEs on final fields initialized in that subclass constructor.

On the other hand I doubt that this method needs to be called from any other class then the game itself. (I would create a new Game object for another round to play...)


public void changeTurn(){
    if(currentPlayer == player1){
        currentPlayer = player2;
    }
    else{
        currentPlayer = player1;
    }
}

This is a straight forward approach to the problem, just two thoughts:

  • Naming
    The method does not change a turn (what ever that may be...). It changes the current player. Therefore I'd name it changeCurrentPlayer().

  • alternative implementation
    What if you hold your players in a List<Player>? In that case switching the player would be like this:

    public void changeTurn(){
        currentPlayer = players.remove(0); // get first player from the list;
        players.add(currentPlayer); // enqueue the current player as last
    }
    

    Benefits are:

    • shorter
    • variables player1 and player2 are not needed anymore
    • works for any number of players. (OK, no killer for this game...)

Finally I'd discourage the use of arrays and rather suggest Collection types instead. But maybe that's just me...

\$\endgroup\$
2
\$\begingroup\$

I had a look to console.Application class and see this:

    Game game;

    // Initialize the game with its players and symbols from the CLI.
    try{
        game = CliOptionsParser.parseOptions(args);
    }
    catch (ParseException ex){
        System.out.println("An error has been found with the arguments: " + ex.getMessage());
        return;
    }

I think that here you should avoid to put knowledge of core.Game class in the console package.

I rather prefer to see code like this:

class Game {
    public Game(GameOptions options) { .... }
}

class GameOptions {
    public static GameOptions from(Options options) {
        GameOptions opts = new GameOptions();

        // validate options...

        return opts;
    }
}

class Application {
    public static void main(String args) {
        CliOptionsParser optionParser = new CliOptionsParser(args);

        Game game = new Game(GameOptions.from(optionParser));
        ...
    }
}

So you put all the initialization code in the main class and you decouple the console from the core package.

The point is that CliOptionsParse should care just of parsing command line options, and nothing more.

You should avouid to put there code to initialize objects.

The reason to introduce a specific class like GameOptions is an optional that allow the decoupling the core package from the console package.

If you decide to use this adapter class, consider to create a proper interface in order to separete the implementation too.

GameOptions should contain the options validation code connected to the game.

For example, if you pass just one player in the proper format, should be GameOptions to complain the missing player, not the CliOptionsParser, that should just care that the command line options are in a proper format.

This helps to place the business logic in the proper place.

\$\endgroup\$
1
  • \$\begingroup\$ This gave me a lot of insight! Thanks a lot :D \$\endgroup\$ – Javier García Manzano Jan 5 '18 at 12:23

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.