9
\$\begingroup\$

I am teaching myself JavaScript and made a little Tic-Tac-Toe web app. I'm not asking anyone to read through it all, but to just tell me what I need or should not do in the future.

var boxes = document.querySelectorAll("td");
var boxtext = document.querySelectorAll("td span");
var player = document.querySelector("#player");
var playerTurnDisplay = document.querySelector("#playerTurnDisplay");
var resetButton = document.querySelector("#resetButton");
var isXTurn = true;
var boxesLeftToClick = 9;
var gameOver = false;

function init() {
  addClickListeners(boxes);
}

function addClickListeners(arr) {
    for (var i = 0; i < arr.length; i++) {
      arr[i].addEventListener("click", function() {
        if (isEmpty(this) && gameOver === false) {
          addObjectToBox(this);
          isXTurn = !(isXTurn);
          boxesLeftToClick--;
          displayTurn();
          checkWinner(arr);
        }
      });
      resetButton.addEventListener("click", function() {
        reset();
      })
    }
  }
  // is the box empty [true] or does it have an X or O in it [false]

function isEmpty(box) {
    if (box.textContent !== "X" && box.textContent !== "O")
      return true;
    else
      return false;
  }
  // adds the Object "O" to the board

function addCross(box) {
    box.classList.add("cross");
    box.textContent = "X";
  }
  // adds the Object "X" to the board

function addCircle(box) {
    box.classList.add("circle");
    box.textContent = "O";
  }
  // displays whos turn it is to the players

function displayTurn() {
    if (boxesLeftToClick > 0) {
      player.textContent = (isXTurn) ? "X" : "O";
      playerTurnDisplay.classList.toggle("xTurn");
      playerTurnDisplay.classList.toggle("yTurn");
    } else {
      playerTurnDisplay.textContent = "Draw";
      playerTurnDisplay.classList.toggle(playerTurnDisplay.classList);
      playerTurnDisplay.classList.add("gameOver");
    }
  }
  // if someone wins get rid of the background color and display the text of whoever won

function setWinnerDisplay(box, orientation) {
    playerTurnDisplay.classList.remove("xTurn");
    playerTurnDisplay.classList.remove("yTurn");
    playerTurnDisplay.classList.add("gameOver");
    playerTurnDisplay.textContent = box.textContent + " wins " + orientation;
    gameOver = true;
  }
  // adds the "line" through the winning boxes

function crossOut(box1, box2, box3) {
    box1.classList.add("crossOut");
    box2.classList.add("crossOut");
    box3.classList.add("crossOut");
  }
  // all the things that need to happend when a player wins will happen if you call this function

function win(box1, box2, box3, orientation) {
    setWinnerDisplay(box1, orientation);
    crossOut(box1, box2, box3);
  }
  // returns true if the boxes are all X's or O's false if they are empty or not the same

function isTheSame(box1, box2, box3) {
  if (box1.textContent === box2.textContent && box1.textContent === box3.textContent && isEmpty(box1) === false) return true;
  else
    return false;
}

function reset() {
    for (var i = 0; i < boxes.length; i++) {
      boxes[i].textContent = "";
      isXTurn = true;
      boxes[i].classList.remove("crossOut");
      boxes[i].classList.remove("gameOver");
    }
    gameOver = false;
    if (isXTurn) playerTurnDisplay.classList.add("xTurn");
    boxesLeftToClick = 9;
  }
  // checks an array of 9 elements to see if their textContent is the same

function checkWinner(arr) {
  if (isTheSame(arr[0], arr[1], arr[2]))
    win(arr[0], arr[1], arr[2], "horizontally");
  else if (isTheSame(arr[3], arr[4], arr[5]))
    win(arr[3], arr[4], arr[5], "horizontally");
  else if (isTheSame(arr[6], arr[7], arr[8]))
    win(arr[6], arr[7], arr[8], "horizontally");
  else if (isTheSame(arr[0], arr[3], arr[6]))
    win(arr[0], arr[3], arr[6], "vertically");
  else if (isTheSame(arr[1], arr[4], arr[7]))
    win(arr[1], arr[4], arr[7], "vertically");
  else if (isTheSame(arr[2], arr[5], arr[8]))
    win(arr[2], arr[5], arr[8], "vertically");
  else if (isTheSame(arr[0], arr[4], arr[8]))
    win(arr[0], arr[4], arr[8], "diagonally");
  else if (isTheSame(arr[2], arr[4], arr[6]))
    win(arr[2], arr[4], arr[6], "diagonally");
}

function addObjectToBox(box) {
  if (isXTurn)
    addCross(box);
  else
    addCircle(box);
}

init();
h1 {
  text-align: center;
  margin: 0;
}
.gameOver {
  background: #14FF00;
  border: 3px solid #14FF00;
}
#nav-bar {
  margin: 0 auto;
  width: 430px;
}
button {
  margin: 0px auto 3px auto;
  display: inline-block;
  width: 211px;
  border: 2px dotted grey;
  font-size: 20px;
  text-transform: uppercase;
  font-weight: 500;
  background: gold;
}
#playerTurnDisplay {
  display: inline-block;
  text-align: center;
  width: 205px;
  margin: 0px auto 20px auto;
}
.xTurn {
  background: red;
  border: 3px solid red;
}
.yTurn {
  background: blue;
  border: 3px solid blue;
}
table {
  margin: auto;
}
td {
  width: 205px;
  height: 205px;
  text-align: center;
}
.vertical {
  border-left: 1px solid black;
  border-right: 1px solid black;
}
.horizontal {
  border-top: 1px solid black;
  border-bottom: 1px solid black;
}
.circle {
  font-size: 150px;
  color: blue;
  transition: all 0.1s;
  --webkit-transition: all 0.1s;
  --moz-transition: all 0.1s;
}
.cross {
  font-size: 150px;
  color: red;
  transition: all 0.1s;
  --webkit-transition: all 0.1s;
  --moz-transition: all 0.1s;
}
.crossOut {
  background: gold;
  color: black;
  transition: all 0.4s;
  --webkit-transition: all 0.4s;
  --moz-transition: all 0.4s;
}
<!DOCTYPE html>
<html>

<head>
  <meta charset="utf-8">
  <title>TicTacToe</title>

  <link rel="stylesheet" href="css/master.css" />
</head>

<body>
  <h1>Tic Tac Toe</h1>
  <div id="nav-bar">
    <button id="resetButton">reset</button>
    <h3 class="xTurn" id="playerTurnDisplay"><span id="player">X</span>'s turn</h3>
  </div>
  <table>
    <tbody>
      <tr>
        <td><span id="top-left"></span>
        </td>
        <td class="vertical"><span id="top-mid"></span>
        </td>
        <td><span id="top-right"></span>
        </td>
      </tr>
      <tr>
        <td class="horizontal"><span id="mid-left"></span>
        </td>
        <td class="vertical horizontal"><span id="mid-mid"></span>
        </td>
        <td class="horizontal"><span id="mid-right"></span>
        </td>
      </tr>
      <tr>
        <td><span id="bottom-left"></span>
        </td>
        <td class="vertical"><span id="bottom-mid"></span>
        </td>
        <td><span id="bottom-right"></span>
        </td>
      </tr>
    </tbody>
  </table>

  <script src="js/master.js" charset="utf-8"></script>
</body>

</html>

\$\endgroup\$
1

2 Answers 2

3
\$\begingroup\$

Suggestions:

  1. Use Event Delegation to handle events (e.g. click) by inspecting the target of the event (e.g. with event.target) to determine the action, instead of adding event handlers to each element (e.g. table cells, the reset button, etc.). That way if the elements ever get added/removed from the DOM, the event handlers won't be missing/lost, which could lead to memory leaks (which could eventually crash the browser).

    So the addClickListeners() function can be written like this:

    document.addEventListener('click', function(clickEvent) {
      if (clickEvent.target.tagName.toUpperCase() == 'TD') {
         cellClickHandler(clickEvent.target);
      } else if (clickEvent.target.id == 'resetButton') {
         reset();
      }
    });
    
  2. Use a functional programming approach to iterate through the cells - e.g. with Array.prototype.forEach() instead of a for statement. To learn more about functional programming in JS, try going through these exercises. One of the major benefits is that you don't have to manually increment the iterator variable and then use that to index into the array - the item at each index is passed as a parameter to the callback function. Of course there are drawbacks to this approach, like the scope changes slightly.

    So the reset() function could be rewritten like this:

    function reset() {
      boxes.forEach(function(box) {
        box.textContent = "";
        box.classList.remove("crossOut");
        box.classList.remove("gameOver");
      });
      ...
    }
    
  3. The logic for some functions like isTheSame and isEmpty can be simplified because the expressions for the if statements are booleans, and the functions themselves are booleans, so the expressions can be returned. For example:

    function isTheSame(box1, box2, box3) {
      if (box1.textContent === box2.textContent && box1.textContent === box3.textContent && isEmpty(box1) === false) return true;
      else
        return false;
    }
    

    can be re-written as:

    function isTheSame(box1, box2, box3) {
      return box1.textContent === box2.textContent && box1.textContent === box3.textContent && isEmpty(box1) === false;
    }
    
  4. Use document.getElementById() to select elements by Id attribute instead of using document.querySelector() and passing a hash character followed by the id. When I ran this performance test, the latter was 62% slower. It likely doesn't make much of a difference for a small DOM structure like this but on a larger project it could. A different approach to selecting the table cells than using document.querySelector() would be to select the table element and query under it for the table cells.

    var player = document.getElementById("player");
    var playerTurnDisplay = document.getElementById("playerTurnDisplay");
    var resetButton = document.getElementById("resetButton");</pre>
    

See these changes in action below:

var boxes = document.querySelectorAll("td");
var boxtext = document.querySelectorAll("td span");
var player = document.getElementById("player");
var playerTurnDisplay = document.getElementById("playerTurnDisplay");
var resetButton = document.getElementById("resetButton");
var isXTurn = true;
var boxesLeftToClick = 9;
var gameOver = false;
document.addEventListener('DOMContentLoaded', function() {
  document.addEventListener('click', function(clickEvent) {
    if (clickEvent.target.tagName.toUpperCase() == 'TD') {
      cellClickHandler(clickEvent.target);
    } else if (clickEvent.target.id == 'resetButton') {
      reset();
    }
  });
});

function cellClickHandler(target) {
  if (isEmpty(target) && gameOver === false) {
    addObjectToBox(target);
    isXTurn = !(isXTurn);
    boxesLeftToClick--;
    displayTurn();
    checkWinner(boxes);
  }
}
  // is the box empty [true] or does it have an X or O in it [false]

function isEmpty(box) {
    return box.textContent !== "X" && box.textContent !== "O";
  }
  // adds the Object "O" to the board

function addCross(box) {
    box.classList.add("cross");
    box.textContent = "X";
  }
  // adds the Object "X" to the board

function addCircle(box) {
    box.classList.add("circle");
    box.textContent = "O";
  }
  // displays whos turn it is to the players

function displayTurn() {
    if (boxesLeftToClick > 0) {
      player.textContent = (isXTurn) ? "X" : "O";
      playerTurnDisplay.classList.toggle("xTurn");
      playerTurnDisplay.classList.toggle("yTurn");
    } else {
      playerTurnDisplay.textContent = "Draw";
      playerTurnDisplay.classList.toggle(playerTurnDisplay.classList);
      playerTurnDisplay.classList.add("gameOver");
    }
  }
  // if someone wins get rid of the background color and display the text of whoever won

function setWinnerDisplay(box, orientation) {
    playerTurnDisplay.classList.remove("xTurn");
    playerTurnDisplay.classList.remove("yTurn");
    playerTurnDisplay.classList.add("gameOver");
    playerTurnDisplay.textContent = box.textContent + " wins " + orientation;
    gameOver = true;
  }
  // adds the "line" through the winning boxes

function crossOut(box1, box2, box3) {
    box1.classList.add("crossOut");
    box2.classList.add("crossOut");
    box3.classList.add("crossOut");
  }
  // all the things that need to happend when a player wins will happen if you call this function

function win(box1, box2, box3, orientation) {
    setWinnerDisplay(box1, orientation);
    crossOut(box1, box2, box3);
  }
  // returns true if the boxes are all X's or O's false if they are empty or not the same

function isTheSame(box1, box2, box3) {
  return box1.textContent === box2.textContent && box1.textContent === box3.textContent && isEmpty(box1) === false;
}

function reset() {
  boxes.forEach(function(box) {
      box.textContent = "";
      box.classList.remove("crossOut");
      box.classList.remove("gameOver");
    });
    isXTurn = true;
    gameOver = false;
    playerTurnDisplay.classList.add("xTurn");
    boxesLeftToClick = 9;
  }
  // checks an array of 9 elements to see if their textContent is the same

function checkWinner(arr) {
  if (isTheSame(arr[0], arr[1], arr[2]))
    win(arr[0], arr[1], arr[2], "horizontally");
  else if (isTheSame(arr[3], arr[4], arr[5]))
    win(arr[3], arr[4], arr[5], "horizontally");
  else if (isTheSame(arr[6], arr[7], arr[8]))
    win(arr[6], arr[7], arr[8], "horizontally");
  else if (isTheSame(arr[0], arr[3], arr[6]))
    win(arr[0], arr[3], arr[6], "vertically");
  else if (isTheSame(arr[1], arr[4], arr[7]))
    win(arr[1], arr[4], arr[7], "vertically");
  else if (isTheSame(arr[2], arr[5], arr[8]))
    win(arr[2], arr[5], arr[8], "vertically");
  else if (isTheSame(arr[0], arr[4], arr[8]))
    win(arr[0], arr[4], arr[8], "diagonally");
  else if (isTheSame(arr[2], arr[4], arr[6]))
    win(arr[2], arr[4], arr[6], "diagonally");
}

function addObjectToBox(box) {
  if (isXTurn)
    addCross(box);
  else
    addCircle(box);
}
h1 {
  text-align: center;
  margin: 0;
}
.gameOver {
  background: #14FF00;
  border: 3px solid #14FF00;
}
#nav-bar {
  margin: 0 auto;
  width: 430px;
}
button {
  margin: 0px auto 3px auto;
  display: inline-block;
  width: 211px;
  border: 2px dotted grey;
  font-size: 20px;
  text-transform: uppercase;
  font-weight: 500;
  background: gold;
}
#playerTurnDisplay {
  display: inline-block;
  text-align: center;
  width: 205px;
  margin: 0px auto 20px auto;
}
.xTurn {
  background: red;
  border: 3px solid red;
}
.yTurn {
  background: blue;
  border: 3px solid blue;
}
table {
  margin: auto;
}
td {
  width: 205px;
  height: 205px;
  text-align: center;
}
.vertical {
  border-left: 1px solid black;
  border-right: 1px solid black;
}
.horizontal {
  border-top: 1px solid black;
  border-bottom: 1px solid black;
}
.circle {
  font-size: 150px;
  color: blue;
  transition: all 0.1s;
  --webkit-transition: all 0.1s;
  --moz-transition: all 0.1s;
}
.cross {
  font-size: 150px;
  color: red;
  transition: all 0.1s;
  --webkit-transition: all 0.1s;
  --moz-transition: all 0.1s;
}
.crossOut {
  background: gold;
  color: black;
  transition: all 0.4s;
  --webkit-transition: all 0.4s;
  --moz-transition: all 0.4s;
}
 <h1>Tic Tac Toe</h1>
<div id="nav-bar">
  <button id="resetButton">reset</button>
  <h3 class="xTurn" id="playerTurnDisplay"><span id="player">X</span>'s turn</h3>
</div>
<table>
  <tbody>
    <tr>
      <td><span id="top-left"></span>
      </td>
      <td class="vertical"><span id="top-mid"></span>
      </td>
      <td><span id="top-right"></span>
      </td>
    </tr>
    <tr>
      <td class="horizontal"><span id="mid-left"></span>
      </td>
      <td class="vertical horizontal"><span id="mid-mid"></span>
      </td>
      <td class="horizontal"><span id="mid-right"></span>
      </td>
    </tr>
    <tr>
      <td><span id="bottom-left"></span>
      </td>
      <td class="vertical"><span id="bottom-mid"></span>
      </td>
      <td><span id="bottom-right"></span>
      </td>
    </tr>
  </tbody>
</table>

\$\endgroup\$
3
\$\begingroup\$

1.

Consider using ES2015. Most of its features are there to make your life easier. Modern browsers support > 95% of it and for the rest there's transpilers/polyfills (babel).

2.

function isEmpty(box) {
  if (box.textContent !== "X" && box.textContent !== "O")
    return true;
  else
    return false;
}

should be just

function isEmpty(box) {
  return box.textContent !== "X" && box.textContent !== "O"
}

This is general advice that is valid for other programming language. You can do the same for the isTheSame function.

I also feel like the condition should be box.textContent === ''; this is more in line with the isEmpty name.

3.

The checkWinner can be rewritten in a less repetitive fashion:

const winningCombination = [
  { triple: [0, 1, 2], name: 'horizontally' },
  { triple: [3, 4, 5], name: 'horizontally' },
  // etc
].find(({ triple }) => isTheSame(...triple))

if (winningCombination) {
  const { triple, name } = winningCombination
  win(...triple, name)
}

Why is an array better than that long if-else chain? Because you can potentially store these combinations in a separate file or even generate them. If you decide to have 4x4 tic-tac-toe you're going to get tired of typing them out explicitly.

It's generally a good idea to store data-heavy/table-looking code as data.

4.

Consider using forEach/for ... of instead of plain for loops. In both of your cases you don't care how you're iterating arr/boxes. Let forEach/for ... of concern themselves with HOW the iteration is done.

\$\endgroup\$

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.