5
\$\begingroup\$

A while back I had opened this topic: Tic Tac Toe - Stage 1: console PvP

I had to put that mini-project on hold due to exams, but I've started working on it again a couple of days ago. Now it's possible to play vs. an AI, or to let two AIs play against each other. The choices can be made in a menu.
I've studied in the meantime, of course, and I believe that my understanding of OO design has improved [shoutout (xD) to Head First Design Patterns. This has helped me a lot, even though I've learned just a couple of patterns. I'm looking forward to learn more of those!]. That being said, I'm well aware that I'm still lacking a lot.

Before I ask for a code review: I had to open a new repository on github, because I wanted to continue working on Eclipse but I couldn't manage to import the old repository.
I currently have 19 classes, so I imagine that this post is going to be rather long. If you want to view the classes on github, here is the link: https://github.com/kebiro/TicTacToe/tree/GameMenu/TicTacToe/src/game

Overall, I believe that I managed to solve some parts of my design in a good way, others in a not-so-good way. I'm especially unsure whether my Input/Output classes and my GameMenu are properly designed. Board still needs some proper refactoring as well. I've tried to keep future ideas in mind whereever I could, but I'm not sure if I've succeeded in keeping my design properly extendable.

Anyway, here is the code (some classes are excluded, like Main.java):

Board, Move, Position

package game;

public class Move {
    private String move;

    public Move(String move) {
        if(move.length() != 1 || !"123456789".contains(move))
            throw new IllegalMoveException("move needs to be a digit (1-9)");

        this.move = move;
    }

    public int getMove() {
        return Integer.parseInt(move);
    }

    @Override
    public String toString() {
        return move;
    }
}

package game;

public class Position {
    private int row, col;

    public Position(Move move) {
        this.row = (move.getMove() - 1) / 3;
        this.col = (move.getMove() - 1) % 3;
    }

    public int getRow() {
        return row;
    }

    public int getCol() {
        return col;
    }
}

package game;

public class Board {
    private String[][] board;

    public Board() {
        init();
    }

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

    private void init() {
        board = new String[3][3];
        int count = 1;

        for(int i=0; i < board.length; ++i) {
            for(int j=0; j < board[i].length; ++j) {
                // set the field positions, from 1 to 9
                board[i][j] = "" + count++;
            }
        }
    }

    public boolean isFull() {
        boolean isFull = true;

        for(int i=0; i < board.length; ++i) 
            for(int j=0; j < board.length; ++j)
                // still contains an unmarked field (indicated by a number)
                if("123456789".contains(board[i][j]))
                    isFull = false;

        return isFull;
    }

    // ???
    public void markField(Move move, Sign sign) {
        if(!fieldIsMarked(move)) 
            setValueAtPosition(move, sign + "");
    }

    // ???
    public boolean fieldIsMarked(Move move) {
        return !getValueAtPosition(move).equals(move.toString());
    }

    // ???
    private String getValueAtPosition(Move move) {
        Position pos = new Position(move);
        return board[pos.getRow()][pos.getCol()];
    }

    // ???
    private void setValueAtPosition(Move move, String val) {
        Position pos = new Position(move);
        board[pos.getRow()][pos.getCol()] = val;
    }

    public boolean hasThreeInARow() {
        boolean horizontal = 
                board[0][0].equals(board[0][1]) && board[0][1].equals(board[0][2])
                || ( board[1][0].equals(board[1][1]) && board[1][1].equals(board[1][2]) )
                || ( board[2][0].equals(board[2][1]) && board[2][1].equals(board[2][2]) );

        boolean vertical =
                board[0][0].equals(board[1][0]) && board[1][0].equals(board[2][0])
                || board[0][1].equals(board[1][1]) && board[1][1].equals(board[2][1])
                || board[0][2].equals(board[1][2]) && board[1][2].equals(board[2][2]);

        boolean diagonal = 
                board[0][0].equals(board[1][1]) && board[1][1].equals(board[2][2])
                || board[0][2].equals(board[1][1]) && board[1][1].equals(board[2][0]);

        return horizontal || vertical || diagonal;
    }

    @Override
    public String toString() {
        StringBuilder sb = new StringBuilder();
        int formatSize = 0;

        for(int i=0; i < board.length; ++i) {
            for(int j=0; j < board[i].length; ++j) {
                sb.append(board[i][j] + " | ");
            }

            // #_|_#_|_#_|_ - delete the last two blanks + |
            sb.delete(sb.length()-3, sb.length());
            // determines how many "-" to draw horizontally to make it look more aligned 
            formatSize = (formatSize == 0) ? sb.length() : formatSize;

            if(i < board.length-1) {
                sb.append(System.lineSeparator());
                // http://stackoverflow.com/questions/1235179/simple-way-to-repeat-a-string-in-java
                sb.append(new String(new char[formatSize]).replace("\0", "-"));
                sb.append(System.lineSeparator());
            }
        } 

        return sb.toString();
    }
}

I'm not sure if I've used Position properly. I marked the places with // ???. I also noticed, when writing this, that maybe markField(Move, Sign) could be extracted from Board.
hasThreeInARow() has been moved from Game to Board. Other than that, Board is pretty much the same as before.


Game

package game;

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

public class Game {
    private Board board;
    private Player player1, player2;
    private Player activePlayer;
    private boolean hasThreeInARow;
    private ManagerFactory managerFactory;
    private Output outputManager;

    public Game(ManagerFactory managerFactory) {
        this.managerFactory = managerFactory;
        this.outputManager = managerFactory.createOutputManager();

        GameMenu menuManager = managerFactory.createMenuManager();
        menuManager.menu();
        board = menuManager.createBoard();
        player1 = menuManager.createPlayer1();
        player2 = menuManager.createPlayer2();
    }

    /**
     * play a Tic-Tac-Toe game 
     */
    public void playGame() {
        activePlayer = playerToStart();

        while(!board.isFull()) {
            outputManager.callForTurn(activePlayer);
            outputManager.showBoard(board);
            Move move = activePlayer.makeMove();
            board.markField(move, activePlayer.getSign());
            hasThreeInARow = board.hasThreeInARow();
            if(hasThreeInARow)
                break;
            changeTurns();
        }

        System.out.println();
        outputManager.showBoard(board);

        if(hasThreeInARow)
            outputManager.declareWinner(activePlayer);
        else
            outputManager.println("draw");

        outputManager.println("===============================================");
        new Game(managerFactory).playGame();
    }

    // what follows are auxiliary methods for playGame(); to make playGame() more readable

    private Player playerToStart() {
        return (new Random().nextInt(2) == 0) ? player1 : player2;
    }

    private void changeTurns() {
        activePlayer = (activePlayer == player1) ? player2 : player1;
    }
}

I have extracted a lot of methods ouf of Game. I believe it has become quite cohesive now. Things I'm unsure about is whether the way I construct Game is done right. (But the problem source is probably GameMenu, if there is a problem ...)
Also, I'm not sure if the playGame() loop will work for a GUI the way it is now. (The Sysout after the loop is bothersome, and I call showBoard(Board) in every iteration, which is necessary for console)


The Participants and Strategy

package game;

public class Player {
    private String name;
    private Sign sign;
    private MoveStrategy moveStrategy;


    public Player(String name, Sign sign, MoveStrategy moveStrategy) {
        this.name = name;
        this.sign = sign;
        this.moveStrategy = moveStrategy;
    }

    public Move makeMove() {
        return moveStrategy.makeMove();
    }

    public Sign getSign() {
        return sign;
    }

    @Override
    public String toString() {
        return name;
    }
}

package game;

public interface MoveStrategy {
    public Move makeMove();
}

package game;

import game.io.*;

public class HumanMoveStrategy implements MoveStrategy {
    private Board board;
    private Output outputManager;
    private Input inputManager;

    public HumanMoveStrategy(Board board, ManagerFactory managerFactory) {
        this.board = board;
        this.outputManager = managerFactory.createOutputManager();
        this.inputManager = managerFactory.createInputManager();
    }


    @Override
    public Move makeMove() {
        outputManager.print("Your move: ");

        Move move = null;
        do {
            try {
                move = new Move(inputManager.read());
                while(board.fieldIsMarked(move)) {
                    outputManager.print("That move has alreary been made, try again: ");
                    move = new Move(inputManager.read());
                }
            } catch(IllegalMoveException ime) {
                outputManager.print(ime.getMessage() + ", try again: ");
                move = null;
            }
        } while(move == null);

        return move;
    }
}

package game;

import game.io.*;
import java.util.Random;

public class AIrandomMoveStrategy implements MoveStrategy {
    private Board board;
    private Output outputManager;

    public AIrandomMoveStrategy(Board board, ManagerFactory managerFactory) {
        this.board = board;
        this.outputManager = managerFactory.createOutputManager();
    }

    @Override
    public Move makeMove() {
        Random rand = new Random();
        int movePos = Math.floorMod(rand.nextInt(), 9) + 1;

        Move move = new Move(movePos + "");
        while(board.fieldIsMarked(move)) {
            movePos = Math.floorMod(rand.nextInt(), 9) + 1;
            move = new Move(movePos + "");
        }

        outputManager.println("Move made: " + movePos);

        return move;
    }
}

I have moved the makeMove() method out of Game and put it into Player. I was thinking of how to implement an AI (which was planned for the next stage). I figured that the only difference between a human player and an AI is the way a move is made.
Here I applied the Strategy Pattern, which I believe was the right decision. So Player delegates to MoveStrategy. Originally I had planned to wait before I start with stage 2 until stage 1 is fully finished. The Strategy made the implementation of an AI fairly simple though, so I did it. (I'll postpone the Minimax-AI for a while though.)


Input/Output

package game.io;

public interface Input {
    public String read();
    public String readLine();
}

package game.io;

import java.util.*;

public class InputConsole implements Input {

    private Scanner scanner = new Scanner(System.in);

    @Override
    public String read() {
        return scanner.next();
    }

    @SuppressWarnings("resource")
    @Override
    public String readLine() {
        Scanner scanner = new Scanner(System.in);
        return scanner.nextLine();
    }

}

package game.io;

import game.Board;
import game.Player;

public interface Output {
    public void showBoard(Board board);
    public void callForTurn(Player activePlayer);
    public void declareWinner(Player winner);
    public void print(String s);
    public void println(String s);
}

package game.io;

import game.Board;
import game.Player;

public class OutputConsole implements Output {

    @Override
    public void showBoard(Board board) {
        System.out.println("=========");
        System.out.println(board);
        System.out.println("=========");
    }

    @Override
    public void callForTurn(Player activePlayer) {
        System.out.printf("%n%s's turn (%s)%n", activePlayer, activePlayer.getSign());
    }

    @Override
    public void declareWinner(Player winner) {
        System.out.printf("%s (%s) won%n", winner, winner.getSign());
    }

    @Override
    public void print(String s) {
        System.out.print(s);
    }

    @Override
    public void println(String s) {
        System.out.println(s);
    }

}

As mentioned above, I had trouble with these classes. This was proposed in my previous question, not sure how well I've succeeded in implementing the idea. I noticed, however, that this is the Strategy Pattern. At least it's very similar to it. To make the Input/Output work with a GUI, I just have to provide concrete implementations that work with a GUI.


The Game Menu

package game;

import game.io.*;

public abstract class GameMenu {
    protected Player player1, player2;
    protected ManagerFactory managerFactory;
    protected Output outputManager;
    protected Input inputManager;
    protected Board board = new Board();

    public abstract void menu();

    public abstract Player createPlayer1();

    public abstract Player createPlayer2();

    public Board createBoard() {
        return board;
    }
}

package game;

public class ConsoleGameMenu extends GameMenu {

    public ConsoleGameMenu() {
        this.managerFactory = new ConsoleManagerFactory();
        this.outputManager = managerFactory.createOutputManager();
        this.inputManager = managerFactory.createInputManager();
    }

    @Override
    public void menu() {
        outputManager.println("Welcome to Tic Tac Toe");
        outputManager.println("Please choose the gmae you would like to play: ");
        outputManager.println("\t(1) Singleplayer");
        outputManager.println("\t(2) Multiplayer");
        outputManager.println("\t(3) watch mode (Computer vs. Computer)");
        outputManager.println("-----------------------------------------------");
        outputManager.println("For help, press 'h'");
        outputManager.println("To exit, press 'x'");
        outputManager.println("===============================================");

        String input = inputManager.read();
        while(!input.matches("[1, 2, 3, h, x]")) {
            outputManager.println("Invalid input, please try again");
            input = inputManager.read();
        }


        switch(input) {
        case "1": initSingleplayer(); break;
        case "2": initMultiplayer(); break;
        case "3": initWatchMode(); break;
        case "h": showHelp(); break;
        case "x": exit(); break;
        default: // TODO

        outputManager.println("===============================================");
        outputManager.println("==================Tic Tac Toe==================");
        outputManager.println("===============================================");
        }
    }


    private void initSingleplayer() {
        outputManager.println("At what difficulty would you like to play?");
        outputManager.println("\t(1) easy");
        outputManager.println("\t(2) hard - not implemented yet");
        outputManager.println("===============================================");

        String input = inputManager.read();
        while(!input.matches("[1, 2]")) {
            outputManager.println("Invalid input, please try again");
            input = inputManager.read();
        }

        outputManager.print("Player 1 enter your name: ");
        player1 = new Player(inputManager.readLine(), Sign.X, new HumanMoveStrategy(board, managerFactory));

        switch(input) {
        case "1": player2 = new Player("Random AI", Sign.O, new AIrandomMoveStrategy(board, managerFactory)); break;
        case "2": break;
        default: // TODO
        }
    }


    private void initMultiplayer() {
        outputManager.print("Player 1 enter your name: ");
        player1 = new Player(inputManager.readLine(), Sign.X, new HumanMoveStrategy(board, managerFactory));
        outputManager.print("Player 2 enter your name: ");
        player2 = new Player(inputManager.readLine(), Sign.O, new HumanMoveStrategy(board, managerFactory));
    }

    private void initWatchMode() {
        player1 = new Player("AI#1", Sign.X, new AIrandomMoveStrategy(board, managerFactory));
        player2 = new Player("AI#2", Sign.O, new AIrandomMoveStrategy(board, managerFactory));
    }

    private void showHelp() {
        outputManager.println("This is the help menu");
        outputManager.println("I hope this was helpful");
        outputManager.print("To return back to the main menu, press 'r': ");

        String input = inputManager.read();
        while(!input.matches("[r, R]")) {
            input = inputManager.read();
        }

        outputManager.println("===============================================");
        menu();
    }

    private void exit() {
        outputManager.print("Are you sure that you want to end the game (y/n)? ");

        String input = inputManager.read();
        while(!input.matches("[y, Y, n, N]")) {
            outputManager.println("Invalid input. Please answer with yes (y/Y) or no (n/N)");
            input = inputManager.read();
        }

        if(input.equalsIgnoreCase("y")) {
            outputManager.println("Good bye!");
            System.exit(0);
        }
        else {
            outputManager.println("===============================================");
            menu();
        }
    }

    // TODO: PlayerNotInitializedException
    @Override
    public Player createPlayer1() {
        return player1;
    }

    @Override
    public Player createPlayer2() {
        return player2;
    }

}

This also gave me some trouble. GameMenu ended up like a talking Abstract Factory, so that's kind of two responsibilities it has (1: it plays the role of the game menu, and 2: it returns two instances of Player and a Board). But maybe this is justified, since the instantiation of these classes depend on the menu calls. ConsoleGameMenu is not fully complete, but that's the basic structure.


Managers and Abstract Factory

package game;

import game.io.*;

public interface ManagerFactory {
    public Output createOutputManager();
    public Input createInputManager();
    public GameMenu createMenuManager();
}

package game;

import game.io.*;

public class ConsoleManagerFactory implements ManagerFactory {

    @Override
    public Output createOutputManager() {
        return new OutputConsole();
    }

    @Override
    public Input createInputManager() {
        return new InputConsole();
    }

    @Override
    public GameMenu createMenuManager() {
        return new ConsoleGameMenu();
    }
}

Finally, I've used the Abstract Factory Pattern to enforce that a Game is always constructed with corresponding classes, that is, to enforce that something like new Game(new InputConsole(), new OutputGUI(), new GameMenuFile()) can't happen. (I'm sure that having input on console and output on a GUI is not necessarily wrong depending on the context, but I don't want it.)


Before I finish this, I have two more questions:

  • Are there places where comments would be appropriate? What methods should have Javadoc attached to them?
  • I haven't wrote any tests thus far. What parts should definitely be tested? Where are tests unnecessary?

I appreciate in advance; and I'm really sorry for this long post. =/

\$\endgroup\$
  • \$\begingroup\$ Can you use Java 8? \$\endgroup\$ – Caridorc Mar 4 '16 at 20:22
  • \$\begingroup\$ @Caridorc Eh, still have to learn that xd. I've already been told that it's much better than Java 7. But my main concern right now is coding the OO way. Do you have a site to recommend for learning Java 8? \$\endgroup\$ – Beko Mar 4 '16 at 20:41
  • \$\begingroup\$ Pretty much any site you find in the first page of goggle is a fine source, the main improvement in Java 8 are functional interfaces (getting a step nearer to functional programming) \$\endgroup\$ – Caridorc Mar 4 '16 at 20:46
  • 1
    \$\begingroup\$ @Caridorc I've tried a couple but was dissatisfied by them. Well, I'll find something ... sooner or later :-). \$\endgroup\$ – Beko Mar 4 '16 at 20:52
2
\$\begingroup\$

This will be a number of nitpicks / quick-to-fix small things.

  • package game; is an ... unconventional name. Packages are usually named after an owned domain in reverse order. So I (who actually do not own that domain) use de.vogel612 as prefix for all packages I write for myself. Then I follow with the application's name and then the package hierarchy begins. This convention is to prevent name-clashes of packages.

  • Move.getMove() repeats work. Making a String into an Integer is more work than making an Integer into a String. You should consider using the following code instead.

    public class Move {
        private final int move;
        public Move(String move) {
            this.move = Integer.parseInt(move);
        }
        // ...
    }
    

    This also includes making the private field final. You expect it to never ever change and you initialize it in your constructor. Making it final makes the compiler verify the assertion that this field never changes.
    Note that this may not be strictly necessary in this context, but it's a good habit to get into.

  • The advice about final fields also applies for Position. and interestingly Board.

  • While we're at Board. I have a personal dislike against init() methods, because they do not count as Initializer-Blocks. Those blocks have some minor special rules, especially when working on final fields :D.
    I strongly suggest you inline the init method, because it's basically... counterproductive.

  • "I'm not sure if I've used Position properly. I marked the places with // ???`" ... Yeap. You should actually pass in Positions there. Those methods to too much for what their name is.

  • Player also would benefit of final instance-fields.

I've just quickly skimmed over the rest of the code, but it looks quite nice and well-separated to me. There may be some hidden details in there, but I unfortunately do not have time for a more in-depth review

\$\endgroup\$
  • \$\begingroup\$ Since so much time has passed I now noticed that my post is waaay too long. So I can't blame you for not going deeper into it, and I really appreciate that you took your time to do what you did. I'll consider all of this once I have time to work on it again. \$\endgroup\$ – Beko Jul 15 '16 at 19:44

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.