HiveBrain v1.2.0
Get Started
← Back to all entries
patternjavaModerate

Tic Tac T-OO: Design and Implementation

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
codereviewobject-orientedjavastackoverflowtic-tac-toe
designtictacandimplementation

Problem

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;row0);
}

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=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 players = new ArrayList<>();
private Board gameBoard;
private Scanner in = new Scanner(System.in);
private final char DASH = 'x';
private final

Solution

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) continueelse{..., 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;

//

Code Snippets

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;
    }
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!");
    }

Context

StackExchange Code Review Q#101595, answer score: 10

Revisions (0)

No revisions yet.