0
\$\begingroup\$

I made a TicTacToe implementation using State pattern in Java. I was wondering if this could be improved.

I have states - Setup, UserInput, ProgramInput, Win, Draw and End. A SetupManager has the task of looping through events which would cause a change in State.

The TicTacToe itself is a int[9], where one of the input is from the user via Command line whereas another input is using random.nextInt().


The State interface and its implementations in package tictactoe.state are as follows -

State.java

package tictactoe.state;

public interface State {
    void onEnterState();
    void onExitState();
}

Setup.java

package tictactoe.state;

import tictactoe.statemanager.StateManager;

public class Setup implements State {
    private StateManager manager;

    public Setup(StateManager manager) {
        this.manager = manager;
    }

    @Override
    public void onEnterState() {
        this.manager.reset();
    }

    @Override
    public void onExitState() { }
}

UserInput.java

package tictactoe.state;

import tictactoe.statemanager.StateManager;
import tictactoe.statemanager.TicTacToeStateManager;

import java.util.Scanner;

public class UserInput implements State {
    private final StateManager manager;
    private int row;
    private int column;

    public UserInput(StateManager manager) {
        this.manager = manager;
    }

    @Override
    public void onEnterState() {
        Scanner scanner = ((TicTacToeStateManager) manager).getScanner();

        System.out.printf("Enter a position - row and column: ");
        int row = scanner.nextInt();
        int column = scanner.nextInt();

        while (!manager.isValidMove(row - 1, column - 1)) {
            System.out.printf("Sorry that position is already occupied,"
                    + " please make another choice: ");
            row = scanner.nextInt();
            column = scanner.nextInt();
        }

        this.row = row - 1;
        this.column = column - 1;
        manager.makeAMove(this.row, this.column);
    }

    @Override
    public void onExitState() {
        manager.printGrid();
    }

    public int getRow() {
        return row;
    }

    public int getColumn() {
        return column;
    }
}

ProgramInput.java

package tictactoe.state;

import tictactoe.statemanager.StateManager;

import java.util.Random;

public class ProgramInput implements State {

    private final StateManager manager;
    private final Random random;
    private int row;
    private int column;

    public ProgramInput(StateManager manager) {
        this.manager = manager;
        this.random = new Random();
    }

    @Override
    public void onEnterState() {
        int row = random.nextInt(3);
        int column = random.nextInt(3);

        while (!manager.isValidMove(row, column)) {
            row = random.nextInt(3);
            column = random.nextInt(3);
        }

        this.row = row;
        this.column = column;
        manager.makeAMove(this.row, this.column);
    }

    @Override
    public void onExitState() {
        manager.printGrid();
    }

    public int getRow() {
        return row;
    }

    public int getColumn() {
        return column;
    }

}

Win.java

package tictactoe.state;

import tictactoe.Player;
import tictactoe.statemanager.StateManager;

public class Win implements State {
    private final StateManager manager;
    private final Player winner;

    public Win(StateManager manager, Player winner) {
        this.manager = manager;
        this.winner = winner;
    }

    @Override
    public void onEnterState() {
        manager.setWinner(winner);
        manager.printWin();
    }

    @Override
    public void onExitState() {}
}

Draw.java

package tictactoe.state;

import tictactoe.statemanager.StateManager;

public class Draw implements State {
    private final StateManager manager;

    public Draw(StateManager manager) {
        this.manager = manager;
    }

    @Override
    public void onEnterState() {
        manager.printDraw();
    }

    @Override
    public void onExitState() {}
}

End.java

package tictactoe.state;

import tictactoe.statemanager.StateManager;
import tictactoe.statemanager.TicTacToeStateManager;

public class End implements State {
    private final StateManager manager;

    public End(StateManager manager) {
        this.manager = manager;
    }

    @Override
    public void onEnterState() {
        ((TicTacToeStateManager) manager).getScanner().close();
    }

    @Override
    public void onExitState() { }
}

The package tictactoe.statemanager has the following classes -

StateManager.java

package tictactoe.statemanager;

import tictactoe.Player;
import tictactoe.state.State;

public interface StateManager {
    void nextEvent();
    void changeState(State state);
    boolean isValidMove(int row, int column);
    void makeAMove(int row, int column);
    void printGrid();
    void printWin();
    void printDraw();
    void setWinner(Player winner);
    void reset();
}

TicTacToeManager.java

package tictactoe.statemanager;

import tictactoe.Player;
import tictactoe.TicTacToe;
import tictactoe.state.*;

import java.util.Scanner;

public class TicTacToeStateManager implements StateManager {

    private final TicTacToe ticTacToe;
    private Scanner scanner;
    private State state;
    private Player winner;

    public TicTacToeStateManager(TicTacToe ticTacToe, Scanner scanner) {
        this.ticTacToe = ticTacToe;
        this.scanner = scanner;

        this.state = new Setup(this);
        nextEvent();
    }

    @Override
    public void nextEvent() {
        if (state instanceof Setup) {
            changeState(getStateAfterSetup());
        } else if (state instanceof UserInput) {
            changeState(getStateAfterInput());
        } else if (state instanceof ProgramInput) {
            changeState(getStateAfterInput());
        } else if (state instanceof Draw) {
            changeState(getStateAfterDraw());
        } else if (state instanceof Win) {
            changeState(getStateAfterWin());
        } else if (state instanceof End) {
            return;
        }

        nextEvent();
    }

    private State getStateAfterSetup() {
        System.out.printf("Who goes first? 2 for USER, " +
                "1 for PROGRAM and 0 to EXIT: ");
        int mover = scanner.nextInt();
        ticTacToe.setFirstMover(mover); // This is a sideEffect.
                                        // There should be some 
                                        // other way to do this.

        if (mover == Player.PROGRAM.value) {
            return new ProgramInput(this);
        } else if (mover == Player.USER.value) {
            return new UserInput(this);
        } else {
            return new End(this);
        }
    }

    private State getStateAfterInput() {
        if (state instanceof UserInput) {
            int row = ((UserInput) state).getRow();
            int column = ((UserInput) state).getColumn();

            if (hasWon(row, column, Player.USER)) {
                return new Win(this, Player.USER);
            } else if (areAllMovesExhausted()) {
                return new Draw(this);
            }

            return new ProgramInput(this);
        } else if (state instanceof ProgramInput) {
            int row = ((ProgramInput) state).getRow();
            int column = ((ProgramInput) state).getColumn();

            if (hasWon(row, column, Player.PROGRAM)) {
                return new Win(this, Player.PROGRAM);
            } else if (areAllMovesExhausted()) {
                return new Draw(this);
            }

            return new UserInput(this);
        }
        return null;
    }

    private boolean hasWon(int row, int column, Player player) {
        return ticTacToe.hasWon(row, column, player.value);
    }

    private boolean areAllMovesExhausted() {
        return ticTacToe.areAllMovesExhausted();
    }

    private State getStateAfterWin() { return new Setup(this); }

    private State getStateAfterDraw() { return new Setup(this); }

    @Override
    public void changeState(State newState) {
        state = newState;
        state.onEnterState();
        state.onExitState();
    }

    @Override
    public boolean isValidMove(int row, int column) { 
        return ticTacToe.isMoveValid(row, column); 
    }

    @Override
    public void makeAMove(int row, int column) {
        if (state instanceof UserInput) {
            ticTacToe.makeAMove(row, column, Player.USER);
        } else if (state instanceof ProgramInput) {
            ticTacToe.makeAMove(row, column, Player.PROGRAM);
        }
    }

    @Override
    public void printGrid() { ticTacToe.printGrid(); }

    @Override
    public void printWin() { ticTacToe.printWin(winner); }

    @Override
    public void printDraw() { ticTacToe.printDraw(); }

    @Override
    public void setWinner(Player winner) {
        this.winner = winner;
        ticTacToe.setWinner(winner);
    }

    @Override
    public void reset() { ticTacToe.reset(); }

    public Scanner getScanner() { return scanner; }
}

And finally the main class TicTacToe -

Player.java

package tictactoe;

public enum Player {
    PROGRAM(1),
    USER(2);

    public int value;

    Player(int value) { this.value = value; }
}

TicTacToe.java

package tictactoe;

import tictactoe.statemanager.TicTacToeStateManager;

import java.util.Scanner;

public class TicTacToe {
    // 0 is available, 1 is for the program and 2 is for the user.
    private static final int AVAILABLE = 0;
    private static final int PROGRAM = 1;
    private static final int USER = 2;
    private final int[] grid;
    private int userScore;
    private int programScore;
    private int firstMover;
    private int moves;

    TicTacToe(Scanner scanner) {
        grid = new int[9];
        new TicTacToeStateManager(this, scanner);
    }

    public static void main(String[] args) {
        TicTacToe ticTacToe = new TicTacToe(new Scanner(System.in));
    }

    public void setFirstMover(int firstMover) {
        this.firstMover = firstMover;
    }

    public boolean isMoveValid(int row, int column) {
        int position = row * 3 + column;
        return grid[position] == AVAILABLE;
    }

    public void makeAMove(int row, int column, Player currentPlayer) {
        moves++;
        grid[row * 3 + column] = currentPlayer.value;
    }

    public void printGrid() {
        StringBuilder builder = new StringBuilder();

        for (int i = 0; i < 3; i++) {
            for (int j = 0; j < 3; j++) {
                int moveBy = grid[i * 3 + j];

                if (moveBy == AVAILABLE) {
                    builder.append("   ");
                } else if (moveBy == firstMover) {
                    builder.append(" X ");
                } else {
                    builder.append(" O ");
                }

                if (j == 2) {
                    builder.append("");
                } else {
                    builder.append("|");
                }
            }
            builder.append("\n");
            if (i != 2) {
                builder.append("-----------")
                        .append("\n");
            }
        }

        System.out.println(builder.toString());
    }

    public boolean hasWon(int row, int column, int player) {
        // Same row
        int i;
        for (i = 0; i < 3; i++) {
            if (grid[row * 3 + i] != player) {
                break;
            }
        }
        if (i == 3) return true;

        // Same column
        for (i = 0; i < 3; i++) {
            if (grid[i * 3 + column] != player) {
                break;
            }
        }
        if (i == 3) return true;

        // Same diagonal
        if ((grid[0] == player && grid[4] == player && grid[8] == player) ||
                (grid[2] == player && grid[4] == player && grid[6] == player)) {
            return true;
        }
        return false;
    }

    public boolean areAllMovesExhausted() {
        return moves == 9;
    }

    public void setWinner(Player player) {
        if (player == Player.USER) {
            userScore++;
        } else if (player == Player.PROGRAM) {
            programScore++;
        }
    }

    public void reset() {
        moves = 0;
        clearGrid();
    }

    private void clearGrid() {
        for (int i = 0; i < 9; i++) {
            grid[i] = 0;
        }
    }

    public void printWin(Player currentPlayer) {
        if (currentPlayer == Player.USER) {
            System.out.println("Yay you won!");
        } else {
            System.out.println("Better luck next time");
        }
        printScore();
    }

    public void printDraw() {
        System.out.println("Nobody Won!");
        printScore();
    }

    private void printScore() {
        System.out.printf("The score is - USER: %d, PROGRAM: %d%n",
                userScore, programScore);
    }
}

I have some questions regarding the state and events (nextEvent() being called recursively).

  • Is the implementation of nextEvent() correct?
  • Should there be some changes in changeState() and its related methods?
  • I tend to hop from TicTacToeManager to TicTacToe for the actual data and printing. Should there be any other classes which should have the work of verifying the hasWon(), isMoveValid() etc. and the printing job.

Any other suggestions would also be useful.

\$\endgroup\$
2
\$\begingroup\$
  1. In TicTacToe you have a method isMoveValid but in StateManager you called it isValidMove. Why?
  2. TicTacToeStateManager's constructor not only initializes itself but also starts the game (i.e. calls nextEvent). You should avoid doing that. I'd suggest you to make the first call to nextEvent start the game or even make a method solely for that purpose..
  3. You have created a enum to refer to Players but instead of using it you use an int. You should only represent the player as an int during console input. Everything after that should use your Player enum.
  4. You use System.out.printf even when you don't need to. It's not too important but if you don't need the formatting capability I'd stick to using System.out.println.
  5. I don't exactly like that your StateManager runs on it's own (in nextEvent it calls itself). I'd propose you to instead add a getCurrentState method to StateManager and make main call nextEvent and possibly have some logic (that's the reason for getCurrentState method).
  6. In getStateAfterInput you have two basically the same if statements. You should create a interface like Input (probably with a better name) with methods getRow, getColumn and maybe getPlayer (to indicate which player's input the class processes) from which ProgramInput and UserInput would inherit. That would shorten your method by half and would make it more sensible. You'd probably also could shorten makeAMove by half. And shorten nextEvent a bit too
  7. You could create a interface responsible for user interface (in your case it's the console). That way StateManager could have a method to expose it and you won't need to cast the StateManager to your implementation in UserInput and End (casting to specific implementations is bad even if you have only one of them as you are limiting yourself to only one implementation without even disclosing it in the constructor) then.
\$\endgroup\$
  • \$\begingroup\$ I have implemented the changes you suggested in a gist. Please let me know if you have any more suggestions. \$\endgroup\$ – yadav_vi Jul 27 '17 at 9:43
  • \$\begingroup\$ @yadav_vi I wrote a comment in the gist \$\endgroup\$ – Mibac Jul 29 '17 at 12:35
1
\$\begingroup\$

Thanks for sharing your code.

public class TicTacToeStateManager implements StateManager {
    // ...
    @Override
    public void nextEvent() {
        if (state instanceof Setup) {
            changeState(getStateAfterSetup());
        } else if (state instanceof UserInput) {
            changeState(getStateAfterInput());
        } else if (state instanceof ProgramInput) {
            changeState(getStateAfterInput());
        } else if (state instanceof Draw) {
            changeState(getStateAfterDraw());
        } else if (state instanceof Win) {
            changeState(getStateAfterWin());
        } else if (state instanceof End) {
            return;
        }
        nextEvent();
    }
    // ...
}

One of the promary goals of OOP is to avoid decisions based on the object type of the users side.

If you had approached that correctly this method would look like this:

    @Override
    public void nextEvent() {
        state=  state.changeState();
        nextEvent();
    }

This reveals another problem: This method is recursive and changes the objects state as a side effect.

A caller would not expect that by the name of the method.

And according the the single layer of abstraction principle the method should either change the objects state or call other methods, not both.


   Scanner scanner = ((TicTacToeStateManager) manager).getScanner();

This is a similar problem. Again you depend on knowledge of the actual type that should be hidden behind the interface.

You do that because you work around another problem in your code: hidden dependencies.

Your class UserInput needs a Scanner object to work properly. You should pass this Scanner object in ass constructor parameter (as you do at TicTacToe(Scanner scanner) or TicTacToeStateManager(TicTacToe ticTacToe, Scanner scanner).

\$\endgroup\$
  • \$\begingroup\$ I have made my state pattern based on an implementation that I saw here. I have made the changes that were suggested in the accepted answer, here is a gist of it. About the nextEvent() implementation that you have mentioned, I am having some difficulty understanding how my states would be. I would appreciate if you could expand on that. \$\endgroup\$ – yadav_vi Jul 27 '17 at 9:38

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.