Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

I am practicing object oriented design and have taken Tic Tac Toe as an example. I have written first all the requirements and then started writing code. I would like to get it reviewed so that I can improve my skill further. Please provide suggestions.

I have placed code at this GitHub repo.

Square.js

//Square class
function Square(){
    this.symbol = "";
    this.isOccupied = false;
}

//Based on player turn set symbols
Square.prototype.setSymbol = function(value){
    if(value !== 'X' && value !== 'O'){
        return 'Please enter X or O';
    }else if(this.isOccupied){
        return 'This square is already filler'
    }else{
        this.symbol = value;
        this.isOccupied = true;
    }
};

Square.prototype.getSymbol = function(){
    return this.symbol;
};

Board.js

//Board Class
function Board(sqaures){
    this.squares = sqaures;
    this.grid = [];
    this.filledSquare = 0;
}

//Builds board based on number of squares
Board.prototype.buildBoard = function(){
    var i,j;
    for(i = 0; i < this.squares; i++){
        this.grid[i] = [];
        for(j = 0; j < this.squares; j++){
            this.grid[i][j] = new Square();
        }
    }
};

//Updates board on each players turn
Board.prototype.update = function(row, column, symbol){
    this.grid[row][column].setSymbol(symbol);
    this.filledSquare++;
};

//Show the current state of board
Board.prototype.displayBoard = function(){
    var i, j,brd = "";
    for(i = 0; i < this.squares; i++){
        for(j = 0; j < this.squares; j++){
            brd += this.grid[i][j].getSymbol() +' - ';
        }
        brd += '\n';
    }
    console.log(brd);
};

Player.js

//Player Class
function Player(name,symbol){
    this.name = name;
    this.symbol = symbol;
}

tttgame.js

//The game class which governs the play
function TTTGame(squares){
    this.board = new Board(squares);
    this.board.buildBoard();
    this.players = [];
    this.players[0] = new Player('John','X');
    this.players[1] = new Player('Doe', 'O');

    this.getCurrentPlayer = function(){
        return (this.board.filledSquare %2 === 0 ? 'X' : 'O');
    };

    //I have written this as dummy method which should be replaced with actual logic
    this.play = function(){
        this.board.displayBoard();
        var currentPlayer = this.getCurrentPlayer();
        var row, col, symbol;
        this.board.update(0,1,currentPlayer);
        currentPlayer = this.getCurrentPlayer();
        this.board.update(0,0,currentPlayer);
        currentPlayer = this.getCurrentPlayer();
        this.board.update(1,1,currentPlayer);
        currentPlayer = this.getCurrentPlayer();
        this.board.update(2,1,currentPlayer);
        currentPlayer = this.getCurrentPlayer();
        this.board.update(1,0,currentPlayer);
        this.board.displayBoard();
    }

    //It will contain method to decide winner or tie
}

GameStarter.js

//Starts the game
(function GameStarter(){
    var tttGame = new TTTGame(3);
    tttGame.play();
})();
share|improve this question

2 Answers 2

Overall this looks pretty good. Just a couple of things come to mind:

Square.js

  1. The setSymbol method should throw errors instead of returning strings, especially since I don't see any place where those strings are handled in a manor allowing the user to correct their mistake.
  2. The error message when the square is occupied just needs a slight grammatical correction: This square is already filled (filled instead of filler)

TTTGame.js

  1. Create all methods on the prototype. You aren't using any private function scope variables, so putting the methods on the prototype will be more efficient.

General Comments

The name TTTGame is kind of obtuse. I know it is probably an abbreviation for "Tick Tack Toe Game" but I think a more descriptive name is in order. All around, throwing these classes into a namespace would be useful. Consider the semantic differences between these two sets of classes:

  • Square (is it a tick tack toe square or a two dimensional representation of a plane in mathematics)
  • Board (Diving board? Game board? Discussion board?)
  • TTTGame (What does TTT stand for?)

Vs:

  • TickTackToe.Square
  • TickTackToe.Board
  • TickTackToe.Game

No explanation necessary for the second set of class names.

All in all, this looks pretty good.

share|improve this answer

Interesting,

from a once over:

  • You seem to do double data maintenance in Square, you can always tell from the value of symbol whether the place is occupied or not, so I would make isOccupied a function.

    //Square class
    function Square(){
        this.symbol;
    }
    
    Square.prototoype.isOccupied(){
        return this.symbol ? true : false;
    }
    

    This should also make setSymbol a bit easier

  • The code in setSymbol seems a bit paranoid, not trusting the input and throwing unhandled strings when the input is not 'X' or 'O'. Since you leave the player symbol open-ended, I would leave this part open-ended as well and just allow square.symbol = .. since you no longer have to set isOccupied.

  • This:

    Square.prototype.getSymbol = function(){
        return this.symbol;
    };
    

    has little value, why would you not provide direct access to symbol? Especially since you have no similar getter for isOccupied.

  • sqaures -> squares (actually I would call this size, since size 3 will give 9 squares..)

  • filledSquare -> filledSquares

  • brd -> board and unless you are going for a Node.js tic-tac-toe, I would focus on getting an HTML output instead of console.log.

  • From a high level perspective ( especially once you start checking for winning conditions ) you are several times looping over all squares. It could be convenient to be able to pass a function to a function which will apply the provided function to all squares.

  • This make Playerhaving a symbol a bit moot:

    this.getCurrentPlayer = function(){
        return (this.board.filledSquare %2 === 0 ? 'X' : 'O');
    };
    

    How about

    this.getCurrentPlayer = function(){
        return this.players[ this.board.filledSquare %2 ];
    };
    
  • var row, col, symbol; <- Wot ?

All, in all, this looks like a good start, but you still have a ways to go.

share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.