Three in a row: Tic, Tac, Toe

The other day, I started thinking of a new personal project to start that I wanted to do in Java. As I started to do it, I realized that I was constantly deleting classes, creating new ones, merging them, etc. I finally have come to the conclusion that my understanding of OOP is not very strong.

To practice, I thought the best thing to do would be to start with something simple. For that, I chose the game Tic Tac Toe.

Main.java

package com.sirpython.tictactoe;

import java.util.Scanner;

public class Main {
    public static void main(String[] args) {
        TicTacToeGame game = new TicTacToeGame();
        Scanner input = new Scanner(System.in);

        while(true) {
            game.getBoard().display();
            System.out.println("\nIt is " + game.getTurn().getSymbol() + "'s turn.");

            int space = input.nextInt(10);

            if(space  game.MAX_SPACE) {
                System.out.println("Please choose a space 0-8");
                continue;
            }

            try {
                game.place(space);
            } catch(OccupiedSpaceException e) {
                System.out.println(e.getMessage());
                continue;
            }

            Player winner = game.getWinner();

            if(winner != null) {
                System.out.println("Winner: " + winner.getSymbol());
                break;
            }

            game.switchTurns();
        }
    }
}

TicTacToeGame.java

```
package com.sirpython.tictactoe;

public class TicTacToeGame {
public final int MIN_SPACE = 0;
public final int MAX_SPACE = 8;

private final int[][] winConditions = { {0,1,2}, {3,4,5}, {6,7,8}, {0,3,6}, {1,4,7}, {2,5,8}, {0,4,8}, {2,4,6} };
private final Space[][] winSpaces;

private Board board;
private Player turn;

public TicTacToeGame() {
board = new Board();
turn = Player.X;
winSpaces = new Space[winConditions.length][winConditions[0].length];

for(int i = 0; i < winC

How is code, OO-wise? Do classes do what they should do and nothing else? Should some classes be refactored?

The Main class sticks out like a sore thumb.
For one thing, "Main" doesn't mean anything.
TicTacToeRunner would be better.
More important, it has intimate knowledge of how the game works.
It would be better to move most of the content of Main.main into TicTacToeGame.play (a new static method I'm proposing), for example.
The TicTacToeRunner could be in charge of setting up input streams and passing what's necessary for TicTacToeGame.play.
This will also make it possible to unit test.


How is my handling of the win (TicTacToe.java)? Is it a good idea to have an entire variable dedicated to hold the square numbers that form a win condition so that it can be used to get the actual square/space? And, should this be in a static block rather than the constructor?

winConditions will be the same for every single instance.
So clearly it should be static.

I don't think winSpaces really buys you much.
It just takes space for little benefit if any.
I think you can refactor using just winConditions to determine a winner.


How is my documentation? [...] Are there any practices that I am missing? I've occasionally seen HTML tags being used.

Most of the docstrings are on getters and setters,
which are trivial.
I don't think a docstring is necessary at all for those.
The non-trivial cases are fine.

Not feeling the need for HTML in a docstring is a good thing. Simple.


How is my connection between the classes? As in, is the way that my classes are talking to each other wise? I am mainly concerned about the main methods interaction with TicTacToeGame, and that class's interaction with Board.

I already pointed out above about Main -> TicTacToeGame.

Chained calls like game.getBoard().display() in Main are a warning sign of poor information hiding: Main knows that Game has a Board, and the display method of the board. Calling from Main, game.display() would be better, hiding the internal details of game calling this.board.display().

If you move these operations out of Main into Game as I suggested,
then game.getBoard().display() becomes OK,
as you'll be inside the Game class,
so it's normal that it knows such internal details.

Actually, it would be best if game.getBoard() disappeared:
nobody else really needs to know about the board.
And if yes, probably a defensive copy should be returned to prevent tampering with the board outside of the game.


Is my code idiomatic?

More or less.

The loop in getWinner would be more idiomatic as an enhanced for-each.

The custom exception seems a bit over-engineered.
You could make place return a boolean to indicate success/failure.
I think that way would be better,
because it will do the job but simpler.


I am not very familiar at all with Java, so I often have the API docs up in my browser as I am programming (is this bad?).

I look at the JavaDoc all the time.
Occasionally the tutorials too, like this one.
And the source code too,
to know how stuff is implemented.
Is this bad?


If I were writing this in a non-OOP language, I probably would've stored the two players in an array and done some tricks (turn = (turn + 1) % 2) to find out whose turn it was. However, since this in an OOP language and it's Java, I thought it would be best to store the players in an enum. Is this the best choice, or would it have been better to create an entire class?

A traditional 2-player tic-tac-toe game has two players.
A more OOP way would have been that the game is initialized with two players,
and it would track them in separate fields,
for example firstPlayer, secondPlayer.

Also, some odd elements:

• A "symbol" is not a player. A symbol is a symbol, players use symbols

• A "turn" is not a player

I would prefer switching turns like this, given Player currentPlayer:

if (currentPlayer == firstPlayer) {
    currentPlayer = secondPlayer;
} else {
    currentPlayer = firstPlayer;
}

Any other things that jump out are encouraged.

A few other suggestions:

-
The "get" prefix is typically used for simple getters that just return the value of a member variable (or defensive copy). getWinner is more involved, so the "get" prefix is misleading. Perhaps calculateWinner or checkWinner would be better.

-
Instead of setOccupant and getOccupant I'd go for setPlayer and getPlayer

-
Is the empty constructor of Space necessary there?

-
In switchTurns, not having an else is odd. It would be better to convert the last else if to an else. Or you might add an else that throws IllegalStateException.

StackExchange Code Review Q#105410, answer score: 5