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) || 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();
}
}
