Anyone remember "War games?"
I have a very long way to go, but I thought it would be interesting to make a version of Tic Tac Toe which plays itself. At present the AI is "choose a random available space" (i.e nonexistent). I will build on that later, but for now I would value some feedback on my basic approach. I've used an object literal pattern, which seems to make sense for the task. I'm curious to learn about any improvements I could make in terms of the structure and implementation. Also, what did I do well please? Is there anything obviously amateurish about my approach?
const TicTacToe = {
setup: function() {
this.player1 = {
name: "Crosses",
symbol: "X"
};
this.player2 = {
name: "Naughts",
symbol: "0"
};
this.restart();
},
restart: function() {
this.board = [
[ "", "", "" ],
[ "", "", "" ],
[ "", "", "" ]
];
this.currentPlayer = Math.floor( Math.random() * 2 ) == 0 ? this.player1 : this.player2;
this.playRound();
},
playRound: function() {
console.log( this.currentPlayer.name + " start." );
while ( !this.isWinningCombination( this.player1.symbol, this.board ) && !this.isWinningCombination( this.player2.symbol, this.board ) && !this.isDraw( this.board ) ) {
this.sleep( 500 );
this.playerTurn( this.currentPlayer );
}
},
playerTurn: function( player ) {
while ( true ) {
let pos = [ Math.floor( Math.random() * 3 ), Math.floor( Math.random() * 3 ) ];
if ( this.board[ pos[ 0 ] ][ pos[ 1 ] ] == "" ) {
this.board[ pos[ 0 ] ][ pos[ 1 ] ] = player.symbol;
this.displayBoard( this.board );
if ( this.isWinningCombination( player.symbol, this.board ) ) {
console.log( this.currentPlayer.name + " win!" )
this.playAgainDialogue();
} else if ( this.isDraw( this.board ) ) {
console.log( "It's a draw" );
this.playAgainDialogue();
}
this.currentPlayer = this.currentPlayer == this.player1 ? this.player2 : this.player1;
break;
}
continue;
}
},
displayBoard: function( board ) {
display = "";
// First row
display += board[ 0 ][ 0 ] == "" ? " " : ` ${board[0][0]}`;
display += "|";
display += board[ 0 ][ 1 ] == "" ? " " : `${board[0][1]}`;
display += "|";
display += board[ 0 ][ 2 ] == "" ? " \n" : `${board[0][2]}\n`;
// Filler
display += "--|-|--\n";
// Second row
display += board[ 1 ][ 0 ] == "" ? " " : ` ${board[1][0]}`;
display += "|";
display += board[ 1 ][ 1 ] == "" ? " " : `${board[1][1]}`;
display += "|";
display += board[ 1 ][ 2 ] == "" ? " \n" : `${board[1][2]}\n`;
// Filler
display += "--|-|--\n";
// Third row
display += board[ 2 ][ 0 ] == "" ? " " : ` ${board[2][0]}`;
display += "|";
display += board[ 2 ][ 1 ] == "" ? " " : `${board[2][1]}`;
display += "|";
display += board[ 2 ][ 2 ] == "" ? " \n" : `${board[2][2]}\n`;
console.log( display );
console.log( "" );
},
// Check for win
isWinningCombination: function( symbol, board ) {
return [ // Rows
board[ 0 ][ 0 ] === symbol && board[ 0 ][ 1 ] === symbol && board[ 0 ][ 2 ] === symbol,
board[ 1 ][ 0 ] === symbol && board[ 1 ][ 1 ] === symbol && board[ 1 ][ 2 ] === symbol,
board[ 2 ][ 0 ] === symbol && board[ 2 ][ 1 ] === symbol && board[ 2 ][ 2 ] === symbol,
// Columns
board[ 0 ][ 0 ] === symbol && board[ 1 ][ 0 ] === symbol && board[ 2 ][ 0 ] === symbol,
board[ 0 ][ 1 ] === symbol && board[ 1 ][ 1 ] === symbol && board[ 2 ][ 1 ] === symbol,
board[ 0 ][ 2 ] === symbol && board[ 1 ][ 2 ] === symbol && board[ 2 ][ 2 ] === symbol,
// Diagonals
board[ 0 ][ 0 ] === symbol && board[ 1 ][ 1 ] === symbol && board[ 2 ][ 2 ] === symbol,
board[ 0 ][ 2 ] === symbol && board[ 1 ][ 1 ] === symbol && board[ 2 ][ 0 ] === symbol
].
includes( true );
},
isDraw: function( board ) {
return !this.isWinningCombination( this.player1.symbol, board ) && !this.isWinningCombination( this.player2.symbol, board ) && !this.board.some( row => row.includes( "" ) );
},
// Terrible practice, they say.
sleep: function( delay ) {
const start = new Date().getTime();
while ( new Date().getTime() < start + delay );
},
playAgainDialogue : function(){
console.log("Type TicTacToe.restart() to play again");
}
};
TicTacToe.setup();
<h1>Please open your console to start</h1>
Please note the snippet behaves differently to running in a browser (no "sleep" functionality).
1 Answer 1
What you did right
It's good that you designed an object, and that you separated some operations to functions. I think it would be good to go further: more dedicated objects, smaller functions, helper functions. More on all that below.
Decompose to smaller functions
The playerTurn
function looks really complex,
with deeply nested code:
a while (true)
outer loop,
and then nested conditionals.
Try to split to smaller steps. The steps performed by this function:
- Find randomly an available position
- Mark the player's symbol on the board
- Check if the game is over
If you move the first step to a dedicated function, you get a much simpler loop:
findEmptyPos: function() {
while (true) {
let pos = Pos.create(randomInt(3), randomInt(3));
if (this.board.isEmpty(pos)) return pos;
}
}
Thanks to this new function,
the playerTurn
will no longer have a complex while
loop in it.
Note that I extracted the repeated Math.floor(Math.random() * 3)
calls to a helper function call randomInt(3)
.
It's good to avoid duplicated code, much less to type, and with good descriptive names, easier and less to read.
I also replaced other elements:
Position used to be represented as an array of two elements. It worked, but it's a lazy solution with several flaws: the index 0 and 1 by themselves don't mean anything, and also nothing prevents you from assigning to other indexes. It probably makes sense to create a proper
Pos
abstraction, withrow
andcol
fields. Those fields will have meaning thanks to their names.The check if a position is empty used verbose code like
this.board[pos[0]][pos[1]] == ""
, comparing to a hardcoded value. Instead of that, it's better to have aBoard
abstraction with anisEmpty
function, that would encapsulate and hide the implementation detail that "empty" is represented as an empty string.
Lastly in the playerTurn
function,
this snippet is a bit unfortunate:
if ( this.isWinningCombination( player.symbol, this.board ) ) { console.log( this.currentPlayer.name + " win!" ) this.playAgainDialogue(); } else if ( this.isDraw( this.board ) ) { console.log( "It's a draw" ); this.playAgainDialogue(); } this.currentPlayer = this.currentPlayer == this.player1 ? this.player2 : this.player1;
That is,
why let the this.currentPlayer
switch take place after calling this.playAgainDialogue();
?
It doesn't seem to make sense,
and that the if
and else if
there should have had a break
.
Short-circuit evaluation
This may seem kind of clever, but it's not:
return [ // Rows board[ 0 ][ 0 ] === symbol && board[ 0 ][ 1 ] === symbol && board[ 0 ][ 2 ] === symbol, board[ 1 ][ 0 ] === symbol && board[ 1 ][ 1 ] === symbol && board[ 1 ][ 2 ] === symbol, board[ 2 ][ 0 ] === symbol && board[ 2 ][ 1 ] === symbol && board[ 2 ][ 2 ] === symbol, // Columns ... ]. includes( true );
The problem is that all the conditions in the array are computer,
and then the includes(...)
will return when it finds the first that was true
. That's too late.
If instead of creating an array,
you had chained the conditions with ||
then short-circuiting would apply,
and once there is a match (any winning row, or column, etc),
the remaining conditions would not be evaluated.
Benefit more from this
Some parameters are passed around to functions,
most noticeably this.board
,
when those functions could access the board via this.board
.
You can safely remove such parameters and access the values directly.
Simplify printing
The printing of the board is very tedious and error-prone. Let's take a closer look at this:
// First row display += board[ 0 ][ 0 ] == "" ? " " : ` ${board[0][0]}`; display += "|"; display += board[ 0 ][ 1 ] == "" ? " " : `${board[0][1]}`; display += "|"; display += board[ 0 ][ 2 ] == "" ? " \n" : `${board[0][2]}\n`;
For board[0][0]
there is a space prefix in both branches of the ternary.
For board[0][2]
there is a \n
in both branches of the ternary.
=> It's better to not repeat characters in both branches of ternaries, for example:
// First row
display += " " + (board[ 0 ][ 0 ] == "" ? " " : `${board[0][0]}`);
display += "|";
display += board[ 0 ][ 1 ] == "" ? " " : `${board[0][1]}`;
display += "|";
display += (board[ 0 ][ 2 ] == "" ? " " : `${board[0][2]}`) + "\n";
Now the lines look more similar. When things look similar, they make you think of further generalization opportunities.
You could benefit more from the `...`
strings in JavaScript,
and also of the fallback values with || ...
:
// First row
display += ` ${board[0][0] || ' '}`;
display += "|";
display += `${board[0][1] || ' '}`;
display += "|";
display += `${board[0][2] || ' '}\n`;
Minor technical issues
The last statement in the
while
loop ofplayerTurn
iscontinue
. That's pointless and should be removed.Some lines are extremely long. It's hard to read code like that, better to split to multiple lines.
In
displayBoard
you forgot to use thelet
keyword for thedisplay
variable.
-
1\$\begingroup\$ Brilliant, that's exactly the kind of feedback I was looking for. Thank you. \$\endgroup\$Robin Andrews– Robin Andrews2018年08月12日 11:27:54 +00:00Commented Aug 12, 2018 at 11:27
Explore related questions
See similar questions with these tags.