11
\$\begingroup\$

I want to learn OO design and as a start, I implemented Tic Tac Toe. I'd like to hear your thoughts on the design and implementation.

Classes:

  • Player - Has just a name as of now. I hope to develop this more to use for other similar situations.
  • Board - Has a 3x3 matrix and takes care of checking if game is over and if any legal move is left.
  • Controller - Contains two players and a game board.

package ood.tictac2;


class Player {
  private String pName;

  public Player(String name){
      this.pName=name;
  }
  public String getPlayerName(){
      return this.pName;
  }
}


package ood.tictac2;

class Board{
    private char board[][] = new char[3][3];
    private int gridSpaceLeft=9;

    public void initBoard(){
        for(int row=0;row<board.length;row++)
            for(int col=0;col<board[0].length;col++)
                board[row][col]='-';
    }
    public boolean isGridSpaceLeft() {
        return (gridSpaceLeft>0);
    }

    public char getPos(int row, int col){
        return board[row][col];
    }
    public void setPos(int row, int col, char x){
        if(isAllowed(row, col)){
            board[row][col]=x;
            gridSpaceLeft--;
        }
    }

    public boolean isAllowed(int row, int col){
        return (board[row][col]=='-');
    }

    public boolean isGameOver(){
        return checkRows() || checkCols() || checkDiag();
    }

    private boolean checkDiag() {
        StringBuilder d1 = new StringBuilder();
        StringBuilder d2 = new StringBuilder();
        boolean isOver = false;
        for(int row=0,col=2;row<3 && col>=0;row++,col--){
            d1.append(board[row][row]);
            d2.append(board[row][col]);
            if (d1.toString().equals("xxx") || d1.toString().equals("ooo")) isOver=true;
            if (d2.toString().equals("xxx") || d2.toString().equals("ooo")) isOver=true;
        }
        return isOver;
    }
    private boolean checkCols() {
         boolean isOver = false;
         StringBuilder sb;
         for(int row=0;row<3;row++){
            sb = new StringBuilder();
            for(int col=0;col<3;col++){
                sb.append(board[col][row]);
                if (sb.toString().equals("xxx") || sb.toString().equals("ooo")) isOver=true;
            }
        }
        return isOver;
    }
    private boolean checkRows() {
        boolean isOver = false;
        for(int row=0;row<3;row++){
            String s = new String(board[row]);
            if (s.equals("xxx") || s.equals("ooo")) isOver = true;
        }
        return isOver;
    }

    public void printBoard() {
        for(int row=0;row<board.length;row++){
            for(int col=0;col<board[0].length;col++)
                System.out.print(board[row][col]);
            System.out.print("\n");
        }
    }
}

package ood.tictac2;

import java.util.ArrayList;
import java.util.List;
import java.util.Scanner;

class Controller{
    private List<Player> players = new ArrayList<>();
    private Board gameBoard;
    private Scanner in = new Scanner(System.in);
    private final char DASH = 'x';
    private final char DOT = 'o';

    //Create and initialize board and players
    public Controller(){        
        Player player;
        String name;
        gameBoard = new Board();
        gameBoard.initBoard();
        while(players.size()<2){
            System.out.print("Enter player name:");
            name = in.next();
            if(name.isEmpty()) continue;
            else{
                player = new Player(name);
                players.add(player);
            }   
        }
    }

    public void playGame(){
        Player temp=players.get(0);
        boolean gameOver = false;
        while(!gameOver && gameBoard.isGridSpaceLeft()){
            int[] move= promptTurn(temp);
            gameBoard.setPos(move[0],move[1], (temp.equals(players.get(0)))?DASH:DOT);
            gameBoard.printBoard();
            if(gameBoard.isGameOver()){
                System.out.println(temp.getPlayerName() + " won!");
                gameOver = true;
            }
            temp = (temp == players.get(0))?players.get(1):players.get(0);
        }
        System.out.println("Game over!");
    }

    private int[] promptTurn(Player temp){
        int row=-1, col=-1;
        System.out.println(temp.getPlayerName()+ ", please make your choice");
        while(row<0 || col<0){
            System.out.print("Enter row (1-3): ");
            row = in.nextInt();
            System.out.println("Enter col (1-3): ");
            col = in.nextInt();
            if(row <1 || row>3 || col<1 || col>3 || !gameBoard.isAllowed(row-1, col-1)){
                System.out.println("Invalid position "+row + ", "+ col+". "+temp.getPlayerName()+", please try again.");
                row=-1;col=-1;
            }
        }
        return new int[]{row-1,col-1};

    }   

    public static void main(String[] args){
        Controller c = new Controller();
        c.playGame();
    }
}

I would appreciate any comments on how to make this extensible. As you can see, I haven't implemented any UI. I did this as a learning exercise and any pointers to help me improve this design and implementation are welcome.

\$\endgroup\$
10
\$\begingroup\$

Here are some comments to your code:

  • Simplify check functions – When you are checking for a winner you could change the logic slightly. First of all you could/should move the check against the xxx or yyy outside of the loop, as it will never hit when you are building up the line. Second, you could break out early if the current grid position is empty, as it then can not be a part of a complete line
  • Alternate check function simplification – Instead of building new strings all the time, you could have two variables, xHasWon and oHasWon. Set them to true before the loop, and falsify them if you hit the other piece or an empty space. If neither is set at end, you don't have a winner. You could also return early if both is false (or the current space is empty). This would also allow for you to see who has won, or do as you do in current version: return xHasWon || oHasWon
  • Simplify diagonal check – When building the two diagonals, this can be done using a single variable, and then using the positions board[i][i] and board[dimension-i-1][i] where dimension in your case is 3
  • Consider allowing for larger dimensions – Instead of hard coding the dimension to a 3x3 board, allow for the constructor to take a parameter indicating the dimension, and then changing all references where it is hard coded to use a the dynamic dimension
  • Add a Position type – Instead of returning an int[] consider returning a simple type holding the position
  • Avoid magic numbers – When reading the next position you use -1 to keep looping, it is better to do a while(true) or while (pos == null) instead of testing against -1. If using the everlasting loop, be sure to break out when you have a valid position
  • Add spaces around if condition – To me it is a little jammed up when you write if(gameBoard.isGameOver()){. I would suggest to open it up a little if (gameBoard.isGameOver()) { as I find it easier to read both where the condition is and where the if block starts
  • Be consistent with use of braces and indentation – Do avoid if or for loops without braces, this will at some point in time create errors for you, and are already causing code to be harder to read. Please do not do stuff like if (true) continue<lineshift>else{..., this is a code smell
  • Be consistent in spacing between functions – For some you have a line shift between functions, and some not. Choose either (hopefully with space), and stick to it
  • Choose good variable names – Most of the time you've chosen good variables name, but then something like temp or c occurs. A better alternative for the first one here would be currentPlayer
  • Reconsider some of the function names – This a minor, but to me some of the functions are not intuitive. Like the checkXxxx, what do you check for? A row? or a winning row? or ??? And promptTurn would be better with something like readNextMove. Likewise with isAllowed, what is allowed? A better name (and close to standard naming) would be isEmpty

Hope this doesn't feel to hush, but this was my thoughts when reading through your code. It does seem like it does the job, but I would reconsider some of the checks for when winning. But mostly my comments are related to code styling, which seems like nitpicking, but in the long run you are better of being consisten and writing clean and nice code. It will make your life much easier when revisiting your code at a later point in time, and you need to find a bug or flaw or whatever.

1st addition: Some additional comments

  • Check only cross section for win – Given that no one has won already, you can simplify and only check the row, column and possibly diagonal where the last marker was placed, and you only need to check for that marker type, the x or y that was used. In other words if the last piece placed was an x in pos (0,1) you only need to check for x in row[0], and column[1] and no diagonal since it is in the middle position of first row
  • A better naming standard – My main language is C#, not java, but there we follow this naming standard, which helps identifying the scope and availability of the variables (and also eliminates the need for using this as a qualifier):
    • _name – leading underscore is a private class variable
    • Name – Uppercase first letter for public class variables and properties
    • name – Lowercase first letter for method variables and/or parameters
    • No prefix or postfix to indicate types, like p or lpsz or whatever. Let the compiler worry about types...

2nd addition: New method checkIfWinningMove

Here is some code doing the latter variant of checking whether the last move was a win. I have not implemented any of the other stuff I mentioned, just added this one method (and if I'd change the entire class, then the Dimension would have been made a class variable).

    public boolean checkIfWinningMove(int row, int col, char piece) {
        final int Dimension = 3;

        // Assume all possible winning conditions are true ...
        boolean winningRow = true;
        boolean winningCol = true;

        // ... at least if the position is on the diagonal 
        boolean winningDiagonal = row == col;
        boolean winningReverseDiagonal = (Dimension - row - 1) == col;

        // Falsify the assumption of all are winning, and if falsified
        // don't check again for that winning condition
        for (int i=0; i < Dimension; i++) {
            if (winningRow && board[row][i] != piece) {
                winningRow = false;
            }

            if (winningCol && board[i][col] != piece) {
                winningCol = false;
            }

            if (winningDiagonal && board[i][i] != piece) {
                winningDiagonal = false;
            }

            if (winningReverseDiagonal && board[Dimension-i-1][i] != piece) {
                winningReverseDiagonal = false;
            }

            // If no winning conditions are left, break out early
            if (!winningRow && !winningCol && 
                !winningDiagonal && !winningReverseDiagonal ) {
               break;
            }
        } 

        return winningRow || winningCol || 
               winningDiagonal || winningReverseDiagonal;
    }

This would replace your isGameOver, checkDiag, checkCols and checkRows method. I also changed the playGame to a simpler logic for checking whose turn it is, what to play, please enjoy:

    public void playGame(){
        boolean flipTurns = true;
        Player currentPlayer = players.get(flipTurns ? 0 : 1);
        while(gameBoard.isGridSpaceLeft()){
            int[] move= promptTurn(currentPlayer);
            char playedPiece = flipTurns ? DASH : DOT;
            gameBoard.setPos(move[0],move[1], playedPiece);

            if (gameBoard.checkIfWinningMove(move[0], move[1], playedPiece)) {
                System.out.println(currentPlayer.getPlayerName() + " is a winner!");
                break;
            }

            gameBoard.printBoard();

            flipTurns = !flipTurns;
            currentPlayer = players.get(flipTurns ? 0 : 1);
        }
        System.out.println("Game over!");
    }
\$\endgroup\$
  • \$\begingroup\$ Definitely the row check within the loop was a blunder. I have fixed that now. I will make the board size configurable. One of the questions I had was whether to stick the DASH, DOT in the Controller. I feel it might be better to move it into the player and have each player select the character he wants. If I do that, then the winning condition check needs to change accordingly. I agree with your comments on code styling. I have implemented your suggestions. i will push it to bitbucket so that I can track how the code/design evolves. :) \$\endgroup\$ – RabbitCodes Aug 23 '15 at 10:39
  • \$\begingroup\$ wow! really elegant refactoring. To check only the relevant row, col as well as the diagonal is cool. I only wish I had thought about it first :) I think I'll move the DASH/DOT out of the controller and most likely into the player class and also position as an enum. Perhaps I can now think of an UI for this. \$\endgroup\$ – RabbitCodes Aug 23 '15 at 11:58
  • \$\begingroup\$ @RabbitCodes, just a little note on english terms. This is a dash: -, and this is a dot: ·. You would want to use cross and either circle or nought. This game is also called noughts and crosses. They do, as you say, not really belong in the controller, and for customisation they could be a player attribute. Note how that doesn't change the checkIfWinningMove as you are just passing in the marker. Don't understand why you want to have the position as an enum, this is better of as a struct/simple class. \$\endgroup\$ – holroy Aug 23 '15 at 12:26
3
\$\begingroup\$

This keyword

Use this to disambiguate, in your Player method if your field was called name, it would make sense to use this, (It's always a pain talking about this, I see your problem this is wrong -- let me know if it's confusing). What you'd say in that case is set this current instance of player's name to the name that was passed in. It's not a big issue, but since you have the field as pName you don't need to use the this keyword. e.g. it can just be:

class Player {
  private String pName;

  public Player(String name){
      pName = name;
  }
  public String getName(){
      return Name;
  }

}

Also the method getName() is sufficient, it's being called on a player already, player.playerMethod() sounds redundant. Also, Is name meant to be final in this case? I'd declare your field final if so, or add a setter so you may change it on the fly.

Evaluating win condition

I'd refactor your win condition methods, you're using a two dimensional array, but a single one would be sufficient. With a single one this is what you'd have:

[0][1][2]
[3][4][5]
[6][7][8]

You'd check for:

  1. The rows: [0][1][2], [3][4][5], [6][7][8].
  2. Columns: [0][3][6], [1][4][7], [2][5][8].
  3. and Diagonals: [0][4][8], [2][4][6].

This would be simpler to write, read, and assess than writing and evaluating StringBuilders as you currently are.

Notice that each of these is evaluating the equivalency of these position's content, of course excluding that they are empty, in your case != '-' . You only evaluate three positions at a time, so you can write a method that takes positions as arguments, and do rows, and columns in a loop, throwing in diagonals as well.

So the method you'd have would look something like:

private boolean isWinSet(int pos1, int pos2, int pos3) {
    return board[pos1] != '-' 
        && board[pos1] == board[pos2]
        && board[pos2] == board[pos3];
}

and it could be called from an evaluating method:

private void evaluateBoard() {
    for (int horizontal = 0, vertical = 0; horizontal < board.length; horizontal += 3) {
            if (isWinSet(vertical, vertical + 3, vertical++ + 6)) {
                // win found - do something and/or return
            }
            if (isWinSet(horizontal, horizontal + 1, horizontal + 2)) {
                // win found - do something and/or return
            }
        }
        // diagonal
        if (isWinSet(0, 4, 8) || isWinSet(2, 4, 6)) {
            // win found - do something and/or return
        }
    }

If you want to keep the two dimensional arrays:

[0][0], [0][1], [0][2]
[1][0], [1][1], [1][2]
[2][0], [2][1], [2][2]

The methods you'd write could be even simpler e.g. evalwinRow(0), could check the first row, and you could check all 3 in a loop, same as before, likewise, evalWinCol(2) could evaluate the last column. evalDiag() could just evaluate both diagonal win conditions at once. You'd effectively have several methods:

  1. To determine that a position is filled and equivalent to something else
  2. Take three positions and compare see if they're a win.
  3. Three different methods that evaluate rows, columns, and diagonals all calling the position method.
  4. an Evaluate board method that does all of the above in a simple loop.

Or if you abstract further and make Positions objects you can set a location type e.g.

enum Orientation {
    COLUMN,
    ROW,
    DIAGONAL
}

As it is, notice however, that some would fit two orientations. If you like this idea, you could abstract even further and list all 9 positions -- the purpose would be to only evaluate the only possible ways the position that was last set could win. Though this is simple, and you needn't go that far, it would make things more efficient and make for an interesting exercise.

\$\endgroup\$
  • \$\begingroup\$ Let us continue this discussion in chat. \$\endgroup\$ – Legato Aug 22 '15 at 23:39
  • \$\begingroup\$ yes, pName should have been final. Thanks for catching that. I didn't use hardcoded positions for checking winning condition because I wanted to make board size configurable. However, for this application, the idea of using enum for position seems really cool. I will explore that. I have not used enum at all. This might be a good time to learn that. \$\endgroup\$ – RabbitCodes Aug 23 '15 at 10:42

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.