6
\$\begingroup\$

I have developed a tic-tac-toe game in native JavaScript. Can you tell me how to optimize more and what are the missing things? JSBIN

The logic, modular approach, testable and scalable?

(function() {
  var playerOneData = {
    name: null,
    arr: []
  };
  var playerTwoData = {
    name: null,
    arr: []
  };
  winnerFound = false;
  var clickCount = 2;

  var start = document.getElementById('start');
  var resultsList = [
    [0, 1, 2],
    [3, 4, 5],
    [6, 7, 8],
    [0, 3, 6],
    [1, 4, 7],
    [2, 5, 8],
    [0, 4, 8],
    [2, 4, 6],
  ];
  var setApplicationView = function(screenId, display) {
    //     hide user infor form
    document.getElementById(screenId).style.display = display;
  };
  start.addEventListener('click', function() {
    //     fetch username
    playerOneData.name = document.getElementById('player-one-name').value;
    playerTwoData.name = document.getElementById('player-two-name').value;
    //     hide user info form
    setApplicationView('user', 'none');
    //     show game board
    setApplicationView('game-board', 'block');
    //     set the player's name
    document.querySelector('#p1-info span').innerHTML = playerOneData.name;
    document.querySelector('#p2-info span').innerHTML = playerTwoData.name;
  });

  var ifClicked = function(index, tile) {
    var i = 0;
    while (i < tile.length) {
      if (index == tile[i])
        return false;
      i++;
    }
    return true
  };
  var selectedTiles = [];

  var winnerFinder = function(playersData, resultsList) {
    if(playersData.arr.length<2) return true;
    var player = playersData.arr;
    var i = 0;
    resultsList.forEach(function(element) {
      i = 0;
      player.forEach(function(ele) {
        if (element.indexOf(ele) >= 0) {
          i++;
        }
      });
      if (i == element.length) {
        matrix.removeEventListener('click', startMatrix);
        document.getElementById('result').innerHTML = "Winner is " + playersData.name;
        winnerFound = true;
        return false;
      }
    });
  };
  var matrix = document.getElementById('matrix');
  var startMatrix = function($event) {
    var target = $event.target;
    var tileIndex = +target.getAttribute('tile-index');
    var alreadyClickedStatus = ifClicked(tileIndex, selectedTiles);
    if (target.className === 'tile' && alreadyClickedStatus === true) {
      selectedTiles.push(tileIndex);


      if (clickCount % 2 === 0) {
        playerOneData.arr.push(tileIndex);
        target.className = 'tile red';

        if (winnerFinder(playerOneData, resultsList) === false) {
          matrix.removeEventListener('click', startMatrix);
        }
      } else {
        playerTwoData.arr.push(tileIndex);
        target.className = 'tile blue';
        if (winnerFinder(playerTwoData, resultsList) === false) {
          matrix.removeEventListener('click', startMatrix);
        }
      }

      clickCount++;
      if (clickCount === 11 && winnerFound === false) {
        document.getElementById('result').innerHTML = 'Match drawn';
      }
    }
  }
  matrix.addEventListener('click', startMatrix);
  var resetBoard = function() {
    winnerFound = false;
    clickCount = 2;
    matrix.addEventListener('click', startMatrix);
    document.getElementById('result').innerHTML = '';
    selectedTiles = [];
    var tile = document.querySelectorAll('.tile')
    tile.forEach(function(ele, index) {
      tile[index].className = 'tile';
    });
    playerOneData.arr = [];
    playerTwoData.arr = [];
  };
  var restart = document.getElementById('restart');
  restart.addEventListener('click', function() {

    resetBoard();
  });
  var newGame = document.getElementById('new-game');
  newGame.addEventListener('click', function() {
    //     show user info form
    setApplicationView('user', 'block');
    //     hide game board
    setApplicationView('game-board', 'none');
    resetBoard();
  });
})();
\$\endgroup\$
5
\$\begingroup\$

Note, some of these require a build process

  • the iife should be added in the build process, it's distracting
  • The code doesn't expose anything. It's untestable.
  • var should be let or const
  • You can use arrow functions to turn some of the functions into one liners
  • Inconsistent formatting - I recommend getting a linter
  • setApplicationView - Should probably be replaced with show/hide functions. The current name is not very descriptive to me.
  • // hide game board - You can make this a function and get rid of the comment. Several other similar issues.
  • ifClicked - A function name should generally describe what the functions does, not what happened when it was called. You could also just inline it.
  • winnerFinder - It would be so much nicer if this was called findWinner, and returned the result instead of storing it globally. Then you could test it as well. As it stands, it actually returns false when winnerFound is true. Even more confusing.
  • winnerFinder - You can use player.filter here to get rid of the index variable
  • playerData.arr - What is arr?
  • var player = playersData.arr; - How can a player be an array?
  • document.getElementById - You can run all these calls at the top to shorten the code and get rid of unnecessary dom queries.
  • innerHTML - It's quite hard to tell how this whole thing works with view logic and game logic all mixed in together. In general I would recommend separating the view logic from the game logic as much as possible, and even better would be to make the view logic declarative.
\$\endgroup\$
3
+25
\$\begingroup\$

Disclosure: I know that it has been ~6 days since Mangus's answer was submitted and you have already updated your code. However, I still have suggestions that will hopefully help.

Your Questions

Can you tell me how to optimize more and what are the missing things?

See the suggestions below for optimizing code. I am not really sure what is "missing"... that could be a very broad subject.

The logic, modular approach, testable and scalable?

I'm not sure what exactly to say about the logic. It does seem a bit complex - possibly more complicated than it needs to be. In that same vein, the code that adds and removes event handlers depending on the state of the game seems excessive. Could the click handler just be registered once and have it check the state of the game?

Obviously the code is testable using manual testing. If you want to use automated testing then that may be possible -perhaps with a framework like mocha/chai or something similar. I haven't worked with testing UI code much but you could write unit tests for the various functions.

Feedback and Suggestions

ES-6, constants

As Magnus's answer mentioned - features like const and let can be used to limit the scope of variables and avoid accidental re-assignment. And most idiomatic code in c-based languages have constants in all capitals. So you could do that for resultsList. Also, a more appropriate name would be something like WINNING_COMBINATIONS or something instead of RESULTS_LIST, because it is a list of combinations.


DOM references can be simplified

Let's look at the callback function for the click events for start - every time that function runs, there are DOM queries for four elements: #player-one-name, #player-two-name, #p1-infospan, #p2-info span. Those should all happen once when the DOM is ready and be stored in Javascript variables.

Scope of variable i in winnerFinder()

In winnerFinder() I see this:

var i = 0;
resultsList.forEach(function(element) {
  i = 0;
  player.forEach(function(ele) {
    if (element.indexOf(ele) >= 0) {
      i++;
    }
  });

Initially I am confused why i is set to 0 outside the foreach and then set to 0 in each iteration. One could just declare i inside the loop, since it doesn't appear to be used outside the inner function. After really thinking about this, i is a count of the elements that are contained with element... so Array.filter() could be used:

i = player.filter(function(ele) {
  return element.indexOf(ele) >= 0;
}).length;

This could also be simplified using arrow function notation:

i = player.filter( ele => element.indexOf(ele) >= 0 ).length;

And if element is an array, then Array.includes() could be used instead of indexOf().

Resetting class names for tiles

Inside resetBoard I see this:

tile.forEach(function(ele, index) {
  tile[index].className = 'tile';
});

Why not use ele instead of tile[index]?

It appears that you are removing any color class - so you could use classList.remove() and do something like:

ele.classList.remove('red', 'blue');

And classList.add() could be used to add the class name in the startMatrix function.

Registering the click handler on the restart button

Towards the end of the JS code I see this:

var restart = document.getElementById('restart');
restart.addEventListener('click', function() {
  resetBoard();
});
  1. When adding an event listener with an anonymous function that merely calls a single function, a function reference can be used instead:

    restart.addEventListener('click', resetBoard);
    
  2. restart is only used once so it is a waste of memory to assign it to a variable - use it without the variable:

    document.getElementById('restart').addEventListener('click', resetBoard);
    

    Though if you take my suggestion of caching DOM references when the DOM is ready, ideally you would use a reference like restartButton:

    restartButton.addEventListener('click', resetBoard);
    

click handler function: startMatrix()

The name of this function could have a more appropriate name, like matrixClickHandler() or something along those lines.

I see it accepts the event parameter, but as $event.

var startMatrix = function($event) {
  var target = $event.target;

It isn't wrong to name variables with a dollar sign in the beginning. jQuery code has a convention of doing this but many JavaScript developers believe that should be left to jQuery core code or plugins, or variables that come from library code like jQuery. This event object should just be MouseEvent, so there isn't really a need to prefix it with a dollar sign. See responses to Why would a JavaScript variable start with a dollar sign? for more context.

\$\endgroup\$

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.