3
\$\begingroup\$

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!

\$\endgroup\$
1
  • \$\begingroup\$ Thanks Sam for editing! \$\endgroup\$
    – ablueblaze
    Nov 10 at 11:49
2
\$\begingroup\$

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;
    }
}
\$\endgroup\$
3
  • 2
    \$\begingroup\$ "Note that includes does not work in all browsers, not that anybody should be using IE9 anymore" the OP is also using es6 features like arrow functions and even class expressions which are not supported by IE so the point is moot. \$\endgroup\$ Nov 3 at 15:15
  • \$\begingroup\$ @SᴀᴍOnᴇᴌᴀ, I agree but some of us unfortunate souls still have to cater for fools running IE :-( \$\endgroup\$
    – jdt
    Nov 3 at 15:46
  • \$\begingroup\$ Thank you for the feed back. I was afraid that my little project was to simple for anyone to take a look at. :) \$\endgroup\$
    – ablueblaze
    Nov 10 at 11:48

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.