Tic Tac Toe with SOLID principles

Question

I am making a Tic Tac Toe game to be played in the console either against another human or a simple AI. It is a little rough but it works. What concerns me is how well it adheres to SOLID and other design principles as this is my first time using them in practice. I would appreciate a review of how my classes are structured and any advice. I am sharing a link from GitHub because there is too much code to post everything here.

These are some of the largest classes and give me an uneasy feeling.

GameBoard.class - has the game board and holds players' markers

package board;

import java.util.ArrayList;


public class GameBoard {

    private Cell[] board;

    private ArrayList<Integer> availableSpots;

    public GameBoard(int n) {
        availableSpots = new ArrayList<Integer>();

        //the game board
        board = new Cell[n];
        for (int i = 0; i < n; i++) {
            board[i] = new Cell();
        }

        //list of initial open cells
        for (int i = 0; i < 9; i++) {
            availableSpots.add(i);
        }
    }


    public Cell[] getBoard() {
        return board;
    }

    public void addMarker(int n, String s) {
        if (availableSpots.contains(n)) {
            board[n].setMarker(s);
            removeSpotFromList(n);
            //System.out.println(availableSpots.size());
        }
        else {
                System.out.println("Cell not available");

            }
    }

    public String getCellValue(int n) {
        return board[n].getMarker();
    }

    public ArrayList<Integer> getAvailableSpots() {
        return availableSpots;
    }

    //remove number from arraylist
    public void removeSpotFromList(int n) {
        availableSpots.set(n, null);
        availableSpots.trimToSize();
        for (int i=0; i<availableSpots.size(); i++) {
            //System.out.println(availableSpots.get(i) + ", ");
        }
    }

}

Game.class - switches between players and calls each turn

import board.GameBoard;
import board.PrintBoard;
import consuleInput.InputReader;
import import players.Playable;
import players.PlayerGroup;


public class Game {

    public static void play() {
        Modes modes =  new Modes();
        InputReader ir = new InputReader();
        GameBoard bg = new GameBoard(9);
        PrintBoard pb = new PrintBoard();
        PlayerGroup pg = new PlayerGroup();
        GameStateChecker gsc = new GameStateChecker();
        System.out.println("choose a mode:");
        System.out.println("1. Player vs player");
        System.out.println("2. player v computer");
        System.out.println("3. computer vs computer");

        int mode =  ir.toIntegerReader("type below");

        modes.chooseMode(mode, pg);

        pb.print(bg);

        int counter = 9;
        Playable currentPlayer;
        boolean hasWinner = false;
        int player = 0;
        //int choice;
        do{
            currentPlayer = pg.getPlayer(player);
            //int choice = ir.toIntegerReader("pick a square (0-8):");
            bg.addMarker(currentPlayer.playerMove(bg.getAvailableSpots()), currentPlayer.getMarker());
            pb.print(bg);
            hasWinner = gsc.checkPatterns(bg, currentPlayer);
            //System.out.println("is there a winner " + hasWinner);
            counter--;
            if (player == 0) {
                player = 1;
            }
            else {
                player = 0;
            }
            /*
            currentPlayer = pg.getPlayer(1);
            choice = ir.toIntegerReader("pick a square (0-8):");
            bg.addMarker(choice, currentPlayer.getMarker());
            pb.print(bg);
            hasWinner = gsc.checkPatterns(bg, currentPlayer);
            counter--;
            */
        }
        while (!hasWinner);


    }
}

Answer 1

Aside from RobAu's comment, I think the addMarker method can be simplified into:

board[n].setMarker(s);

By delegating the checking the availability of the cell to the Cell class. I think it would be best to define your own exception extending from IllegalArgumentException.

public void setMarker(String marker) {
    if(!marker.equals(" "))
        throw new CellIsNotAvailableException();
    this.marker = marker;
}

Another thing I want to point out is the way you name your methods and variables. One should always name their methods and variables properly such that it won't be neccessary to add a comment explaining what these things do (It's fine to write comments explaining why they exist though). As much as possible, Express yourself in code! Take addMarker for example. I think addMarker would be better written as markCellWithMarker(int cellNumber, String marker) (I consider X's and O's as tokens to be used in the game), and getCellValue as getMarkerAt(int cellNumber).

Applying these changes, here's my refactored version of the GameBoard Class:

public class GameBoard {

    private Cell[] board;

    public GameBoard(int numberOfCells) {
        board = new Cell[numberOfCells];
        for (int i = 0; i < numberOfCells; i++) {
            board[i] = new Cell();
        }
    }

    public void markCellWithMarker(int cellNumber, String token) {
        board[cellNumber].setMarker(token);
    }

    public String getMarkerAt(int cellNumber) {
        return board[cellNumber].getMarker();
    }
}

As for the Game class, it doesn't really justify its existence as it is right now. It contains only one method that is supposed to run a game (if I read your code right). Refactoring it into this might justify its existence:

public class Game {
    private GameBoard gameBoard;
    private Player[] players;
    private int turnNumber;

    //In case you want a two player game.
    public Game(int numberOfCells, Player player1, player player2){
        gameBoard = new GameBoard(numberOfCells);
        setFirstTurnPlayer(player1, player2);
    }

    //In case you want a single player game against the computer. 
    public Game(int numberOfCells, Player player){
        gameBoard = new GameBoard(numberOfCells);
        Player computer = new Player("computer");
        setFirstTurnPlayer(player1, computer);
    }

    private void setFirstTurnPlayer(Player player1, Player player2){
        //It's up to you how you decide the turnorder: die roll, coin toss or something.
        //The first one in the player array is considered as the turn player.
    }

    public String getTurnPlayer(){
        return players[turnNumber%2].getName();
    }

    public void markCellWithMarker(int cellNumber){
        String markerToPlace = players[turnNumber%2].getPlayerMarker()
        gameBoard.markCellWithMarker(cellNumber, markerToPlace)
        checkIfTurnPlayerWinsTheGame()
        turnNumber++;
    }

    //GameStateChecker code moved here. I think this one can be made shorter and more readable.
    public void checkIfTurnPlayerWinsTheGame(){
        //Rename currentPlayer as turnPlayer
        Player currentPlayer = players[turnNumber%2]
        if ((board[0].getMarker().equals(currentPlayer.getMarker()) && board[1].getMarker().equals(currentPlayer.getMarker()) && board[2].getMarker().equals(currentPlayer.getMarker())) ||
            (board[3].getMarker().equals(currentPlayer.getMarker()) && board[4].getMarker().equals(currentPlayer.getMarker()) && board[5].getMarker().equals(currentPlayer.getMarker())) ||
            (board[6].getMarker().equals(currentPlayer.getMarker()) && board[7].getMarker().equals(currentPlayer.getMarker()) && board[8].getMarker().equals(currentPlayer.getMarker())) ||
            (board[0].getMarker().equals(currentPlayer.getMarker()) && board[3].getMarker().equals(currentPlayer.getMarker()) && board[6].getMarker().equals(currentPlayer.getMarker())) ||
            (board[1].getMarker().equals(currentPlayer.getMarker()) && board[4].getMarker().equals(currentPlayer.getMarker()) && board[5].getMarker().equals(currentPlayer.getMarker())) ||
            (board[2].getMarker().equals(currentPlayer.getMarker()) && board[5].getMarker().equals(currentPlayer.getMarker()) && board[8].getMarker().equals(currentPlayer.getMarker())) ||
            (board[0].getMarker().equals(currentPlayer.getMarker()) && board[4].getMarker().equals(currentPlayer.getMarker()) && board[8].getMarker().equals(currentPlayer.getMarker())) ||
            (board[2].getMarker().equals(currentPlayer.getMarker()) && board[4].getMarker().equals(currentPlayer.getMarker()) && board[6].getMarker().equals(currentPlayer.getMarker()))) 
        throw new GameIsOverException(currentPlayer.getName() + " wins!")
}

Here's the code I wrote for a game called ConnectFour which I think is a bit similar to tic-tac-toe. This may help you see some of the things I mentioned here on proper naming and stuff.

Answer 2

Thank you for sharing your code.

Here are my thought on it:

Naming

Finding good names is the hardest part in programming, so always take your time to think about the names of your identifiers.

avoid single character names / abbreviations

Since the number of characters is quite limited in most languages you will soon run out of names. This means that you either have to choose another character which is not so obviously connected to the purpose of the variable. And/or you have to "reuse" variable names in different contexts. Both makes your code hard to read and understand for other persons. (keep in mind that you are that other person yourself if you look at your code in a few month!) The same is true for (uncommon) abbreviations. You might find them being obvious today, while you're actively dealing with the problem, but You might have to "relearn" them if you worked on something else for a while.

On the other hand in Java the length of identifier names names is virtually unlimited. There is no penalty in any way for long identifier names. So don't be stingy with letters when choosing names.

As usual there are some exceptions for this suggestion. E.g. the running index of loops is usually held in a single letter named variable like i in

for(int i =0; i<SOME_CONSTANT; i++)

But even there you should use more problem oriented names to support your readers, especially in nested loops like this:

for(int row =0; row<ENTRY_COUNT; row++)
   for(int column =0; column<ENTRY_FIELD_COUNT; column++)

Choose you names from the problem domain, not from the technical solution.

You have variable names like counter, n (as for number) and s (as for string). Better names for them might be like roundsLeft, selectedFiledIndex and playerMarker.

remove unused variables

In your method play() you have a variable player. You do nothing else with that variable then changing its value. There is nothing else depending on it, so remove it!

define variables as late as possible

In all you methods you define all variables right at the top of the method.

In Java this is not needed (as it is in C).

The problem with that is that it confuses your IDEs automated refactoring feature.

Magic numbers

your code has some magic numbers. This are literal values with a special meaning like here:

    GameBoard bg = new GameBoard(9);
    // ...
    //list of initial open cells
    for (int i = 0; i < 9; i++) {

You should introduce constants with meaningful names:

    GameBoard bg = new GameBoard(NUMBER_OF_FIELDS);
    // ...
    //list of initial open cells
    for (int i = 0; i < NUMBER_OF_FIELDS; i++) {

Single Layer Of Abstraction

This principle states that a method (or class) should either do simple/primitive calculations or (exclusive) call other methods (on other objects).

In your method play() you call some methods on other objects but you also modify the player variable with some primitive statements. You should extract this part out of method play() to a method of its own (wich would make it obvious that player is obsolete so that your IDE would tell you that).

Also you mix business logic with user interaction. You should separate that into specialized methods. This would give you the option to move that UI-methods in classes of theit own moving your architection foreward to the Model Vew Controller (MVC) Pattern.

don't use comments to "save" code

Modern IDEs keep a history of the last saved versions of your code files. They have capability to compare with this old versions and get them back (or parts of). Also using a source code management system like git or svn is quite easy for the same purpose.

So there is no need to keep "inactive code" as comments in your code files. just remove them.

Answer 3

Excess classes and interfaces

There is no big sence to have a Movable interface and classes ComputerMove and HumanMove. They may be included in the corresponding Playable classes. Even if you want to have them as separate classes then user shouldn't create them directly, let Playable classes do it in their constructors.

Strange method naming in ConsoleInput class

The fact that toIntegerReader method returns an int is counterintuitive. The better name for the method is readInt. The same for reader method - readString. input field assigned but not used.

Answer 4

[work in progress]

Properly encoded board state

Current you have a Cell class that contains a String that represent it's state.

I would advise against using String whenever possible. The main reason it that you cannot easily extend String or refactor your code. In this case, I would prefer an Enum

 public enum Marker { X, O, EMPTY }

Note: X and O are markers for players, EMPTY not. So you might also model this as an AbstractMarker and have a EmptyMarker and PlayerMarker subclass.

Then, the board is not 9 long, but 3x3. So, why not encode that in the Board directly? There is no big penalty in terms of performance or memory.

public Board {
  Cell[][] cells;
  int size;

  //Assume a square board.
  public Board (int size)
  {
      this.size=size;
      cells = new Cell[size][size];
      for (int i=0; i<size; i++) {
         for (int j=0; j<size; j++) {
         ...
  }
}

Methods for putting a marker on an location become more logical as well:

public putMarker ( Marker m, int x, int y )

Or, as I prefer:

public putMarker ( Marker m, Location loc )

Then the location can be implemented as you like and change as well, and the signature of putMarker won't change.

Getting the available spaces will be something like:

public Collection<Location> getAvailableSpaces() {
          Set<Location> result = new HashSet<>();
          for (int i=0; i<size; i++) {
             for (int j=0; j<size; j++) {
              if (cells[i][j] == Marker.EMPTY) {
                  result.add(new Location(i,j));
              }
             }
          }
          return result;
}

Note that for a HashMap you need a proper hashCode() and equals() method

Main game loop

• Determining the next player should be a method.
• counter is not used, so get rid of it.
• player should be a reference to the current Player instance.

BoardSize

Very useless class. What is wrong with a int in Board?

Winning condition check

Why is GameStateChecker a separate class? This can be easily code in a boolean hasWon(Player p) method in Board. You only need to check this after a player move, because you already keep track of the current player.

Printing the board

For such a small project, I would implement the printing of the board as having a sensible toString() on the Board class (this is useful for debugging as well), and just call System.out.println(board);.

If you want to get fancy you can make a BoardExporter or something and make that write the board to a Writer. Wrap the System.out in a PrintWriter and you have flexible printing options. But IMHO that is over-engineering for now.