7
\$\begingroup\$

Background: As a fun exercise to work on my grasp of the OOP paradigm (my experience learning programming the past 2+ years has focused mainly on C), I wrote a basic implementation of Tic Tac Toe in Java. However, I don't want to just write C code in a Java file. I've read through other questions on here for Tic Tac Toe in Java, gleaning wisdom from them and improving what I had already written. However, I have several questions regarding my version.

  1. I tried to limit the logic contained in my runner class because writing code in main seems to be the way to use Java "improperly" vis-a-vis OOP. Should I implement a play method in my TicTacToe class in order to contain all the gameplay logic within my TicTacToe object? My gut is to move that direction in order to also eliminate the awkwardness of my getters, but I would appreciate feedback on how and where to draw the line for what belongs in a class and what belongs in a respective runner class.
  2. My checkIfWon() method works to check for a winning line in the grid, but as I've seen in other examples here, an alternate approach is to write three helper methods to check rows, columns, and diagonals, respectively. At what point does the logic in a public method necessitate private methods? I find the logic clear and readable for checking each in turn, and 4 methods doing the job of 1 seems more work than necessary. Yet, if a method is to do one thing, and that one thing is really three things, then maybe I should separate it. I'm conflicted on this one and would like insight as to the preferred approach here.
  3. I currently have three instance variables for the grid itself, the number of turns taken, and the overall game status. In my one helper method, I declare a local variable for the validity of a move. The checkIfWon() method updates the appropriate field while the validateMove() method creates and returns its own variable for the validity of a given move. This seems potentially inconsistent to me. If I am not returning values to my runner class, should I make everything a private instance variable rather than a local method variable? To me, move validity is not inherent to the game as a whole in the same that my three fields are.

There are other features I'd like to add (taking in user names, restarting, playing the computer, etc.), so my concern is not with expansion/improvement of gameplay itself (yet). Indeed, before this grows any further, I want to make sure I'm approaching the code with the right object-oriented approach. I would ask for general OOP feedback, but in my reading of the help center, I saw that that was too vague a request. However, I welcome any feedback not related to the questions above that I should note as I work more with Java.

TicTacToe.java

import java.util.Arrays;

public class TicTacToe
{
    private char[][] grid;
    private int turnNumber;
    private boolean gameWon;

    public TicTacToe()
    {
        grid = new char[3][3];
        for (char[] row : grid)
        {
            Arrays.fill(row, ' ');
        }

        turnNumber = 0;
        gameWon = false;
    }

    public int getTurnNumber()
    {
        return turnNumber;
    }

    public boolean getGameIsWon()
    {
        return gameWon;
    }

    public void makeMove(int row, int col)
    {
        if (validateMove(row, col))
        {
            if (turnNumber % 2 == 0)
            {
                grid[row][col] = 'X';
            }
            else
            {
                grid[row][col] = 'O';

            }
            turnNumber++;
        }
    }

    public void printGrid()
    {
        for (int row = 0; row < grid.length; row++)
        {
            for (int column = 0; column < grid[row].length; column++)
            {
                switch (column)
                {
                    case 0:
                        System.out.print(" " + grid[row][column] + " | ");
                        break;
                    case 1:
                        System.out.print(grid[row][column] + " | ");
                        break;
                    case 2:
                        System.out.println(grid[row][column]);
                        break;
                }
            }
            if (row < grid.length - 1)
            {
                System.out.println("-----------");
            }
        }
    }

    public void checkIfWon()
    {
        if (turnNumber >= 5)
        {
            for (int i = 0; i < grid.length; i++)
            {
                if (grid[0][i] != ' ' && grid[0][i] == grid[1][i] && grid[1][i] == grid[2][i])
                {
                    gameWon = true;
                }
                if (grid[i][0] != ' ' && grid[i][0] == grid[i][1] && grid[i][1] == grid[i][2])
                {
                    gameWon = true;
                }
            }
            if (grid[1][1] != ' ' && ((grid[0][0] == grid[1][1] && grid[1][1] == grid[2][2]) ||
                                      (grid[0][2] == grid[1][1] && grid[1][1] == grid[2][0])))
            {
                gameWon = true;
            }
        }
    }

    public void printFinalStatus()
    {
        if (gameWon)
        {
            if (turnNumber % 2 != 0)
            {
                System.out.println("X is the champion");
            }
            else
            {
                System.out.println("O is the champion");
            }
        }
        else
        {
            System.out.println("Draw");
        }
    }

    private boolean validateMove(int row, int col)
    {
        boolean validMove = true;

        if (row < 0 || col < 0 || row > 2 || col > 2)
        {
            System.out.println("ERROR: Invalid location.\nTry again.");
            validMove = false;
        }
        else if (!(grid[row][col] == ' '))
        {
            System.out.println("ERROR: Spot taken.\nTry again.");
            validMove = false;
        }

        return validMove;
    }
}

TicTacToeRunner.java

import java.io.*;
import java.util.*;

public class TicTacToeRunner
{
    public static void main(String args[])
    {
        TicTacToe game = new TicTacToe();

        Scanner readIn = new Scanner(System.in);

        while (game.getTurnNumber() < 9 && !game.getGameIsWon())
        {
            System.out.print("Row: ");
            int row = readIn.nextInt();
            System.out.print("Column: ");
            int col = readIn.nextInt();

            game.makeMove(row, col);
            game.printGrid();
            game.checkIfWon();
        }

        game.printFinalStatus();
    }
}
\$\endgroup\$
  • \$\begingroup\$ Honestly your example seems too simple to me to provide useful feedback. It's not very OOP, but it's also fine the way it is, because it's very simple. Q's 1 & 2: I think it's fine because the code you have is simple and easy to understand. Nothing else is really important in code. Q 3: Local variables should generally be preferred, imo. Don't make instance (or especially static) fields without a good reason. OOP & OOD is for larger problems; simple problems should be solved simply. My 2 bits. \$\endgroup\$ – markspace Nov 28 '17 at 4:37
  • 2
    \$\begingroup\$ @markspace I disagree completely. You can't learn climbing by climbing mount everest first, without knowing how to use a snap hook. \$\endgroup\$ – slowy Nov 29 '17 at 18:52
  • \$\begingroup\$ @slowy Well, that's fine. However I don't think that learning about a snap hook will teach you how to climb. Most folks start on a climbing wall, or a designated beginner climb. In other words you don't have to make a "Mt. Everest" project to learn, but projects do have to be a little larger than the example presented. A small text game or "office" project that used some Swing components would be better way to understand the zeitgeist of OOP. \$\endgroup\$ – markspace Nov 29 '17 at 19:56
7
+25
\$\begingroup\$

One thing you should look into is separating the game play more from the front-end by moving all output (System.out.println(...)) into the runner object.

Conversely the runner should have as little knowledge about the game play as possible. For example, it's shouldn't need to know (or even check) that the game is over after 9 turns.

This allows you to use the main game object unchanged with a different front-end.

Instead of having a method getGameIsWon() have a method getGameIsOver() (or more Java like: isGameOver()).

Also the method checkIfWon() is unnessecary. The game object should know (calculate) the game status directly automatically after making a move.

So the main game loop should look like something like this:

while (!game.isGameOver()){
  System.out.print("Row: ");
  int row = readIn.nextInt();
  System.out.print("Column: ");
  int col = readIn.nextInt();

  game.makeMove(row, col);

  // Printing the game is task of the runner. The game object will
  // need to provide (read only) access to the board, so it can do it.
  printGrid(game);
}

// Again task of the the runner to print the result. The game object 
// should only provide the information needed.
printFinalStatus(game.getWinner());

One thing you should look into (independently from OOP) is to avoid "magic numbers (and strings)" , in this explicit case the size of the grid, the number of players and the player symbols.

Write the whole thing in a way that you can easily change either of those things at a single place, e.g. as constants or constructor arguments. For example:

// Public because the runner needs the information, for example in
// order to print the grid.
public static final GRID_SIZE = 3;
private static final PLAYERS = new char[]{'X', 'O'};
private static final PLAYER_COUNT = PLAYERS.length;

Instead of calculating the current player "on the fly" with the modulo operator have a method such as:

public char getCurrentPlayer() {
   return PLAYERS[turnNumber % PLAYER_COUNT];
}

so that makeMove becomes.

public void makeMove(int row, int col)
{
    if (validateMove(row, col))
    {
        grid[row][col] = getCurrentPlayer();
        turnNumber++;
    }
}

One thing you could attempt to make the whole thing more OO is represent the players with objects or even take it one step further with an interface. This would be however completely over-engineered for a case like this. Just see it as an example for a typical OO pattern.

Currently the only information you need about a player is their symbol, so you could use an interface such as:

interface Player {
   char getSymbol();
}

Use only this interface within the game class e.g.:

private final Player[] players;
private Player[][] grid;

public TicTacToe(Player[] players)  {
    this.players = players;
    this grid = new Player[3][3];

    for (Player[] row : grid)
    {
        Arrays.fill(row, null); // (*) See below 
    }
 }

Now the Runner class has to create an array of objects that implement the player interface. One very basic way could be using anonymous classes:

Player player1 = new Player() {
   @Override 
   public char getSymbol() { return 'X'; }
};

Player player2 = new Player() {
   @Override 
   public char getSymbol() { return 'O'; }
};

TicTacToe game = new TicTacToe(new Player[] {player1, player2});

Or you could create a simple implementation class such as:

class PlayerImplementation implements Player {
    private char symbol;

    public PlayerImplementation(char symbol) {
        this.symbol = symbol;
    }

    @Override
    public getSymbol() { return symbol; }
}

TicTacToe game = new TicTacToe(new Player[] {new PlayerImplementation('X'), new PlayerImplementation('O')});

or maybe in the form of an enumeration

enum PlayerEnum implements Player {
   X('X'), O('O');

   public PlayerEnum(char symbol) {
        this.symbol = symbol;
    }

    @Override
    public getSymbol() { return symbol; }
}

TicTacToe game = new TicTacToe(PlayerEnum.values());

In any case the actual implementation doesn't matter as long as you only use the Player interface inside the game class.

Try extending that, for example, with a player name by giving Player a getName() method which you use inside the game class.


(*) Regarding the use of null: First off, the initialization of the array is not necessary, since by default they are already filled with null.

Generally in Java you have to watch out when handling classes not at access an obejct property when it could be null. You need to keep track in your code where null could come up and handle the cases accordingly.

One way is to avoid using null at all, if possible. One solution for that could be the use of a "null object", for example:

class TicTacToe {

   private static final NO_ONE = new Player() {
       @Override 
       public char getSymbol() { return ' '; }
    };

    public TicTacToe(Player[] players)  {
        // Make sure you don't get `null` from outside, then for here on
        // you know there can't be any nulls in the players array any 
        // no longer have to check for them.
        for (Player player: players) {
          if (player == null) {
            throw new IllegalArgumentException("Player array may not contain 'null'.");
          }
        }

        this.players = players;
        this grid = new Player[3][3];

        for (Player[] row : grid)
        {
            Arrays.fill(row, NO_ONE ); 
        }
     }


}

\$\endgroup\$
  • \$\begingroup\$ Please don't use interfaces for this simple task. Don't make code more complex for no gain. Keep it simple. At the fancy stuff IF and only if you need it (you probably never will). \$\endgroup\$ – Emily L. Nov 28 '17 at 14:39
  • \$\begingroup\$ @EmilyL. You are completely right. I originally wanted to write a paragraph (but forgot) on how this is just an example of the patterns you can use in OOP and that it's totally overblown for this case. \$\endgroup\$ – RoToRa Nov 28 '17 at 15:11
  • \$\begingroup\$ @EmilyL. I've just added the explanation now. \$\endgroup\$ – RoToRa Nov 28 '17 at 15:16
2
\$\begingroup\$

Here is a somewhat different factorization of @Peter's code. It has five abstractions.

ArrayLocation: the row and column numbers captured in an object.

ArrayPlayer: an object that can play a symbol in an array based game.

TicTacToeBoard: manages the array aspects of the game and allows ArrayPlayers to play at ArrayLocations.

TicTacToe: manages the basic operations of the game.

TicTacToeGame: main - runs a single game

There are a few things that might be made better on a future edit. Some are noted in comments.

Note that I changed the user input rows and columns to be 1..3 based, which is a more common way of thinking about the game for most players. Internally usual array indices are used.

ArrayLocation

package tictactoe.peter;

/**
 * Represents a location in a two dimensional array. It is immutable. The
 *  upper left corner has (0, 0) coordinates. 
 *  No check for validity is done.
 * 
 * @author Buffy
 *
 */
public class ArrayLocation {
    private int row;
    private int column;

    public ArrayLocation(int row, int column) {
        this.row = row;
        this.column = column;
    }

    /** A row in the array with 0 as the first row
     * @return the row number - zero based
     */
    public int row() {
        return this.row;
    }

    /** A column in the array with 0 as the first column
     * @return the column number - zero based
     */
    public int column() {
        return this.column;
    }
}

ArrayPlayer

package tictactoe.peter;

import java.util.Scanner;

/** A player in an array based interactive game. The player can read a 
 * location in which to play and has a symbol that it can utilize 
 * at that location. It reads from aScanner for the location. 
 * It does not, however, remember the location. 
 * 
 *  @author Buffy, Peter
 */
public class ArrayPlayer {
    private char symbol;
    private Scanner input;

    public ArrayPlayer(char symbol, Scanner input) {
        this.symbol = symbol;
        this.input = input;
    }

    /** The symbol that the player uses
     * @return the symbol of this player
     */
    public char symbol() {
        return this.symbol;
    }

    /** Prompt a user for a row to play (1..n) and a column (1..n). 
     * The validity is not checked here. 
     * 
     * @return an ArrayLocation object with (0..n-1) coordinates. 
     */
    public ArrayLocation getMove() {
        System.out.print("Row: ");
        int row = input.nextInt();
        System.out.print("Column: ");
        int column = input.nextInt();
        return new ArrayLocation(row - 1, column - 1);
    }
}

TicTacToeBoard

package tictactoe.peter;

import java.util.Arrays;

/** Represents a 3 by 3 board for a tic tac toe game.
 * 
 * @author Buffy, Peter
 *
 */
public class TicTacToeBoard {

    private int size = 3;
    private char[][] grid = new char[size][size];

    /** Create a 3 by 3 Tic Tac Toe board
     * 
     */
    public TicTacToeBoard() {
        for (char[] row : grid) {
            Arrays.fill(row, ' ');
        }
    }

    private boolean validLocation(ArrayLocation location) {
    return location.row() >= 0 && 
        location.row() < size && 
        location.column() >= 0 && 
        location.column() < size;
    }

    /** Marks the board with the player's symbol if the location is valid. 
     * Otherwise a no-op.
     * @param player the current player
     * @param location the location to play
     */
    public void play(ArrayPlayer player, ArrayLocation location) {

        if (validLocation(location)) {
            grid[location.row()][location.column()] = player.symbol();
        }
    }

    /** Does the board show a winner?
     * @return whether the board has a winner. 
     */
    public boolean haveWinner() {
        // Assumes the size is 3. Could use a fixup.
        boolean result = false;
        for (int i = 0; i < size; i++) {
            if (grid[0][i] != ' ' && grid[0][i] == grid[1][i] && grid[1][i] == grid[2][i]) {
                result = true;
            }
            if (grid[i][0] != ' ' && grid[i][0] == grid[i][1] && grid[i][1] == grid[i][2]) {
                result = true;
            }
        }
        if (grid[1][1] != ' ' && ((grid[0][0] == grid[1][1] && grid[1][1] == grid[2][2])
                || (grid[0][2] == grid[1][1] && grid[1][1] == grid[2][0]))) {
            result = true;
        }

        return result;
    }

    /** Is the location a valid place for a move
     * @param location a location that may be on the board or not
     * @return whether the location is on the board and has not been
     * played
     */
    public boolean isValidAndFree(ArrayLocation location) {
        boolean validMove = true;
        int row = location.row();
        int col = location.column();

        if (row < 0 || col < 0 || row >= size || col >= size) {
            System.out.println("ERROR: Invalid location.\nTry again.");
            validMove = false;
        } else if (!(grid[row][col] == ' ')) {
            System.out.println("ERROR: Spot taken.\nTry again.");
            validMove = false;
        }

        return validMove;
    }

    /** Print a representation of the board to standard output.
     * 
     */
    public void printGrid() {
        // assumes size = 3. Fixup?
        for (int row = 0; row < grid.length; row++) {
            for (int column = 0; column < grid[row].length; column++) {
                switch (column) {
                case 0:
                    System.out.print(" " + grid[row][column] + " | ");
                    break;
                case 1:
                    System.out.print(grid[row][column] + " | ");
                    break;
                case 2:
                    System.out.println(grid[row][column]);
                    break;
                }
            }
            if (row < grid.length - 1) {
                System.out.println("-----------");
            }
        }
    }

}

TicTacToe

package tictactoe.peter;

/** Manages basic operations for a Tic Tac Toe game
 * 
 * @author Buffy, Peter
 *
 */
public class TicTacToe {
    private TicTacToeBoard board = new TicTacToeBoard();

    private int turnNumber = 0;
    private boolean gameWon = false;

    /** Determine whether another move is possible
     * @return true if a move can be made
     */
    public boolean moveAvailable() {
        return !(turnNumber >= 9 || gameWon);
    }

    /** A player attempts to make a move
     * @param player the player trying to move
     * @param location the board location of the move
     * @return true if the move is successful
     */
    public boolean makeMove(ArrayPlayer player, ArrayLocation location) {
        boolean success = validateMove(location);
        if (success) {
            board.play(player, location);
            turnNumber++;
        }
        return success;
    }

    /** Print a representation of the board to standard output. 
     * 
     */
    public void printBoard() {
        board.printGrid();
    }

    /** Check to see if the game has been won and remember the result. 
     * 
     */
    public void checkIfWon() {
        if (turnNumber >= 5) {
            gameWon = board.haveWinner();
        }
    }

    /** Print what happened in the game
     * @param player the last player that moved. 
     */
    public void printFinalStatus(ArrayPlayer player) {
        if (gameWon) {
            System.out.println(player.symbol() + " is the champion");
        } else {
            System.out.println("Draw");
        }
    }

    /** Determine if a location represents a valid move
     * @param location the location to be checked
     * @return true if the location is on the board and empty
     */
    private boolean validateMove(ArrayLocation location) {
        return board.isValidAndFree(location);
    }
}

TicTacToeGame

package tictactoe.peter;

import java.util.*;

/** Play a standard game of tic tac toe. 
 * 
 * @author Buffy
 *
 */
public class TicTacToeGame {

    public static void main(String args[]) {
        TicTacToe game = new TicTacToe();

        Scanner readIn = new Scanner(System.in);

        ArrayPlayer xPlayer = new ArrayPlayer('X', readIn);
        ArrayPlayer oPlayer = new ArrayPlayer('O', readIn);

        ArrayPlayer player = xPlayer;
        ArrayPlayer other = oPlayer;

        while (game.moveAvailable()) {
            ArrayLocation where = player.getMove();

            boolean success = game.makeMove(player, where);
            game.printBoard();
            game.checkIfWon();
            if (success) {
                // swap the players for next round
                ArrayPlayer temp = other;
                other = player;
                player = temp;
            }
        }

        game.printFinalStatus(other); // already swapped
        readIn.close();
    }
}

Note that it would be fairly easy to remove variable turnNumber from TicTacToe leaving almost all manipulation of primitives of Java in class TicTacToeBoard. Nearly everything else is a higher level abstraction.

I've added a package and javadocs (which should always be present) and changed the position of initial braces to a more common standard - that you may love or hate.

\$\endgroup\$
1
\$\begingroup\$

Some additions to the other answers:

In those circumstances, it's usually always a good idea to ask yourself the question: If I were to exchange the presentation layer, what part has to be changed. In your case, it would be printGrid(), printFinalStatus() validateMove(..), the main method which takes user input.

Question 1: Yes, usually main() just makes an instance of an object. Again: If you would have a GUI,...

Question 2: Yeah, one method should do one thing, the thing it's named for. So, it basically checks, if a game is won, but it has to check three things. The 'three things' part is more of a readability-issue. If you have three methods checkRows() / -Columns() / -Diagonals(), it's much easier to understand. Just think about having a bug. If you are in the method 'checkRows(), you know, that the error occurs during that check. If you have everything in one method, you have to figure out that part first.

Question 3: "Tell, don't ask" (https://martinfowler.com/bliki/TellDontAsk.html). In the main, you ask the turn number, and you ask if the game is won. Make a method 'gameOver()' or something similar.

To the code itself: It's actually very clear and understandable. A bit too much nesting for my liking, some conventions I don't like ({ on a new line), but nothing serious. You did group the problems into subroutines, which is a good start. I think if you put a GUI on top of the game, you will learn a lot more about OO, how to decouple stuff from other stuff, which type should be responsible for stuff, and so on.

My tips: Google SOLID, five core object oriented principles. Start unit testing your code, google JUnit.

Hope this helps,

slowy

\$\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.