Tic-Tac-Toe classes according to SOLID principles

Question

I have these classes for my Tic-Tac-Toe game project. I am very new to object oriented analysis and design. Can anyone help determine if SOLID principles are preserved in this code or if it is violating some of them?

public class InputReader {

    private BufferedReader inputReader;

    public InputReader() {
        inputReader = new BufferedReader(new InputStreamReader(System.in));
    }

    public String getUserInput(String messagePrompt) {
        String userInput = null;
        System.out.println(messagePrompt);
        try {
            userInput = inputReader.readLine();
        } catch (IOException e) {
            e.printStackTrace();
        }
        return userInput;
    }

    public void close() {
        try {
            inputReader.close();
        } catch (IOException e) {
            e.printStackTrace();
        }
    }

}


public class Board {

    private static final int SIZE = 3;

    private int occupiedCells = 0;

    private char[][] board = new char[SIZE][SIZE];

    private Set<String> validCells;

    public Board() {
        validCells = new HashSet<String>();
        for (int row = 0; row < SIZE; row++) {
            for (int column = 0; column < SIZE; column++) {
                board[row][column] = ' ';
                validCells.add(row+""+column);
            }
        }
    }

    public void print() {
        int column, row;
        for (row = SIZE - 1; row >= 0; row--) {
            System.out.print(row + " ");
            for (column = 0; column < SIZE; column++) {
                System.out.print("|" + board[row][column]);
            }
            System.out.println("|");
        }
        System.out.print("   ");
        for (column = 0; column < SIZE; column++) {
            System.out.print(column + " ");
        }
        System.out.println();

    }

    public void markCell(char playerMark, int row, int column) {
        if (board[row][column] != ' ') {
            throw new CellOccupiedException("Error..!!! Cell already occupied.");
        } else {
            board[row][column] = playerMark;
            occupiedCells++;
        }
    }

    public boolean hasWinner() {
        return (checkRowsForWinner() || checkColumnsForWinner() || checkDiagonalsForWinner());
    }

    public boolean checkRowsForWinner() {
        for (int row = 0; row < SIZE; row++) {
            if (checkRow(row)) {
                return true;
            }
        }
        return false;
    }

    public boolean checkColumnsForWinner() {
        for (int column = 0; column < SIZE; column++) {
            if (checkColumn(column)) {
                return true;
            }
        }
        return false;
    }

    public boolean checkDiagonalsForWinner() {
        return checkFirstDiagonal() || checkSecondDiagonal();
    }

    private boolean checkRow(int row) {
        char cell1 = board[row][0];
        int column = 1;
        while (column < SIZE) {
            if (cell1 == ' ' || cell1 != board[row][column]) {
                return false;
            }
            column++;
        }
        return true;
    }

    private boolean checkColumn(int column) {
        char cell1 = board[0][column];
        int row = 1;
        while (row < SIZE) {
            if (cell1 == ' ' || cell1 != board[row][column]) {
                return false;
            }
            row++;
        }
        return true;
    }

    private boolean checkFirstDiagonal() {
        char cell1 = board[0][0];
        int rowColumn = 1;
        while (rowColumn < SIZE) {
            if (cell1 == ' ' || cell1 != board[rowColumn][rowColumn]) {
                return false;
            }
            rowColumn++;
        }
        return true;
    }

    private boolean checkSecondDiagonal() {
        char cell1 = board[0][2];
        int row = 1;
        int column = 1;
        while (row < SIZE) {
            if (cell1 == ' ' || cell1 != board[row][column]) {
                return false;
            }
            row++;
            column--;
        }
        return true;
    }

    public boolean isBoardFull() {
        return SIZE*SIZE == occupiedCells;
    }

    public void checkForValidCell(String inputCell) {
        if(validCells.contains(inputCell)){
            return;
        }else{
            throw new InvalidInputException("Error..!!! Invalid Input");
        }
    }

}


public class Player {

    private String name;

    private char mark;

    public Player(String name, char mark) {
        this.name = name;
        this.mark = mark;
    }

    public String getName() {
        return name;
    }

    public char getMark() {
        return mark;
    }
}


public class GameRunner {
    Board board;
    Player player1;
    Player player2;
    InputReader inputReader;

    public GameRunner() {
        inputReader = new InputReader();
        player1 = new Player(inputReader.getUserInput("Enter Player Name 1"), 'O');
        player2 = new Player(inputReader.getUserInput("Enter Player Name 2"), 'X');
        board = new Board();
    }

    public void play() {
        String playerInput = null;
        int turnCount = 1;
        String winnerOrDraw = "Game Drawn..!!";
        boolean keepPlaying = true;
        Player currentPlayer;
        while (keepPlaying) {
            currentPlayer = turnCount % 2 == 1 ? player1 : player2;
            while (playerInput == null) {
                board.print();
                playerInput = readAndValidateUserInput(currentPlayer.getName(),
                        currentPlayer.getMark());
            }
            if (board.hasWinner()) {
                winnerOrDraw = currentPlayer.getName() + " wins..!!";
                keepPlaying = false;
            } else if (board.isBoardFull()) {
                keepPlaying = false;
            }
            playerInput = null;
            turnCount++;
        }
        printResult(winnerOrDraw);

    }

    private String readAndValidateUserInput(String userName, char userMark) {
        String playerInput = null;
        try {
            playerInput = inputReader.getUserInput(userName
                    + " turn. Enter <row><col>. Eg. 20 for top left cell.");
            board.checkForValidCell(playerInput);
            markBoardCell(userMark, playerInput);
        } catch (CellOccupiedException wrongCell) {
            System.out.println(wrongCell.getErrorMessage());
            playerInput = null;
        } catch (InvalidInputException invalidInput) {
            System.out.println(invalidInput.getErrorMessage());
            playerInput = null;
        }
        return playerInput;

    }

    private void markBoardCell(char mark, String playerInput) {
        board.markCell(mark, Integer.parseInt(playerInput.substring(0, 1)),
                Integer.parseInt(playerInput.substring(1)));

    }

    private void printResult(String winnerOrDraw) {
        board.print();
        System.out.println("Result: " + winnerOrDraw);
        System.out.println("Press Enter key to exit...");
        try {
            System.in.read();
        } catch (IOException e) {
            e.printStackTrace();
        }finally{
            inputReader.close();
        }

    }

}

Answer 1

Well... You can always be very dogmatic about programming principles. Your code looks rather good to me. Anyway, I'll name some examples.

Single responsibility principle:

class Board 

What is the responsability of the Board-class? A board is supposed to hold some sort of players and objects for a game. In this case: The 'X's and 'O's. Hold them, keep them, no more, no less. Your class however does the following:

• Hold marks of both players (Yey!)

• Print the board

• Check if someone has won

• Holds static information about the game size (SIZE-Field)

  I see four responsabilities right here. Think about putting them into BoardPrinter, GameStateChecker and GameSize-classes (What if you'd like to have a 4x4-Board one day?).

Open/Closed-Principle:

Lets stay with your Board-class. As mentioned in the SRP-Section, it contains the field SIZE. In case your requirements change and you want a differently shaped board, you violate the Open/Closed-Principle. You have to modify the Board-class. If you change the static field to a parameter in the board constructor however, you can easily fulfill this principle:

private int boardEdgeLength;
public Board(int boardEdgeLength) {
    this.boardEdgeLength = boardEdgeLength;
    ...
}

Hint: You could improve this a lot by using a GameSize-class as mentioned above. Furthermore you could use the builder-pattern to create a board of any imaginable shape (How about 3D Tic-Tac-Toe?).

Liskov substitution principle:

Doesn't apply here, since you don't have any subtypes.

Interface segregation principle:

Doesn't apply here, since you don't have any interfaces. Or does it? Maybe you should use interfaces. For example your InputReader-class, could be an implementation of an interface. Imagine the following interface:

public interface InputReader{
    public Position getUserInput(Player playerToGetInputFrom);
}

And your implementation as:

public class ConsoleInputReader implements InputReader,AutoCloseable{...}

And how funky would be a:

public class GamepadInputReader implements InputReader{...}

Dependency inversion principle:

Dependency inversion is awesome. Imagine if your class GameRunner didn't even know which game is being played? It just says "There is a number of players, a game, and a mechanism that decides who's turn it is". It gets these three objects as parameters and boom. Completly reusable for any turn-based-game.

Another example: Your class Player. It should be an interface that is being implemented by either HumanPlayer or BotPlayer. Your GameRunner gets two Objects, that implement the Player-interface and does not need to know whether one of them is a bot. Or both.

I hoped I gave you a little insight on how you can improve your code according to the SOLID-Principles. If you have further questions (which are not "can you program this for me"), don't hesitate asking.

Answer 2

This looks well designed and implemented. Here are a couple things I noticed:

Redundant checking:

In checkRow(int), you redundantly check whether the first square is ' ' every time. I would check this first, then check whether the rest of the cells are equal to it:

if (cell1 == ' ') { return false; }

Looping over a known range:

for loops are designed for looping over a known range, and while loops are designed for looping over an unknown range until a condition is met. So, instead of:

int column = 1;
while (column < SIZE) {
    if (cell1 == ' ' || cell1 != board[row][column]) {
        return false;
    }
    column++;
}

You can use a for loop:

for (int column = 1; column < SIZE; column++)
{
    if (cell1 != board[row][column]) {
        return false;
    }
}

But here you are iterating over an array, and could possibly make use of the enhanced for:

for (char cell : board[row])
{
    if (cell == ' ' || cell != board[row][0]) {
        return false;
    }
}

This way, you have the redundant check that each cell is not ' ', but otherwise, the syntax is cleaner. This is just something to consider.

Common Exceptions:

throw new InvalidInputException("Error..!!! Invalid Input");

Exceptions are supposed to be thrown over uncommon occurrences. If the user enters the input with the keyboard, incorrect values are going to be common occurrences and should not throw an exception. I would just loop without an exception until the user input the correct value.

Answer 3

Concerning the InputReader class:

Since this class is a wrapper for a BufferedReader and already contains close() method, it would be better if it implemented the AutoCloseable interface. This will allow the users of this class (GameRunner) to benefit from try-with-resources block and they will not need to call close() explicitly.

If an IOException is thrown in getUserinput method (which would hardly occur here, but imagine that System.in is replaced by another source), the Player objects created in GameRunner constructor will become inconsistent, with name=null. Probably, that is not what was intended. Returning null in this context is a replacement for an exceptional situation and is not a valid value for the calling routines. I'd suggest to avoid it here (as well as in general) and to re-throw the exception, which should be caught and managed outside. The resulting class will look as follows:

public class InputReader implements AutoCloseable {

  private final BufferedReader inputReader;

  public InputReader() {
    inputReader = new BufferedReader(new InputStreamReader(System.in));
  }

  public String getUserInput(String messagePrompt) throws IOException {
    System.out.println(messagePrompt);
    return inputReader.readLine();
  }

  @Override
  public void close() {
    // IOException may also be rethrown here
    try {
      inputReader.close();
    } catch (IOException e) {
      e.printStackTrace();
    }
  }
}

Consequently, some changes will be necessary in GameRunner class, concerning the initialization of the InputReader instance and exceptions handling.