4
\$\begingroup\$

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).

\$\endgroup\$
4
\$\begingroup\$

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:

  1. Find randomly an available position
  2. Mark the player's symbol on the board
  3. 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, with row and col 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 a Board abstraction with an isEmpty 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 of playerTurn is continue. 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 the let keyword for the display variable.

\$\endgroup\$
  • 1
    \$\begingroup\$ Brilliant, that's exactly the kind of feedback I was looking for. Thank you. \$\endgroup\$ – Robin Aug 12 '18 at 11:27

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.