JavaScript Tic-Tac-Toe

Question

The first function makes 2 players

const Player = (name,mark) => {
return {name, mark}
}
let player1 = Player('Player 1', '<div>X</div>');
let player2 = Player('Player 2', '<div>O</div>');

The rest of the code which runs the game

const gameBoard = (() => {
  board = [["","",""],["","",""],["","",""]];
  let whichPlayer = getCurrentPlayer();

  const AddEventListeners = (() => {
    $('.square').click(function(event) {
        if(win == false && this.innerHTML == ''){
            console.log(this)
        let a = addToBoard(whichPlayer()).bind(this);
        a(this.id)}
    })
    $('.restart').click(function(e) {
        board = [["","",""],["","",""],["","",""]];
        $('.square').empty()
        btnDiv.style.display = "none";
        $('.winnermsg').hide();
        win = false;
    })
  })();

  function getCurrentPlayer(){
    let currentPlayer = player1
    function getPlayer() {
        if(currentPlayer === player1){
            currentPlayer = player2
            crntPlayer = player1
            return player1
        }else{
            currentPlayer = player1
            crntPlayer = player2
            return player2
        }
    }
    return getPlayer;
  }

  function addToBoard(currentPlayer) {
    return function(square){
        this.innerHTML = currentPlayer.mark
        let coords = square.split('')
        board[coords[0]][coords[1]] = currentPlayer.mark
        checkWin()
    }

  }
  function declareWinner(tie = false){
    win = true;
    $('.winnermsg').show();
    if(tie){
        $('.winnermsg').html(`<p>Tie game, try again</p>`);
    }else{
        $('.winnermsg').html(`<p>Congratulations, ${crntPlayer.name} you have won</p>`);
    }
    if (crntPlayer == player1){whichPlayer();}
    btnDiv.style.display = "grid"
  }

  function checkWin(){
    //check horizontal
    for(let i=0;i<board.length;i++){
        array=[]
        if(board[i].every(x => x == crntPlayer.mark )){
            declareWinner()
        }
        //check vertical
        for(let j=0;j<board[i].length;j++){
            array.push(board[j][i])
            if(array.every(x=>x == crntPlayer.mark && array.length == 3)){
                declareWinner()
            }
        }            
    }
    //check diags
    newArray=[]
    newArray.push(board[0][0],board[1][1],board[2][2])
    if(newArray.every(x => x == crntPlayer.mark)){
        declareWinner()
    }
    newArray=[]
    newArray.push(board[0][2],board[1][1],board[2][0])
    if(newArray.every(x => x == crntPlayer.mark)){
        declareWinner()
    }
    //check tie
    newArray=board.reduce((a,b) => a.concat(b))
    if(!newArray.includes('')){declareWinner('tie')}


  }

})();

So I just add event listeners to each div and if it's empty it changes the innerHTML to the player's mark and adds that mark to the board array and then checks if there's a winner.

I feel like it's not very well organized but i'm not sure how it would best be improved.

Answer 1

Just a few pointers, some of which are my personal preference!

• Since you use modern concepts like arrow functions, const, and shorthand objects, you might as well replace jQuery $(".square") with document.querySelectorAll(".square")
• Passing a div as an argument is not a good idea. I would keep all visualisation of the game in one separate function, for example: function drawBoard(). This function does all the DOM manipulation, and the DOM elements or jQuery functions should not be mentioned anywhere else in your code.
• I would use one game state array, and the event handlers should alter that game state array directly. If square 1 is clicked by player one, the state array would become [1,0,0,0,0,0,0,0,0], and after the game state has updated, you call drawBoard() which draws the X and O visuals in the DOM.
• You can use this state array in your checkWin function too, instead of using so many temporary arrays! If you use numbers you can even see who won just by adding the numbers.
• You use a lot of temporary variables. I've added a suggestion to switch players without using those.
• Don't nest so many functions in each other, don't call them implicitly by sometimes adding () at the end, and other times not. It's hard to reason about.
• Arrow functions don't have their own scope, but sometimes you DO need a scope, for example in your gameboard class.

Below are just a few sketches to rewrite the code, I realise this is far from complete but it might give you some ideas:

function Player(name, mark, id){
  this.name = name
  this.mark = mark
  this.id = id
}

function GameBoard(){
  let state = [0,0,0,0,0,0,0,0,0]
  let player1 = new Player("Mark","X", 1)
  let player2 = new Player("Joe", "O", 2)
  let currentPlayer = player1

  this.initHandlers = function(){
    let fields = document.querySelectorAll(".field")
    for(let f of fields){
     f.addEventListener("click", (e)=> this.updateState(e))
    }
  }

  this.updateState = function(e){
    console.log("id of the clicked tile is " + e.target.id)
    let id = parseInt(e.target.id)
    state[id] = currentPlayer.id
    this.drawBoard()
    this.checkWin()
    currentPlayer = this.switchPlayers()
  }


  this.switchPlayer = function(){
      return (currentPlayer == player1) ? player2 : player1
  }

  this.drawBoard = function(){
    // all DOM manipulation here
  }

  this.checkWin = function(){
    if(state[0] + state[3] + state[6] == 3) console.log("player one wins!")
  }
}

let board = new GameBoard()
board.initHandlers()