Tic-Tac-Toe JavaScript readability and additional suggestions

Question

Requires Underscore and jQuery

I'm looking for suggestions on how to make this a little more readable and additional suggestions on flow, logic, etc.

The game can be run in a browser by just copying and pasting the code(HTML/JS/CSS) below.

JavaScript: app/js/app.js

var Utils = {
    rules: {
        isHorizontal: function (array, move) {
            return array[0] === move[0];
        },
        isVertical: function (array, move) {
            return array[1] === move[1];
        },
        isDiagonalDown: function (array) {
            return array[0] === array[1];
        },
        isDiagonalUp: function (array, boardSize) {
            return (array[0] + array[1]) === (boardSize - 1);
        }
    },

    isDuplicate: function (moves, move) {
        var duplicateAttempt = false;
        for (var x = 0, l = moves.length; x < l; x++) {
            if ((moves[x][0] === move[0]) && (moves[x][1] === move[1])) {
                duplicateAttempt = true;
            }
        }
        return duplicateAttempt;
    },

    nextPlayerIndex: function (prevPlayer, players) {
        if (prevPlayer === null) {
            return Utils.randomPlayerIndex(players);
        } else {
            return (prevPlayer === players.length - 1) ? 0 : prevPlayer + 1;
        }
    },

    randomPlayerIndex: function (players) {
        return Math.floor(Math.random() * (players.length));
    }
};

var Markup = {
    template: function (players, size) {
        var playerList = function () {
            var list = '<ul>';
            _.each(players, function (player) {
                list = list + '<li>' + player.symbol + ' / ' + player.name;
            });
            return list;
        },
            table = '<table>',
            thead = '<thead><tr><th colspan="' + size + '"> ' + playerList() + ' <tbody>'
            row = '<tr>',
            cell = '<td style="width: ' + (100 / size).toFixed(2) + '%;">';

            table = table + thead;

        for (var x = 0; x < size; x++) {
            table = table + row;
            for (var y = 0; y < size; y++) {
                table = table + cell;
            }
        }
        return $(table);
    },

    off: function (gameboard) {
        gameboard.off('click', 'td').find('table').addClass('complete');
    },

    getCell: function (index, gameboard) {
        return $('tbody tr', gameboard).eq(index[0]).children().eq(index[1]);
    },

    markCell: function (index, symbol, gameboard) {
        var cell = Markup.getCell(index, gameboard);
        cell.html(symbol ? symbol : 'x').addClass('played');
    },

    highlightCells: function (array, gameboard) {
        _.each(array, function (index) {
            var cell = Markup.getCell(index, gameboard);
            cell.addClass('winning-cell');
        });
    },

    markNextPlayer: function (gameboard, game) {
        $('thead li', gameboard).removeClass('turn').eq(game.currentPlayer).addClass('turn');
    }
};

var Game = function (players, size) {
    this.data = {
        players: [],
        size: size || 3,
        allMoves: [],
        ended: false,
        prevPlayer: null,
        currentPlayer: null,
        winningMoves: {},
        winCondition: [],
        status: {}
    };

    _.each(players, function (player) {
        _.extend(player, { moves: [], isWinner: false });
        this.data.players.push(player);
    }, this);

    this.data.currentPlayer = Utils.randomPlayerIndex(this.data.players);
};

_.extend(Game.prototype, {
    play: function (player, move) {
        var g = this.data,
            p = g.players[g.currentPlayer];

        g.winningMoves = { horizontal: [], vertical: [], diagonalDown: [], diagonalUp: [] };
        g.status.wrongPlayer = g.currentPlayer !== player;
        g.status.isDuplicate = Utils.isDuplicate(g.allMoves, move);

        if (g.status.wrongPlayer) { return g; }
        if (g.status.isDuplicate) { return g; }

        g.allMoves.push(move);
        p.moves.push(move);

        if (p.moves.length >= g.size) {
            _.each(p.moves, function (pmove) {
                this.checkRules(pmove, move);
            }, this);

            g.winCondition = this.findWinner(g.winningMoves, g.size);
            p.isWinner = true;
        }

        if (g.allMoves.length === (g.size * g.size) && !g.winCondition) {
            _.each(g.players, function (pl) {
                pl.isWinner = null;
            });
            g.winCondition = [];
        }

        g.prevPlayer = g.currentPlayer;
        g.currentPlayer = Utils.nextPlayerIndex(g.prevPlayer, g.players);

        return g;
    },

    checkRules: function (playerMoves, lastMove) {
        if (Utils.rules.isHorizontal(playerMoves, lastMove)) {
            this.data.winningMoves.horizontal.push(playerMoves);
        }

        if (Utils.rules.isVertical(playerMoves, lastMove)) {
            this.data.winningMoves.vertical.push(playerMoves);
        }

        if (Utils.rules.isDiagonalDown(playerMoves)) {
            this.data.winningMoves.diagonalDown.push(playerMoves);
        }

        if (Utils.rules.isDiagonalUp(playerMoves, this.data.size)) {
            this.data.winningMoves.diagonalUp.push(playerMoves);
        }
    },

    findWinner: function (moves, size) {
        var winner = [];
        _.each(moves, function (move) {
            if (move.length >= size) {
                winner = move;
            }
        }, this);
        return winner;
    }
});

$(function () {
    var gameboard = $('.gameboard'),

        players = [
            { name: 'John Doe', symbol: 'x' },
            { name: 'Peter Brown', symbol: 'o' }
        ],

        startGame = function (gameboard, players) {
            var game = new Game(players, 3);

            gameboard.append(Markup.template(game.data.players, game.data.size));

            Markup.markNextPlayer(gameboard, game.data);

            gameboard.on('click', 'td', function () {
                var move = [$(this).parent()[0].sectionRowIndex, this.cellIndex],
                    play = game.play(game.data.currentPlayer, move);

                if (play.status.wrongPlayer) { console.log('Wrong player'); return; }
                if (play.status.isDuplicate) { console.log('Position already played'); return; }

                Markup.markCell(move, play.players[play.prevPlayer].symbol, gameboard);

                if (_.isArray(play.winCondition) && play.winCondition.length) {
                    Markup.highlightCells(play.winCondition, gameboard);
                    Markup.off(gameboard);
                }

                if (!play.winCondition.length) {
                    Markup.markNextPlayer(gameboard, play);
                }
            });
        },

        clearGame = function (gameboard) {
            $('table', gameboard).remove();
            gameboard.off('click');
        };

    $('#startNewGame').click(function () {
        clearGame(gameboard);
        startGame(gameboard, players);
    });

}());

HTML: /index.html

<html>
  <head>
    <title>Tic Tac Toe</title>
      <link href="app/css/app.css" rel="stylesheet" media="screen" />
  </head>
  <body>
    <button id="startNewGame">Start New Game</button>
    <div class="gameboard"></div>

    <script src="http://code.jquery.com/jquery.js"></script>
    <script src="http://underscorejs.org/underscore-min.js"></script>
    <script src="app/js/app.js"></script>
  </body>
</html>

CSS: /app/css/app.css

table {
    border: 0 solid #ccc;
    margin: 1% auto;
    width: 60%;
    height:40em;
}

    table.complete td:hover {
        background-color:#707070;
    }

    table th {
        font-family:Arial, Verdana;
        height:3em;
        background-color:#ffd800;
        padding: 0 1em;
    }

    table td {
        border: 0 solid #ccc;
        text-align:center;
        font-size: 10em;
        line-height:0;
        background-color: #707070;
        color: #fff;
        text-transform:uppercase;
    }

        table td.played:hover {
            background-color:#707070;
        }

        table td:hover {
            background-color:#878686;
        }

        table td.winning-cell:hover {
            background-color:#000;
        }

ul {
    list-style:none;
    margin:0;
    padding: 0;
}

    ul li {
        float:left;
        width:50%;
        text-align:left;
        color:#fff6c6; 
    }

        li.turn {
            color:#000;
        }

.winning-cell {
    background-color:#000;
}

I took the liberty of copying your code (completely as-is) into this jsfiddle for people to try out. — Flambino
– Flambino, Commented Oct 30, 2013 at 22:52
Thanks Flambino, I'll go make the necessary updates to make sure it works properly. I'll add a jsFiddle next time around. — Jack
– Jack, Commented Oct 31, 2013 at 13:47

Alex Netkachov · Accepted Answer · 2013-10-31 00:02:35Z

You code is looks good. I want to suggest a few coding style improvements:

remove unnecessary punctuators (example: (players.length) in return Math.floor(Math.random() * (players.length)););
ifs with one statements are not consistent (sometimes one line, sometimes several lines)
var is frequently recommended to be placed on top of the function scope because it actually works this way (defines variable in function scope, not in statement scope).

Other notes:

I recommend to remove the class Utils. It is not a good name for the container of the game rules. Other methods also may be inlined or moved to better location.
I see that you try to separate different parts of the code (Markup, Game, etc). I suggest to put a little bit more effort in this and apply MVC pattern (see my article about it: http://alexatnet.com/articles/model-view-controller-mvc-javascript ). It is not necessary to strictly follow the article but it may be useful to try to understand the MVC concept.

isDuplicate method:

isDuplicate can be shortened by directly returning the value.
l is usually not good looking name for the variable, because it easy can be mixed up with the 1, especially with some fonts. For example, x < l can be read as x < 1.
it is not necessary to save move.length to l. it is optimized well and does not make significant impact (especially in this case).

So it may be rewritten as follows:

isDuplicate: function (moves, move) {
    for (var x = 0; x < moves.length; x++) {
        if ((moves[x][0] === move[0]) && (moves[x][1] === move[1])) {
            return true;
        }
    }
    return false;
},

nextPlayerIndex method:

the else is redundant, because return breaks the if statement.

it may be shortened as follows:

return null === prevPlayer ? Utils.randomPlayerIndex(players) :
        (prevPlayer + 1) % players.length;

May be removed, because it looks like prevPlayer is never null.

Markup class:

Use string concatenation for generating the HTML code is not very good, because it may case troubles if the data contains the markup. It not happens now, but may be in the future.

The rest of code looks good enough, but probably will be modified a lot if you will follow the recommendations and apply the MVC pattern to your code.

Hey Alex, amazing feedback. I'm reading your article now to find ways of restructuring the code to be more MVC like. The other suggestions are fantastic as well. Thanks!! — Jack
– Jack, Commented Oct 31, 2013 at 13:48
var is frequently recommended to be placed on top of the function scope because it actually works this way (defines variable in function scope, not in statement scope). --- Did you have an example in my code to point at? Are you suggesting removing the var declarations in the for-loop-statements? — Jack
– Jack, Commented Oct 31, 2013 at 14:17
Are you suggesting removing the var declarations in the for-loop-statements? --- and move all of them to the top of the function. Yes. This is how it actually works and it does not gives you the false feeling of using two variables while it is actually one. — Alex Netkachov
– Alex Netkachov, Commented Oct 31, 2013 at 14:32
I can review you code when you done with the changes if you wish: alexatnet.com/projects/code-review-4u Cheers! — Alex Netkachov
– Alex Netkachov, Commented Nov 1, 2013 at 21:32

Stack Exchange Network

Tic-Tac-Toe JavaScript readability and additional suggestions

1 Answer 1

You must log in to answer this question.

Hot Network Questions

Tic-Tac-Toe JavaScript readability and additional suggestions

1 Answer 1

You must log in to answer this question.

Related

Hot Network Questions