Tic-Tac-Toe engine in JavaScript

Question

I just cloned (Github clone, not literal clone) someone's Tic-Tac-Toe game and I was originally going to make a pull request, but I ended up making too many changes, the code is simply unrecognizable now.

here is the original repository

My aim is not creating the best Tic-Tac-Toe engine (if the idea of a Tic-Tac-Toe engine isn't funny in the first place), and I will admit that my intention with the refactorings has been posting the final code to CodeReview all along. So, I don't want recommendations for improvements related to the Tic-Tac-Toe-related stuff, I want general recommendations about code structure, whether or not the patterns I am using make any sense, or anything else that you think would be better implemented in another way.

Here is the engine's code:

const TicTacToe = (function() {
  // returns the index of an element within its parent
  const elementIndex = function elementIndex(el) {
    return Array.from(el.parentNode.children).indexOf(el);
  };

  const checkMovesHasCombination =
    function checkMovesHasCombination(combination, moves) {
      return combination.every((num) => moves.includes(num));
    };

  /* returns the winning combinations for the specified table-size in a
  2d array. */
  const getWinningCombinations = function getWinningCombinations(tableSize) {
    return getHorizontalWinningCombinations(tableSize)
      .concat( getVerticalWinningCombinations(tableSize) )
      .concat( getDiagonalWinningCombinations(tableSize) );
  };

  const getHorizontalWinningCombinations =
    function getHorizontalWinningCombinations(tableSize) {
      const combinations = [];
      for (let i = 0; i < tableSize; i++) {
        const combination = [];
        for (let a = 0; a < tableSize; a++) {
          combination.push( a + (i * tableSize) );
        }
        combinations.push(combination);
      }
      return combinations;
    };

  const getVerticalWinningCombinations =
    function getVerticalWinningCombinations(tableSize) {
      const combinations = [];
      for (let i = 0; i < tableSize; i++) {
        const combination = [];
        for (let a = 0; a < tableSize; a++) {
          combination.push( i + (a * tableSize) );
        }
        combinations.push(combination);
      }
      return combinations;
    };

  const getDiagonalWinningCombinations =
    function getDiagonalWinningCombinations(tableSize) {
      const combinations = [[], []];
      for (let i = 0; i < tableSize; i++) {
        combinations[0].push( i + (i * tableSize) );
        combinations[1].push( (tableSize - 1 - i) + (i * tableSize) );
      }
      return combinations;
    };

  // generates an HTML table of the specified size (size X size)
  const generateTable = function generateTable(size) {
    const table = document.createElement('table');
    for ( let i = 0; i < size; i++ ) {
      const tr = table.appendChild( document.createElement('tr') );
      for ( let a = 0; a < size; a++ ) {
        tr.appendChild( document.createElement('td') );
      }
    }
    return table;
  };

  const mapPlayer = function mapPlayer(player) {
    const playerCopy = Object.assign(player);
    playerCopy.moves = [];
    return playerCopy;
  };

  /* calculates the 'num' attribute for a cell, which represents
  the cell's position in the grid. */
  const getCellPosition = function getCellPosition(el) {
    return elementIndex(el) + (
      elementIndex(el.parentNode) * el.parentNode.children.length
    );
  };

  const Cell = function Cell(el, num) {
    this.el = el;
    this.setNumAttribute(num);
    this.player = null;
  };

  Cell.prototype.getNumAttribute = function getNumAttribute() {
    return parseInt(this.el.getAttribute('data-num'));
  };

  Cell.prototype.setNumAttribute = function setNumAttribute(num) {
    this.el.setAttribute('data-num', num);
    return this.el;
  };

  Cell.prototype.select = function select(player) {
    // if not empty, return 'false'.
    if (this.player) {
      return false;
    }
    this.player = player;
    this.el.innerHTML = this.player.char;
    if ( this.player.className ) {
      this.el.classList.add( this.player.className );
    }
    return true;
  };

  Cell.prototype.clear = function clear() {
    // if already empty, return.
    if (!this.player) {
      return;
    }
    this.el.innerHTML = '';
    if ( this.player.className ) {
      this.el.classList.remove( this.player.className );
    }
    this.player = null;
  };

  /*
  * 'size': an integer representing the number of both the columns and rows of
  * the game grid.
  * 'players': an object that has a 'char' (specifying the symbol to mark cells
  * with for the player) and a 'className' property (specifying the className
  * to be added to cells occupied by a player).
  *
  */
  const TicTacToe = function TicTacToe(size, players) {
    if ( !Number.isInteger(size) ) {
      throw new Error('You have to pass an integer for the "size" argument.');
    } else if ( !Array.isArray(players) || !players.length >= 2 ) {
      throw new Error(
        `You have to pass an array that has at least 2 elements for the
        "players" argument.`
      );
    } else {
      players.forEach( (player) => {
        if ( !(typeof player.char === 'string') ) {
          throw new Error(
            `Each element in the "players" argument must have a string
            "char" property.`
          );
        }
      } );
    }

    this.size = size;
    this.players = players.map( (player) => mapPlayer(player) );
    this._playersInitial = players;

    this._winningCombinations = getWinningCombinations(this.size);
    this._gameResolved = false;

    this._table = generateTable(this.size);
    this._cells = Array.from( this._table.getElementsByTagName('td') )
      .map( (el) => new Cell( el, getCellPosition(el) ) );
    this._cells.clearAll = function clearAll() {
      this.forEach((cell) => cell.clear());
    };
    this._cells.allOccupied = function allOccupied() {
      return this.every( (cell) => cell.player );
    };

    this._selectHandler = null;
    this._turnHandler = null;
    this._winHandler = null;
    this._drawHandler = null;
  };

  // checks if a player has won, if they have, it returns that player's object.
  TicTacToe.prototype._checkForWin = function checkForWin() {
    return this.players.find(
      (player) => this._winningCombinations.some(
        (combination) => checkMovesHasCombination(
          combination, player.moves
        )
      ),
      this
    );
  };

  // checks if the game is a draw (draw == all cells occupied && no one has won)
  TicTacToe.prototype._checkForDraw = function checkForDraw() {
    return this._cells.allOccupied() && !this._checkForWin();
  };

  TicTacToe.prototype._resolveGame = function resolveGame() {
    const winner = this._checkForWin();
    if (winner) {
      if ( typeof this._winHandler === 'function' ) {
        this._winHandler(winner);
      }
      this._gameResolved = true;
    } else if ( this._checkForDraw() ) {
      if ( typeof this._drawHandler === 'function') {
        this._drawHandler();
      }
      this._gameResolved = true;
    }
  };

  TicTacToe.prototype._handlePlayerTurns = function _handlePlayerTurns() {
    this.players.push( this.players.shift() );
    if ( typeof this._turnHandler === 'function'
      && !this._gameResolved === true ) {
      this._turnHandler( this.players[0] );
    }
    return this.players[0];
  };

  TicTacToe.prototype.selectCell = function selectCell(el) {
    if ( this._gameResolved === true ) {
      return;
    }
    const cell = this._cells.find( (cell) => cell.el === el );
    if ( cell && cell.select(this.players[0]) ) {
      this.players[0].moves.push( cell.getNumAttribute() );
      this._resolveGame();
      if ( typeof this._selectHandler === 'function' ) {
        this._selectHandler( el, this.players[0] );
      }
      this._handlePlayerTurns();
    }
  };

  TicTacToe.prototype.getBoard = function getBoard() {
    return this._table;
  };

  TicTacToe.prototype.reset = function reset() {
    this._cells.clearAll();
    this.players = this._playersInitial.map(
      (player) => mapPlayer(player)
    );
    this._gameResolved = false;
  };

  TicTacToe.prototype.registerTurnHandler =
    function registerTurnHandler(handler) {
      this._turnHandler = handler;
    };

  TicTacToe.prototype.registerWinHandler =
    function registerWinHandler(handler) {
      this._winHandler = handler;
    };

  TicTacToe.prototype.registerDrawHandler =
    function registerDrawHandler(handler) {
      this._drawHandler = handler;
    };

  TicTacToe.prototype.registerSelectHandler =
    function registerDrawHandler(handler) {
      this._selectHandler = handler;
    };

  return TicTacToe;
})();

Here is how you use it:

  const players = [
    {
      char: '✕',
      className: 'playerX',
      id: 1,
    },
    {
      char: '◯',
      className: 'playerY',
      id: 2,
    },
  ];

  const ticTacToe = new TicTacToe(3, players);

  const containerElement = document.getElementById('container');
  const turnText = document.getElementById('playerTurn');
  const resetButton = document.getElementById('reset');

  const ticTacToeBoard = ticTacToe.getBoard();
  containerElement.insertBefore(ticTacToeBoard, resetButton);

  Array.from(ticTacToeBoard.getElementsByTagName('td'))
    .forEach(
      (el) => el.addEventListener(
        'click',
        (e) => ticTacToe.selectCell(e.target)
      )
    );


  ticTacToe.registerTurnHandler(
    (player) => {
      turnText.innerHTML =
        'It\'s ' + (player['id'] === 1 ? 'X' : 'O') + '\'s turn.';
    }
  );


  ticTacToe.registerWinHandler(
    (player) => turnText.innerHTML =
      (player['id'] === 1 ? 'X' : 'O') + ' won the game.'
  );

  ticTacToe.registerDrawHandler(
    () => turnText.innerHTML = 'Game Over! It\'s a draw.'
  );


  resetButton.addEventListener( 'click', () => {
    ticTacToe.reset();
    turnText.innerHTML = 'It\'s X\'s turn.';
  });

---

A beautiful Tic-Tac-Toe game built on top of the above code can be found here.
And here is the Github repository.

---

Posted the newer version of the code (the one I updated with the accepted answer's recommendations) on Reddit to get a review on that as well, here it is.

Answer 1

Exposed states are bugs.

Your code is not at all good. This is an example of a banana in an OOP jungle, gaining none of OOP benefits and amplifying only the bad parts.

You have exposed almost all of the object's state which is total against OOP principles.

Some notes.

• Named function expressions have properties that meet some very rare needs. Use function declaration. EG use function name(){} not const name = function name(){}
• Underscores do not protect state, do not make private variables, and do not make good code.

• Use getters and setters EG cell.prototype.getNumAttribute and cell.prototype.setNumAttribute should have a getter and a setter and drop the noise

  They become get num(){ return this.el.dataset.num } and set num(val) { this.el.dataset.num = val }

• registerSelectHandler ???? Your code is not a gov department, it does not have a register, and the functions do not perform an action that could be considered as registering, its a simple assignment. TicTacToe.prototype.registerSelectHandler = function registerDrawHandler(handler) {

  Use API,addEvent(name, func) {... and vet the func when its added not when you are about to call it.

  Or use getters and setters set onwin(func) {... and vet the func

• Do not call callbacks directly from your code. They may throw, and your calling function is then not returned to, mangling your state and breaking all your code.

  Decouple the event via a timer event setTimeout(()=>ticTacToe.win(ticTacToe.player),0); or use the message API (see rewrite for more)

• TicTacToe should only have one instance so don't make it an instantiatable object.

• Protect your state. The ticTactoe object should only expose what is needed to use it.

  Adding properties to that you do not vet means you can not trust the state of the object. Your code is a bug waiting to happen

  Use closure to protect the object's state.

• One line functions used once in the code are just noise and should be inline.

• Be efficient. You have separated many functional steps, and each time you call the various parts your recompute the same data.

  You create a table of game cells, then you query the DOM to get each cell element, and then recalculate the cell index to create the cell. By adding two lines of code to the table creation function you can eliminate many more lines (see rewrite)

  For the winning moves, you iterate the cells indexes 3 times when you only need to do it once.

• You don't need to throw errors that contain documentation. I can see no good reason to throw any errors, as this is supposed to be release code and there should BE NO ERRORS!.

A Rewrite

An example as a rewrite, not how I would write it.

I have change the interface a little.

Rather than create a new TicTacToe(players, size) you init an existing instance via ticTacToe.init(players, size) and it returns the table.

You add events via ticTacToe.addEvent(name, listener). Events are decoupled from the code so that external errors do not interfere with your state management. They will also always fire in the correct order.

const TicTacToe = (function () {
    var resolved, winningCombos, players, cells, table, selCellCount, ready, messageId, eventQueue;
    const events = {turn : null,  win : null, draw : null, select : null};

    /* Fast events will call callback as soon as call stack is clear rather than delayed via setTimeout */
    function fireEvent(name, ...args) {
        if (events[name]) {  
            if (messageId === undefined) {
                messageId = Date.now() + Math.random() * (2<<30);   // random key to stop 3rd party code
                eventQueue = [];
                addEventListener("message", eventHandler, true)
            }
            postMessage(messageId, "");
            eventQueue.push({name, args});
        }
    }
    function eventHandler(event) {
        if (event.source === window && event.data === messageId) {
            const call = eventQueue.shift();
            events[call.name](...call.args);
            event.stopPropagation();
        }
    }        

    function createWinCombos(size) {
        winningCombos = [];
        const diag = [];
        const diag1 = [];
        combos.push(diag);
        combos.push(diag1);        
        for (let i = 0; i < size; i++) {
            const hor = [];
            const vert = [];
            combos.push(hor);
            combos.push(vert);            
            for (let a = 0; a < size; a++) {
                hor.push(a + (i * size));
                vert.push(i + (a * size));
                if (i === a) { diag.push(i + a * size) }
                if (i === (size - 1) - a) { diag.push(i + a * size) }
            }
        }
        return combos;
    }

    function createTable(size) {
        cells = [];
        selCellCount = 0;
        const table = document.createElement('table');
        for (let i = 0; i < size; i++) {
            const tr = table.appendChild(document.createElement('tr'));
            for (let a = 0; a < size; a++) {
                const cell = tr.appendChild(document.createElement('td'));
                cell.dataset.num = size * i + a;
                cells.push(new Cell(cell));
            }
        }
        return table;
    };

    function Cell(element) {
        var player;
        const cellIdx = Number(element.dataset.num);
        const cell =  {
            get value() { return player },
            set value(who) {
                if(who === ""){
                    if (player) { 
                        element.textContent = '';
                        if (player.className) { 
                            element.classList.remove(player.className); 
                        }
                        player = undefined;
                        selCellCount --;
                    }
                }else{
                    if (!player) {
                        player = who;
                        player.moves.push(cellIdx);
                        element.textContent = player.char;
                        if (player.className) { 
                            element.classList.add(player.className); 
                        }
                        fireEvent("select", element, player);
                        selCellCount ++;
                    }
                }
            },
        }
        return cell;
    }

    function resolve() {
        const winner = players.find(player => 
            winningCombos.some(combo => 
                combo.every(num => player.moves.includes(num))
            )
        );
        if (winner) { fireEvent("win", winner) }
        else if (selCellCount === cells.length) { fireEvent("draw") }
        else { return false }
        return resolved = true;
    }

    const API = {
        addEvent(name, listener) {
            if (typeof listener === "function" && events[name]) {  event[name] = listener }
        },
        removeEvent(name) { 
            if (events[name]) { events[name] = undefined } 
        },
        init(thePlayers, size = 3) {            
            players = thePlayers.map(player => ({...player, moves : []}));
            createWinCombos(size);
            table = createTable(size);
            resolved = false;
            ready = true;
            return table;
        },
        set player(who) {
            if (ready) {
                for(var i = 0; i < players.length; i++) {
                    if(players[i].char === who.char){
                        players.unshift(players.splice(i, 1)[0]);
                        if(! resolved) { fireEvent("turn", players[0]) }
                        return;
                    }
                }
            }
        },
        reset(startPlayer = ready ? players[0] : null) {
            if (ready) {
                cells.forEach(cell => cell.value = "");
                API.player = startPlayer;
                resolved = false;
            }
        },    
        selectCell(element) {
            if (ready && ! resolved) {
                const cell = cells[element.dataset.num];
                if (cell){
                    cell.value = players[0];
                    if(cell.value === players[0]){
                        resolve()
                        API.player = players[1];
                    }
                }
            }
        },      
    };
    return API;
})();

Update

Answering some of your comments

point #1 I think function declarations -- when used with vars -- are discouraged (hoisting complexities). But I actually don't know if that's the case with let & const variables, and I am assuming from your response that it isn't. Isn't consting all the functions a good thing though ?

If you like using function expressions then you them.

point #2 So, I should use closured variables instead of _xyz-like variables ?

Closure is very powerful and you should use it as much and often as you can to gain experience. Closure is very secure, fast, and does not carry syntactical baggage

What are other applications of the underscore-preceding-variable style, if it is this bad, why is it common ?

Underscores are used in languages like C/C++ to ensure that variables and functions in libraries did not clash, however it is only a convention not part of the language. Most commonly it would be double underscore __Name

This has also been used in other languages for the same reason, more commonly just as a single underscore.

In Javascript it has been adopted as a sign that a variable is private, and is particularly common in code that uses the class token which does not allow for private properties.

It is bad because the language does not enforce the rule. _private is by trust not enforced in the language.

You can use it if you like but you must still treat it as a public property if you wish to trust your object's state. And if you go through the effort of ensuring the safety of the property then why bother with the underscore in the first place.

Should I add an underscore before each closured function that is not visible to the outside ? "and do not make good code" Could you expound on that ?

No you should not underscore. Underscore has no meaning, and thus makes for a bad variable name. It is just noise that makes the code harder to read.

point #7 Isn't this similar to point #2 ? And I think you are talking about the handlers, right ?

How you expose the properties of an object is up to you.

In the following the values num1, num2 are not critical to maintaining state. They can be any value and your code will run fine.

myObj = {
    num1 : 0,
    num2 : 0,
    test(){
        if(myObj.num1 !== myObj.num2){
            log("Not the same");
        }
    }
}

the next example the values of num1, and num2 can effect the state of you code.

var numberOfSucessfullTests = 0;
const myObj = {
    num1 : 0,
    num2 : 0,
    test(){
        myObj.num1 + myObj.num2;
        numberOfSucessfullTests += 1;
    },
    get testCount() { return numberOfSucessfullTests }
}
// which can break with
myObj.num1 = {toString(){throw "error"}};
myObj.num2 = 1;
myObj.test(); // will throw

...
log(myObj.testCount); // 0 state is corrupted 

You protext the state by vetting the exposed values

var numberOfSucessfullTests = 0;
var num1 = 0;
var num2 = 0;
const myObj = {
    get num1() { return num1 },
    set num1(val) { num1 = Number(val) },
    get num2() { return num2 },
    set num2(val) { num2 = Number(val) },        
    test(){
        myObj.num1 + myObj.num2;
        numberOfSucessfullTests += 1;
    },
    get testCount() { return numberOfSucessfullTests }
}
// which can break with
myObj.num1 = {toString(){throw "error"}};
myObj.num2 = 1;
myObj.test();
log(myObj.testCount); // 1 state is correct

point #8 What if I end up adding something to my code that needs to use that line again ? Won't I have to create a function for it then anyways ? Why shouldn't I do it now if I see that the functions could be re-used in the future, given that they are really intrinsic utilities (to my code) ?

Code review is for completed code, I can not second guess what you are to do with the code.

point #10 I don't understand. The errors I throw are related to the usage of the API, not my inner code, so I don't think that my release code has any errors, they occur only when the user of the API passes something unusable to the API (but I do admit that this is not in unison with other parts of the API, where I don't throw any errors even if the API is used incorrectly).

Throwing errors is in my book an action of last resort, something done to continue execution. If I throw, I will catch. Or you can use throws to help debug code, but that should be removed on release.

Rather than create a new TicTacToe(players, size) you init an existing instance via ticTacToe.init(players, size) and it returns the table. This is how I wanted it to be, but I could not think of anyway I could have this and the new TicTacToe(...) constructor at the same time, I wanted to use objects and prototypes.

Prototypes come with pros and cons.

• Pros. They use less memory per instance of an object, and are slightly faster to instantiate (Note this is browser dependent, on FF function instantiation is not improved by using a prototype).

• Cons. Prototypes are once step down the reference/inheritance tree and thus each time you use a prototyped property there is a slight CPU penalty as it first searches the object for the function, and if not found then it searches the prototype

Prototypes used to be good for objects that are frequently created or hand many instances. Now memory is cheap, GC is much better making the overhead of prototypes not worth and benefit you get instantiating, and using object pools you can avoid instantiation and memory overheads almost completely.

There is only one use for prototypes and that is prototypical inheritance.

I separated the winning-combination creating functions because I thought they would seem less complicated when broken into different functions ? Do you think one shouldn't just care about this micro-stuff ? Is the tradeoff between the initial complexity and the complexity/inefficiency added by creating multiple functions for each step just not worth it ?

The degree of granularity is the number of functions to source code. The more granular code is the more functions there are.

Low granularity (not many functions) is bad for many reasons (I am assuming that is self evident) so breaking down common tasks to functions is beneficial. But there is a point where increasing code granularity begins to effect source code quality and code execution efficiency.

In execution the optimizer spends a lot of time working out what functions are not needed, and in-lines them, this is because function calls require CPU and memory above that of what the function does. Function call have a cost, and you can not rely on the optimizer to inline them for you.

When reading code for the first time (or returning to code after some time) functions can disrupt the understanding of code because you must follow the function call to its source location. Lots of functions makes reading code harder (a spaghetti of function calls).

Finding the balance comes with experience. One liners used once is bad.