trjhtr

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

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) 

 /**
 * 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 

 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)

 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 + "
 System.out.print(ANSI_RESET + "
 

 private static void clearConsole() 
 System.out.print("33[H33[2J");
 System.out.flush();
 

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

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.