4
\$\begingroup\$

This is really the first full app I've written in JavaScript minus CSS styling which I plan on adding later.

Are there any ways it can be improved? Any ways I can make it cleaner or easier to read?

"use strict";

// Constructor function for individual parts of game grid square
// Each grid square contains a state property of empty, x, or o.
// Also contains row and column properties
function GridSquare(column, row) {
    this.state = ko.observable('n');
    // Not an observable because location will never change
    this.row = row;
    this.column = column;
}

// Constructor for game board object
// Contains a list of grid squares and gives each grid square a row and column
// Also contains state property which determines if game is currently playing
function GameBoard() {
    this.topRow     = ko.observableArray(
        [new GridSquare('leftColumn', 'topRow'),
            new GridSquare('middleColumn', 'topRow'),
            new GridSquare('rightColumn', 'topRow')]);

    this.middleRow  = ko.observableArray(
        [new GridSquare('leftColumn', 'middleRow'),
            new GridSquare('middleColumn', 'middleRow'),
            new GridSquare('rightColumn', 'middleRow')]);
    this.bottomRow  = ko.observableArray(
        [new GridSquare('leftColumn', 'bottomRow'),
            new GridSquare('middleColumn', 'bottomRow'),
            new GridSquare('rightColumn', 'bottomRow')]);

    // The number of moves made so far
    this.moves = 0;

    // State property to determine turns and whether or not the game is finished
    // Initial state is xturn
    // States include:
    // xturn for X's turn
    // oturn for O's turn
    // draw for when a draw occurs
    // xwins for when X wins the game
    // owins for when O wins the game
    this.state = ko.observable('xturn');

    // Computed observable to determine the turn or if the game is finished
    this.turn = ko.computed( function() {
        var turn,
            state = this.state();

        switch ( state ) {
            case 'xturn':
                turn = 'x';
                break;
            case 'oturn':
                turn = 'o';
                break;
            default:
                turn = 'finished'; // Catches everything that isn't X's turn or O's turn
                break;
        }

        return turn;
    }, this);

    this.finishedText = ko.computed( function() {
        var text,
            state = this.state();

        switch ( state ) {
            case 'xturn':
                text = "X's Turn";
                break;
            case 'oturn':
                text = "O's Turn";
                break;
            case 'xwins':
                text = "X Wins!";
                break;
            case 'owins':
                text = "O Wins!";
                break;
            case 'draw':
                text = "Draw";
                break;
        }

        return text;
    }, this);
}

// A constructor for a player object. Each player object contains a name and an object
// called squares. The squares object contains lists which contain a list of squares
// that the player has claimed. Each square is added to a list by determining the
// row and column of the square and adding it to the specified row, column,
// and diagonal if applicable. When the length of any list reaches 3, the player has won.
function Player(name) {
    this.name = name;
    this.squares = {
        topRow: [],
        middleRow: [],
        bottomRow: [],
        leftColumn: [],
        middleColumn: [],
        rightColumn: [],
        leftDiagonal: [],
        rightDiagonal: []
    };
}

var ViewModel = function() {
    var self = this;

    // Create a new GameBoard object
    this.gameBoard = new GameBoard();

    this.playerX = new Player('x');
    this.playerO = new Player('o');

    this.currentPlayer = this.playerX;

    this.changeCurrentPlayer = function() {
        self.currentPlayer = self.gameBoard.turn() === 'x' ? this.playerX : this.playerO;
    };

    // Change the value of an individual grid square's state to x or o depending on turn
    // Changes the gameBoard object's state using the changeTurn method
    this.placeMarker = function(gridSquare) {
        if ( gridSquare.state() === 'n'  && self.gameBoard.turn() !== 'finished' ) {
            self.gameBoard.moves += 1;
            gridSquare.state(self.gameBoard.turn());
            self.addSquareToPlayer(gridSquare);
            if( self.gameBoard.moves > 4) {
                self.checkForWin(gridSquare.row, gridSquare.column);
            }
            self.changeTurn(self.gameBoard.state());
            self.checkDraw();
        }
    };

    // Add a square to a player's squares object. Pushes 0 because
    // the victory conditions check for length, not value.
    this.addSquareToPlayer = function(gridSquare) {
            // Add to row and column square list
            self.currentPlayer.squares[gridSquare.row].push(0);
            self.currentPlayer.squares[gridSquare.column].push(0);
            
            // Determine if square is in a diagonal and add to appropriate list(s)
            if ( gridSquare.row === "topRow" ) {
                if ( gridSquare.column === "leftColumn" ) {
                    self.currentPlayer.squares.leftDiagonal.push(0);
                } else if ( gridSquare.column === "rightColumn" ) {
                    self.currentPlayer.squares.rightDiagonal.push(0);
                }
            } else if ( gridSquare.row === "middleRow" &&
                    gridSquare.column === "middleColumn") {
                self.currentPlayer.squares.rightDiagonal.push(0);
                self.currentPlayer.squares.leftDiagonal.push(0);    
            } else if ( gridSquare.row === "bottomRow") {
                if ( gridSquare.column === "leftColumn" ) {
                    self.currentPlayer.squares.rightDiagonal.push(0);
                } else if ( gridSquare.column === "rightColumn" ) {
                    self.currentPlayer.squares.leftDiagonal.push(0);
                }
            }
    };

    // Changes the state of the gameBoard object to reflect whose turn it is
    this.changeTurn  = function(currentState) {

        switch(currentState) {
            case 'xturn':
                self.gameBoard.state('oturn');
                break;
            case 'oturn':
                self.gameBoard.state('xturn');
                break;
        }
        self.changeCurrentPlayer();
    };

    // Method to determine if game is a win or draw
    this.checkForWin = function(row, column) {
        // Check for diagonal wins
        if ( row === 'topRow' &&
                ( column === 'leftColumn' || column === 'rightColumn' )) {
            self.checkDiagonal();
        } else if ( row === 'middleRow' &&
                ( column === 'middleColumn' )) {
            self.checkDiagonal();
        } else if ( row === 'bottomRow' &&
                ( column === 'leftColumn' || column === 'rightColumn' )) {
            self.checkDiagonal();
        }

        //Check for row and column wins
        self.checkRow(row);
        self.checkColumn(column);
    };

    this.checkRow = function(row) {
        if ( self.currentPlayer.squares[row].length === 3 ) {
            self.gameBoard.state(self.currentPlayer.name + "wins");
        }
    };

    this.checkColumn = function(column) {
        if ( self.currentPlayer.squares[column].length === 3 ) {
            self.gameBoard.state(self.currentPlayer.name + "wins");
        }
    };

    this.checkDiagonal = function() {
        if ( self.currentPlayer.squares.leftDiagonal.length === 3 ) {
            self.gameBoard.state(self.currentPlayer.name + "wins");
        } else if ( self.currentPlayer.squares.rightDiagonal.length === 3 ) {
            self.gameBoard.state(self.currentPlayer.name + "wins");
        }
    };

    this.checkDraw = function() {
        if ( self.gameBoard.moves === 9 &&
                !(self.gameBoard.state() =="xwins" ||
                self.gameBoard.state() == "owins")) {
            self.gameBoard.state('draw');
        }
    };
};

ko.applyBindings(new ViewModel());
<!DOCTYPE html>
<html>
    <head>
        <meta charset="UTF-8">
        <title>Tic-Tac-Toe</title>
    </head>
    <body>
        <div class="gameboard" data-bind="with: gameBoard">
            <div class="row" id="top" data-bind="foreach: topRow">
                <span data-bind="text: state, click: $parents[1].placeMarker"></span>
            </div>
            <div class="row" id="middle" data-bind="foreach: middleRow">
                <span data-bind="text: state, click: $parents[1].placeMarker"></span>
            </div>
            <div class="row" id="bottom"data-bind="foreach: bottomRow">
                <span data-bind="text: state, click: $parents[1].placeMarker"></span>
            </div>
        </div>
        <div data-bind="with: gameBoard">
            <span id="finishedtext" data-bind="text: finishedText"></span>
        </div>
        <script src="https://cdnjs.cloudflare.com/ajax/libs/knockout/3.2.0/knockout-min.js"></script>

        <script type="text/javascript" src="js/tictactoe.js"></script>
    </body>
</html>

\$\endgroup\$

1 Answer 1

3
\$\begingroup\$

On surface it's a well laid out code with bunch of helpful comments and good method names.

However, looking deeper the data model of this game looks rather odd. Instead of storing the game state in a simple 2D array as such:

var grid = [
    [0, 0, 0],
    [0, 0, 0],
    [0, 0, 0],
];

Each cell is instead indexed by named coordinates like ("middleColumn", "topRow"). This completely hard-codes this solution to 3x3 grid. Attempting to extend this solution into a larger grid would lead to a lot of trouble.

Another big oddity is that in addition to keeping the gameboard state in the grid where it naturally belongs, it's also partly kept in each player object:

function Player(name) {
    this.name = name;
    this.squares = {
        topRow: [],
        middleRow: [],
        bottomRow: [],
        leftColumn: [],
        middleColumn: [],
        rightColumn: [],
        leftDiagonal: [],
        rightDiagonal: []
    };
}

Each of these arrays in Player.squares keeps a count of how many winning points a player has already scored in rows, columns or diagonals. These arrays are checked after each turn to determine if player has won - first player to get 3 marks into any of these arrays wins.

However, instead of just looping through all these arrays to determine the winner, some kind of very convoluted logic is applied, which only checks the row, column and diagonal where the last move was made. Maybe an optimization, but completely needless for such a small game.

But really... you should just store your data in a simple 3x3 array and inspect this array after each turn to determine the winner. And try to think of how would you build a tic-tac-toe of 100x100 grid.

\$\endgroup\$
3
  • \$\begingroup\$ I'm all for generalization and extensibility. But, can tic-tac-toe make any sense at all with any other than a 3x3 grid? \$\endgroup\$
    – janos
    Commented May 22, 2015 at 6:39
  • \$\begingroup\$ @janos ever heard of Machinarium? There is a minigame much like this one, where one of the players needs to place a 5-piece long straight line to win, on a board like 10x10 or about that. \$\endgroup\$
    – D-side
    Commented May 22, 2015 at 7:22
  • \$\begingroup\$ I see your point @D-side: on a NxN grid, if the required straight length can be tweaked (and < N) to make it interesting. Thanks! \$\endgroup\$
    – janos
    Commented May 22, 2015 at 8:25

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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