14
\$\begingroup\$

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.MIN_SPACE || 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 < winConditions.length; i++) {
            for(int j = 0; j < winConditions[i].length; j++) {
                winSpaces[i][j] = board.getSpace(winConditions[i][j]);
            }
        }
    }

    /**
     * Attempts to place a player on a space
     * @param space The space to place at
     * @throws OccupiedSpaceException The desired space is taken
     */
    public void place(int space) throws OccupiedSpaceException {
        if(!board.isFree(space)) {
            throw new OccupiedSpaceException();
        }

       board.getSpace(space).setOccupant(getTurn());
    }

    /**
     * Returns the Player whose turn it is
     * @return The Player whose turn it is
     */
    public Player getTurn() {
        return turn;
    }

    /**
     * Switches whose turn it is.
     *
     * If it's X's turn, it's now O's.
     * If it's O's turn, it's now X's.
     */
    public void switchTurns() {
        if(turn == Player.X) {
            turn = Player.O;
        } else if(turn == Player.O) {
            turn = Player.X;
        }
    }

    /**
     * Returns the winner if there is one
     * @return The winner or null
     */
    public Player getWinner() {
        for(int i = 0; i < winSpaces.length; i++) {
            Player[] occupants = {winSpaces[i][0].getOccupant(),
                                  winSpaces[i][1].getOccupant(),
                                  winSpaces[i][2].getOccupant()};

            if(occupants[0] == occupants[1] && occupants[1] == occupants[2]) {
                return occupants[0];
            }
        }
        return null;
    }

    /**
     * Returns the board associated with this game
     * @return The board
     */
    public Board getBoard() {
        return board;
    }
}

Board.java

package com.sirpython.tictactoe;

public class Board {
    public final int SIZE = 9;
    public final int ROW_SIZE = 3;

    private final Space[] spaces;

    public Board() {
        spaces = new Space[SIZE];

        for(int i = 0; i < SIZE; i++) {
            spaces[i] = new Space();
        }
    }

    /**
     * Displays the board in the form of a normal Tic Tac Toe board
     */
    public void display() {
        for(int i = 0; i < SIZE; i++) {

            Player occupant = spaces[i].getOccupant();

            /* If there is nothing occupying the space, print the space's number */
            char toPrint = occupant == null ? (char) (i + '0') : occupant.getSymbol();

            /* Print out a newline every third iteration so it looks like a tictactoe board */
            System.out.print((i % ROW_SIZE == 0 && i != 0 ? "\n" : "") + toPrint);
        }
    }

    /**
     * Tells if a space is occupied
     * @param space The number of the space to check
     * @return If the space is occupied
     */
    public boolean isFree(int space) {
        return spaces[space].getOccupant() == null;
    }

    /**
     * Returns the space
     * @param space The number of the space
     * @return The space
     */
    public Space getSpace(int space) {
        return spaces[space];
    }
}

Space.java

package com.sirpython.tictactoe;

public class Space {
    private Player occupant = null;

    public Space() {
    }

    /**
     * Sets the occupant field
     * @param occupant The new value for the field
     */
    public void setOccupant(Player occupant) {
        this.occupant = occupant;
    }

    /**
     * Gets the occupant field
     * @return The occupant field value
     */
    public Player getOccupant() {
        return occupant;
    }
}

Player.java

package com.sirpython.tictactoe;

public enum Player {
    X('x'), O('o');

    private final char symbol;
    private Player(char symbol) {
        this.symbol = symbol;
    }

    /**
     * Returns this Player's symbol
     * @return This Player's symbol
     */
    public char getSymbol() {
        return symbol;
    }
}

OccupiedSpaceException.java

package com.sirpython.tictactoe;

public class OccupiedSpaceException extends Exception {
    public OccupiedSpaceException() {
        super("That space is already occupied");
    }
}

Sample

Here is a sample game I played with myself:

012
345
678
It is x's turn.
0 <--
x12
345
678
It is o's turn.
4 <--
x12
3o5
678
It is x's turn.
6 <--
x12
3o5
x78
It is o's turn.
7 <--
x12
3o5
xo8
It is x's turn.
3 <--
Winner: x

Yay! I beat myself!

Note: The numbers pointed to by an <-- are user input.

Concerns

  • As questioned above, how is this code, OOP-wise? Do classes do what they should do and nothing else? Should some classes be refactored?

  • 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?

  • How is my documentation? I was using the IntelliJ IDEA, which automatically generates a docstring for me to fill out. However, are there any practices that I am missing? I've occasionally seen HTML tags being used.

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

  • Is my code idiomatic? 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?). Are there any classes that I could have used to simplify my code?

  • 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?

  • I created my own exception for handling occupied spaces. Would it have been better to use a built-in exception?

Any other things that jump out are encouraged.

\$\endgroup\$
5
\$\begingroup\$

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.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.