Tic Tac Toe in object oriented JavaScript

Question

I've been working on a course assignment for 'The Odin Project' and was hoping to get a review of my code.

As per the help guide, the code is fully functional.

The criteria for this was pretty straight forward. Try to have as little global code as possible. (At this point I've learned about classes, and modules)

I feel that I'm breaking some rules when it comes to clean code.

Live example:
https://ablueblaze.github.io/TicTacToe/

Repository:
https://github.com/ablueblaze/TicTacToe

JavaScript:

document.addEventListener("DOMContentLoaded", () => {

  // Event listener for all the cells
  document.querySelectorAll(".cell").forEach((cell) =>
    cell.addEventListener("click", (e) => {
      gameControls.makePlay(e, player1, player2, testBoard);
    })
  );

  // Event listener for when the modal is active
  document
    .querySelector("[data-modal]")
    .addEventListener("click", displayControls.toggleModal);

  // Event listener for the new game button
  document.getElementById("new-game").addEventListener("click", () => {
    gameControls.newGameBtn(testBoard);
  });

  // Event listener for the clear scores button
  document.querySelector("#clear-scores").addEventListener("click", () => {
    gameControls.clearAllScoresBtn(player1, player2);
  });
});

class GameBoard {
  constructor() {
    this.controlBoard = [1, 2, 3, 4, 5, 6, 7, 8, 9];
    this.displayBoard = ["", "", "", "", "", "", "", "", ""];
  }
  isFull() {
    let count = 0;
    for (let i = 1; i < 10; i++) {
      if (this.controlBoard.includes(i)) {
        count++;
      }
    }
    if (count === 0) {
      return true;
    }
  }
  markBoard(play, playerMark) {
    if (this.controlBoard.includes(play)) {
      let index = this.controlBoard.findIndex((e) => e === play);
      this.controlBoard[index] = 0;
      this.displayBoard[index] = playerMark;
      return true;
    }
  }
  boardReset() {
    this.controlBoard = [1, 2, 3, 4, 5, 6, 7, 8, 9];
    this.displayBoard = ["", "", "", "", "", "", "", "", ""];
  }
}

// A player class that will hold the players marker, and all the plays that they have made
class Player {
  constructor(marker, plays = [], score = 0) {
    this.marker = marker;
    this.plays = plays;
    this.score = score;
  }
  addPlay(play) {
    this.plays.push(play);
  }
  clearPlays() {
    this.plays = [];
  }
  clearScore() {
    this.score = 0;
  }
  scoreUp() {
    this.score++;
  }
  // returns true if player has a winning hand
  didIWin() {
    const winingHands = [
      [1, 2, 3],
      [4, 5, 6],
      [7, 8, 9],
      [1, 4, 7],
      [2, 5, 8],
      [3, 6, 9],
      [1, 5, 9],
      [3, 5, 7],
    ];
    let count = 0;
    for (let i of winingHands) {
      for (let n of i) {
        if (this.plays.includes(n)) {
          count++;
        }
      }
      if (count === 3) {
        return true;
      }
      count = 0;
    }
    return false;
  }
}

const testBoard = new GameBoard();
let currentBoard = testBoard.displayBoard;
const player1 = new Player("X");
const player2 = new Player("O");

const displayControls = (() => {
  // Updates the pages game board
  function boardUpdate(displayBoard) {
    for (let i = 0; i < displayBoard.length; i++) {
      let activeCell = document.querySelector(`[data-cell-value="${i + 1}"]`);
      activeCell.textContent = displayBoard[i];
    }
  }

  // Updates the pages score board
  function scoreUpdate(player1Score, player2Score) {
    const pXScore = document.querySelector("#player-one-score");
    const pOScore = document.querySelector("#player-two-score");
    pXScore.textContent = player1Score;
    pOScore.textContent = player2Score;
  }

  // Update the current player banner, by flipping the return from current player
  function showCurrentPlayer(currentPLayerMarker) {
    const playerSpan = document.querySelector("#current-player");
    if (currentPLayerMarker === "X") {
      playerSpan.textContent = "O";
      return;
    }
    playerSpan.textContent = "X";
  }

  // Set the Winner text in modal
  function setWinner(winningPlayerMarker) {
    const winner = document.querySelector(".winner");
    winner.textContent = `Winner: ${winningPlayerMarker}`;
  }

  // Set the Winner text in modal to Tie
  function showTie() {
    const winner = document.querySelector(".winner");
    winner.textContent = "It's a Draw!";
  }

  function toggleModal() {
    const modal = document.querySelector("[data-modal]");
    if (modal.className === "modal active") {
      modal.classList.remove("active");
      return;
    }
    modal.classList.add("active");
  }

  return {
    boardUpdate,
    scoreUpdate,
    showCurrentPlayer,
    setWinner,
    showTie,
    toggleModal,
  };
})();

const gameControls = (() => {
  // Toggle between the given players
  let playCounter = 1;
  function togglePlayer(player1, player2) {
    playCounter++;
    if (playCounter % 2 == 0) {
      return player1;
    } else {
      return player2;
    }
  }

  function resetCounter() {
    playCounter = 1;
  }

  function newGameBtn(activeBoard) {
    resetCounter();
    activeBoard.boardReset();
    displayControls.boardUpdate(activeBoard.displayBoard);
    displayControls.showCurrentPlayer("O");
  }

  function clearAllScoresBtn(player1, player2) {
    player1.clearScore();
    player2.clearScore();
    displayControls.scoreUpdate(player1.score, player2.score);
  }

  function clearPlays(player1, player2, activeBoard) {
    player1.clearPlays();
    player2.clearPlays();
    activeBoard.boardReset();
  }

  // Runs after a game is finished
  function nextGame(player1, player2, activeBoard) {
    clearPlays(player1, player2, activeBoard);
    displayControls.boardUpdate(activeBoard.displayBoard);
    displayControls.toggleModal();
  }

  // Check to see if a game is done
  function endGame(currentPlayer, player1, player2, activeBoard) {
    if (currentPlayer.didIWin()) {
      currentPlayer.scoreUp();
      displayControls.setWinner(currentPlayer.marker);
      displayControls.scoreUpdate(player1.score, player2.score);
      nextGame(player1, player2, activeBoard);
    } else if (activeBoard.isFull()) {
      displayControls.showTie();
      clearPlays(player1, player2, activeBoard);
      nextGame(player1, player2, activeBoard);
    }
  }

  function makePlay(event, player1, player2, activeBoard) {
    let currentPlayer = togglePlayer(player1, player2);
    let cellValue = parseFloat(event.target.dataset.cellValue);
    activeBoard.markBoard(cellValue, currentPlayer.marker);
    currentPlayer.addPlay(cellValue);
    displayControls.boardUpdate(activeBoard.displayBoard);
    displayControls.showCurrentPlayer(currentPlayer.marker);
    endGame(currentPlayer, player1, player2, activeBoard);
  }

  return { makePlay, clearPlays, newGameBtn, clearAllScoresBtn };
})();

Thanks A bunch!

Answer 1

Welcome to Code Review, your game looks pretty nice!

There is quite a lot of code to go through but here are some initial observations.

Constructor

You are using exactly the same code in the GameBoard constructor and in boardReset. You should rather simply call boardReset from the constructor:

constructor() {
    this.boardReset();
}

isFull

Note that includes does not work in all browsers, not that anybody should be using IE9 anymore =).

This can be simplified by checking if there are any empty spaces left:

isFull() {
    return this.displayBoard.indexOf("") == -1;
}

markBoard

Here you first use contains to check if the item is in the list and then indexOf to get the index. You could have simplified this like so:

markBoard(play, playerMark) {
    let index = this.controlBoard.findIndex((e) => e === play);
    if (index != -1) {
        this.controlBoard[index] = 0;
        this.displayBoard[index] = playerMark;
        return true;
    }
}

Answer 2

The hardest thing about programming is creating a good project structure, but this is also the most important thing to get correct. Your project is structured well, and cleanly follows the model-view-controller structure, so great job! Your view logic is cleanly separated from the underlying model, and your controller is requesting updates on both the model and the view. They're not trying to do each other's jobs, or cross each other's boundaries.

However, as we zoom into some smaller details, there are a number of things that could certainly be tidied up a bit.

DOMContentLoaded

There's really no need to use .forEach() anymore. for-of can do everything forEach does, and more (e.g. for-of lets you use continue/break within the loop).

You're using both getElementById() and querySelector() to select elements by ID. Pick one and stick with it.

GameBoard

@jdt already gave some good suggestions on how to improve this class. I'll add a couple more things.

isFull()'s logic is a little odd. It looks like you're just trying to check if controlBoard() contains a number between 1 and 9. If so, you return true, if not you return undefined (why not return false instead of undefined?). However, the only other digit this array is able to contain is 0. I think the following code snippet does a better job at capturing the idea we're trying to accomplish:

isFull() {
  for (let x of this.controlBoard) {
    if (x !== 0) return false;
  }
  return true;
}

Or even better, we can use a higher-order function, array.every(). (In general, it may be worthwhile to familiarize yourself with the numerous array methods available, so you can utilize them in times of need).

isFull() {
  return this.controlBoard.every((x) => x === 0);
}

Instead of this:

this.controlBoard.findIndex((e) => e === play);

you can do this:

this.controlBoard.indexOf(play);

Player

In didIWin(), you did for (let i of winingHands) { ... }, generally the "i" loop variable is only used to hold an array index. In this scenario, "i" is holding an array. It would be better to name it something else - anything else, even "x" would be better.

Also, instead of resetting your "count" variable at the end of each iteration, why not move the initial declaration into the loop.

didIWin() {
  const winingHands = [
    [1, 2, 3],
    [4, 5, 6],
    [7, 8, 9],
    [1, 4, 7],
    [2, 5, 8],
    [3, 6, 9],
    [1, 5, 9],
    [3, 5, 7],
  ];
  for (let winningHand of winingHands) {
    let count = 0;
    for (let n of winningHand) {
      if (this.plays.includes(n)) {
        count++;
      }
    }
    if (count === 3) {
      return true;
    }
  }
  return false;
}

We can continue to clean this up a little more by using the higher-order function that we learned about earlier, array.every(), along with its companion array.some().

didIWin() {
  const winingHands = [
    [1, 2, 3],
    [4, 5, 6],
    [7, 8, 9],
    [1, 4, 7],
    [2, 5, 8],
    [3, 6, 9],
    [1, 5, 9],
    [3, 5, 7],
  ];
  const isHandInPlay = hand => hand.every(n => this.plays.includes(n));
  return winingHands.some(isHandInPlay);
}

gameControls

Don't use loose equality (==), use strict equality instead (===) (you correctly use strict equality everywhere else, so I assume this was simply a slip-up).

// before
if (playCounter % 2 == 0) { ... }
// after
if (playCounter % 2 === 0) { ... }

misc

Don't write comments that are trivially inferred by reading the code. For example:

// Updates the pages game board
function boardUpdate(displayBoard) { ... }

There's nothing wrong with adding some comments to help explain the code, but try to reserve it for stuff that isn't self-explanatory.

On naming functions: Prefer "action" verb-like names. i.e. "scoreUpdate" doesn't sound right. The function isn't a single "score update", rather, it's a function that "updates a score". So, a name like updateScore() may be more appropriate, Likewise, resetBoard() makes more sense than boardReset(). Above all, make sure your variable names don't lie, for example, the name endGame() is a lie. This function does not simply end the game, what it really does is check if the game is in an end-condition, and if so, it'll end the game. The difference is important - right now, if you read the definition of makePlay(), it sounds like at the end of each play the game ends, which is simply not true!

function makePlay(event, player1, player2, activeBoard) {
  let currentPlayer = togglePlayer(player1, player2);
  let cellValue = parseFloat(event.target.dataset.cellValue);
  activeBoard.markBoard(cellValue, currentPlayer.marker);
  displayControls.updateBoard(activeBoard.displayBoard);
  displayControls.showCurrentPlayer(currentPlayer.marker);
  endGame(currentPlayer, player1, player2, activeBoard); // The game does not end here!
}

Renaming it to something like endGameIfAble() makes what's going on so much clearer.

GameBoard rewrite

Prefer keeping the amount of state in your application down to a minimum. For example, your GameBoard class is unnecessarily trying to keep two chunks of state in sync - the controlBoard array and the displayBoard array. If you think about it, the controlBoard array could be derived from the displayBoard array at any moment, by passing it through a function such as this:

#getControlBoard() {
  return this.displayBoard.map((square, i) => square === "" ? i + 1 : 0)
}

This would allow us to refactor the class as follows:

class GameBoard {
  constructor() {
    this.resetBoard();
  }
  #getControlBoard() {
    return this.displayBoard.map((square, i) => square === "" ? i + 1 : 0);
  }
  isFull() {
    return this.#getControlBoard().every((x) => x === 0);
  }
  markBoard(play, playerMark) {
    let index = this.#getControlBoard().indexOf(play);
    if (index !== -1) {
      this.displayBoard[index] = playerMark;
    }
    // I made it so it's not returning a boolean anymore.
    // You didn't seem to be using the return value anywhere anyways.
  }
  resetBoard() {
    this.displayBoard = ["", "", "", "", "", "", "", "", ""];
  }
}

This is a generic solution to get rid of duplicate state, but we can actually do even better in this scenario. There are only two places where #getControlBoard() is being used, and neither of them really need this particular data structure. We can simply make them rely on this.displayBoard instead, which actually simplifies the logic a bit.

class GameBoard {
  constructor() {
    this.resetBoard();
  }
  isFull() {
    return this.displayBoard.every((x) => x !== "");
  }
  markBoard(play, playerMark) {
    if (this.displayBoard[play - 1] === "") {
      this.displayBoard[play - 1] = playerMark;
    }
  }
  resetBoard() {
    this.displayBoard = ["", "", "", "", "", "", "", "", ""];
  }
}

I would recommend going further and turning the state into something that more naturally represents the board itself. Tic-tac-toe is played on a 2d board, so perhaps a 3x3 2d array would make more sense as a natural data structure to hold the current state of the board.

There are other areas that keep unnecessary state as well, for example, gameControls keeps track of how many turns have passed since the last time you pushed the reset button (or since the game first started), but you don't actually need that much information anywhere. All you need to know is who's turn it currently is.

// before
let playCounter = 1;
function togglePlayer(player1, player2) {
  playCounter++;
  if (playCounter % 2 === 0) {
    return player1;
  } else {
    return player2;
  }
}

// after
let currentPlayer = 1;
function togglePlayer(player1, player2) {
  currentPlayer = currentPlayer === 1 ? 2 : 1;
  return currentPlayer === 1 ? player1 : player2;
}

Why does this matter? Because it's extra stuff to keep track of, when someone's first reading through your code. It looks like you care about how many turns have passed, when in reality, you don't, so there's no need to make code readers expect that you are using this sort of data in your program. Granted, this is a minor nit-picky thing, but the tiny things add up.

Another locations with extra state would be the players array in the Player class. The plays that a player has done is already found within the game board, you don't need to duplicate pieces of this information across each player. Getting rid of this chunk of state would make it so you don't have to update both the board and the player, each time something changes on the board.

Conclusion

Alright, that's it. Overall, it looks pretty good. It's clean and readable, and you don't have any major issues going on. Keep up the good work :).