Andy Fry.co

andyfry01 · 2022-01-24T21:03:36Z

By using div elements, this makes the game inaccessible for screenreader and keyboard users.

Fortunately, in order to remedy this, all you have to do is refactor these into button elements.

Buttons typically have a lot of browser-specific styling associated with them, which drives this tendency to use an unstyled div as a clean slate for designing a custom UI element. However, you can use a CSS button reset to mitigate this and get your buttons looking the way that you want them to. CSS Tricks has a great blog on the subject.

andyfry01 · 2022-01-24T21:03:36Z

Instead of a "flat" list of empty strings representing coordinates on the board, why not use some formatting to shape them to look like the board itself?

A couple of strategic newlines and tabs will really aid comprehension:

let gameState = ["", "", "", "", "", "", "", "", ""];

andyfry01 · 2022-01-24T21:03:36Z

Excellent practice to wrap things like this up into functions instead of hardcoded strings.

andyfry01 · 2022-02-01T21:49:10Z

Really like how you've implemented the winning conditions for this as a static set of triples representing a winning board. It's a very data-driven approach to checking for a win.

Checking for a winning condition could be implemented programmatically, and you could also write a function which generates this array of triples programmatically as well. If the domain we were working in was bigger, that might be a better choice, but seeing as we're working in a small context with this game, I think a hard-coded set of winning conditions is the perfect match for the task at hand 👍

andyfry01 · 2022-02-01T21:49:10Z

Firstly, some things I really like about this code:

It's wrapped up in a discrete function with a good name

It leverages early returns, and doesn't tangle its logic in a bunch of if/else trees, which can be hard to refactor down the line

It has nice whitespace to separate out the different parts, which helps us focus on the individual pieces of the function.

There are some things we can do which will make it even more readable:

Refactoring your for loop to a reduce function. for loops are good for looping tasks which require mutating state. In this context, we're checking the current board against the game's winning conditions to see whether to continue the game or whether a win or draw has occurred. This is a "data-driven" activity, and reduce is a tool which is much better suited for data-driven tasks.

Refactoring some boolean logic to use strings which represent game states.

Giving names to the logic checks you're performing, which will help us to more easily understand what exactly constitutes a win vs. a draw. When you've got variables to represent concepts, it makes things easier to read, seeing as you can rely on the variable name to communicate what's going on. Without a variable name, you have have to read the logic itself to understand what's going on. The logic in this context is not that hard to understand, but it nevertheless takes more time and effort to grok than a named variable would.

So, that being said, here's how I would refactor the code:

// At the top of the file, we can define some handy constants which represent game states: const IN_PROGRESS = "in progress" const WIN = "win" const DRAW = "draw" // the refactored function itself function handleResultValidation() { const gameStatus = winningConditions.reduce((status, winCondition) => { const a = gameState[winCondition[0]] const b = gameState[winCondition[1]] const c = gameState[winCondition[2]] // a "vector" means either a column or a row const incompleteVector = (a === '' && b === '' && c === '') const filledVector = (a === b && b === c) const boardFilledUp = !gameState.includes("") const gameFinished = filledVector && !incompleteVector if (status === WIN) { return WIN } if (boardFilledUp || status === DRAW) { return DRAW } return gameFinished ? WIN : IN_PROGRESS; }, IN_PROGRESS) return gameStatus === IN_PROGRESS ? handlePlayerChange() : endGame(gameStatus) }

andyfry01 · 2022-01-24T21:03:36Z

This is an example of a variable name that I'd consider too verbose.

We're inside a function called handleCellClick, which takes as an argument an object called clickedCellEvent, which we're grabbing a clickedCell value from. There's a ton of context already for what we're working with – a cell that's been clicked on – so clickedCellIndex, while admirable in that the name strives to clearly communicate the concept that the index belongs to, is a bit excessive.

You could argue that index would be enough to communicate what's going on here, or maybe cellIndex is a happy medium between too verbose and not verbose enough.

andyfry01 · 2022-02-01T21:49:10Z

Things like query selectors for DOM elements fall into a category I refer to as "setup" or "bootstrapping" code. It's often helpful to place query selectors in their own section for organization, and to reference them by variable name instead of querying them directly inside a function. This aids in organization and separation of concerns between the setup for your app and the logic code which actually runs the functionality.

// Somewhere close to the top of the file, with the rest of your queried DOM elements: const cells = document.querySelectorAll('.cell') // what you'd invoke on this line: cells.forEach(cell => cell.innerHTML = "");

@@ -0,0 +1,31 @@
+      <!doctype html>
+      <html lang="en">
+      <head>
+          <meta charset="UTF-8">
+          <meta name="viewport"
+                content="width=device-width, user-scalable=no, initial-scale=1.0, maximum-scale=1.0, minimum-scale=1.0">
+          <meta http-equiv="X-UA-Compatible" content="ie=edge">
+          <title>Tic Tac Toe</title>
+          <link rel="stylesheet" href="style.css">
+      </head>
+      <body>
+          <section>
+              <h1 class="game--title">Tic Tac Toe</h1>
+              <div class="game--container">
+                  <div data-cell-index="0" class="cell"></div>
+                  <div data-cell-index="1" class="cell"></div>
+                  <div data-cell-index="2" class="cell"></div>
+                  <div data-cell-index="3" class="cell"></div>
+                  <div data-cell-index="4" class="cell"></div>
+                  <div data-cell-index="5" class="cell"></div>
+                  <div data-cell-index="6" class="cell"></div>
+                  <div data-cell-index="7" class="cell"></div>
+                  <div data-cell-index="8" class="cell"></div>
+              </div>
+              <h2 class="game--status"></h2>
+              <button class="game--restart">Restart Game</button>
+          </section>
+          <script src="script.js"></script>
+      </body>
+      </html>

@@ -0,0 +1,87 @@
+      const statusDisplay = document.querySelector('.game--status');
+      let gameActive = true;
+      let currentPlayer = "X";
+      let gameState = ["", "", "", "", "", "", "", "", ""];
+      const winningMessage = () => `Player ${currentPlayer} has won!`;
+      const drawMessage = () => `Game ended in a draw!`;
+      const currentPlayerTurn = () => `It's ${currentPlayer}'s turn`;
+      statusDisplay.innerHTML = currentPlayerTurn();
+      const winningConditions = [
+          [0, 1, 2],
+          [3, 4, 5],
+          [6, 7, 8],
+          [0, 3, 6],
+          [1, 4, 7],
+          [2, 5, 8],
+          [0, 4, 8],
+          [2, 4, 6]
+      ];
+      function handleCellPlayed(clickedCell, clickedCellIndex) {
+          gameState[clickedCellIndex] = currentPlayer;
+          clickedCell.innerHTML = currentPlayer;
+      }
+      function handlePlayerChange() {
+          currentPlayer = currentPlayer === "X" ? "O" : "X";
+          statusDisplay.innerHTML = currentPlayerTurn();
+      }
+      function handleResultValidation() {
+          let roundWon = false;
+          for (let i = 0; i <= 7; i++) {
+              const winCondition = winningConditions[i];
+              let a = gameState[winCondition[0]];
+              let b = gameState[winCondition[1]];
+              let c = gameState[winCondition[2]];
+              if (a === '' || b === '' || c === '') {
+                  continue;
+              }
+              if (a === b && b === c) {
+                  roundWon = true;
+                  break
+              }
+          }
+          if (roundWon) {
+              statusDisplay.innerHTML = winningMessage();
+              gameActive = false;
+              return;
+          }
+          let roundDraw = !gameState.includes("");
+          if (roundDraw) {
+              statusDisplay.innerHTML = drawMessage();
+              gameActive = false;
+              return;
+          }
+          handlePlayerChange();
+      }
+      function handleCellClick(clickedCellEvent) {
+          const clickedCell = clickedCellEvent.target;
+          const clickedCellIndex = parseInt(clickedCell.getAttribute('data-cell-index'));
+          if (gameState[clickedCellIndex] !== "" || !gameActive) {
+              return;
+          }
+          handleCellPlayed(clickedCell, clickedCellIndex);
+          handleResultValidation();
+      }
+      function handleRestartGame() {
+          gameActive = true;
+          currentPlayer = "X";
+          gameState = ["", "", "", "", "", "", "", "", ""];
+          statusDisplay.innerHTML = currentPlayerTurn();
+          document.querySelectorAll('.cell').forEach(cell => cell.innerHTML = "");
+      }
+      document.querySelectorAll('.cell').forEach(cell => cell.addEventListener('click', handleCellClick));
+      document.querySelector('.game--restart').addEventListener('click', handleRestartGame);

Andy Fry.co

Code Review How To: Clarity and Simplicity

Today’s topic

Broad themes

Break large functions into smaller functions

Getting to the point with variable names

Using language tools to add intent and expressivity

Original code and example

Review

Refactored code and example

Conclusion

Leave a comment

Related Posts