Problem
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();
})();
Solution
Overall this looks pretty good. Just a couple of things come to mind:
Square.js
- 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 manner allowing the user to correct their mistake. - 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
- 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.
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 allowsquare.symbol = ..
since you no longer have to setisOccupied
. -
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 forisOccupied
. -
sqaures
->squares
(actually I would call thissize
, sincesize
3 will give 9squares
..) -
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 ofconsole.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
Player
having 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.