Change global variable from event function

Question

I am trying to learn Javascript.

I watched some Javascript course on Youtube and am trying to create my first project.

Project should be simple tic-tac-toe game.

Functionality: First click on box should fill with "X" Second click on another box should fill with "Y" Third click on another box should fill with "X" again and so on until all boxes will be filled with character.

there is my codes HTML

<!DOCTYPE html>
<html lang="en">
<head>
    <meta charset="UTF-8">
    <meta name="viewport" content="width=device-width, initial-scale=1.0">
    <title>Document</title>
    <link rel="stylesheet" href="style.css">
</head>
<body>
<div class="main">
<table>
    <tr>
        <td class="b1-1"></td>
        <td class="b1-2"></td>
        <td class="b1-3"></td>
    </tr>
    <tr>
        <td class="b2-1"></td>
        <td class="b2-2"></td>
        <td class="b2-3"></td>
    </tr>
    <tr>
        <td class="b3-1"></td>
        <td class="b3-2"></td>
        <td class="b3-3"></td>
    </tr>
</table>
</div>
<script src="script.js" type="text/javascript"></script>
</body>
</html>

CSS

.main {
    padding: 100px 0;
    width: 360px;
    margin: 0 auto;
}

table, tbody {
    margin: 0;
    padding: 0;
    width: 360px;
    height: 360px;
}

tr {
    width:360px;
    height: 120px;
    margin: 0;
    padding: 0;
}

td {
    text-align: center;
    width:120px;
    height: 120px;
    border: 1px solid #333;
    margin: 0;
    padding: 0;
    font-size: 50px;
}

Javascript

var action = document.querySelectorAll('td');
var gameState = 0;
for (var i = 0; i <= action.length - 1; i++) {
    getClassName = "." + action[i].classList.value;
        if (gameState === 0) {
            document.querySelector(getClassName).addEventListener("click", chasviX(i));
        } else {
            document.querySelector(getClassName).addEventListener("click", chasviO(i));
        }
}

function chasviX(cord) {
        document.querySelector("." + action[cord].classList.value).addEventListener("click", event => {
            document.querySelector("." + action[cord].classList.value).textContent = "X";
            gameState = 1;
        });
};

function chasviO(cord) {
        document.querySelector("." + action[cord].classList.value).addEventListener("click", event => {
            document.querySelector("." + action[cord].classList.value).textContent = "O";
            gameState = 0;
        });
};

There is project link also - https://jsfiddle.net/qwy2k571/

At the moment each box is filling with "X". I know i didn't understand closures and scope chains exactly but please give me an correct code to understand it by example.

Thanks in advance and best regards.

score 3 · Answer 1 · answered Feb 19 '20 at 13:28

Since you are using querySelectorAll which will give a collection, you can iterate that and directly add event listener to it. Aslo you are adding multiple eventlistener which is not required

var action = document.querySelectorAll('td');
var gameState = 0;
action.forEach((item) => {
  item.addEventListener('click', (e) => {
    if (gameState === 0) {
      e.target.textContent = "X";
      gameState = 1;
    } else {
      e.target.textContent = "0";
      gameState = 0;
    }
  })

})

.main {
  padding: 100px 0;
  width: 360px;
  margin: 0 auto;
}

table,
tbody {
  margin: 0;
  padding: 0;
  width: 360px;
  height: 360px;
}

tr {
  width: 360px;
  height: 120px;
  margin: 0;
  padding: 0;
}

td {
  text-align: center;
  width: 120px;
  height: 120px;
  border: 1px solid #333;
  margin: 0;
  padding: 0;
  font-size: 50px;
}

<script src="https://cdnjs.cloudflare.com/ajax/libs/jquery/3.3.1/jquery.min.js"></script>
<div class="main">
  <table>
    <tr>
      <td class="b1-1"></td>
      <td class="b1-2"></td>
      <td class="b1-3"></td>
    </tr>
    <tr>
      <td class="b2-1"></td>
      <td class="b2-2"></td>
      <td class="b2-3"></td>
    </tr>
    <tr>
      <td class="b3-1"></td>
      <td class="b3-2"></td>
      <td class="b3-3"></td>
    </tr>
  </table>
</div>

score 3 · Answer 2 · answered Feb 19 '20 at 13:37

Here I made a working jsfiddle: https://jsfiddle.net/1jnkepLg/1/

I simplified the JS part to:

var gameState = 0; // Holds the current game state
var cells = document.getElementsByTagName("td"); // list of cells of the game board

// attach an event listener at each cell. 
for (var i=0;i<cells.length;i++) 
{    
    cells[i].addEventListener("click", chasvi);
}   

function chasvi() 
{
       // in event callbacks "this" refers to the element that triggered the event. In this case, it is the TD element.
       this.textContent = gameState ? "X" : "0";

       // switch the game state
       gameState = !gameState;

}

In this case, one event listener is enough, you can determine what X|0 to draw at the callback function.

score 2 · Answer 3 · answered Feb 19 '20 at 13:49

The problem with your solution is that the for loop you have written assigns event listeners for setting an 'X' on the board to every single square BEFORE there is even a single click on the board. This means your if logic will never assign an 'O', since all clicks will call chasviX()

You should instead use a forEach() function of an NodeList like your action variable:

 var gameState = false;
 action.forEach(function(act) {
    act.addEventListener("click", function(event) {
        // If gameState is false(logical 0), place an 'X' on the board
        // Otherwise, place a 'O'
        event.target.textContent = gameState ? 'X' : 'O';
        // Invert state at the end
        gameState = !gameState;
    });
})

score 2 · Accepted Answer · answered Feb 19 '20 at 13:57

In your first loop : for (var i = 0; i <= action.length - 1; i++) { ... } you are adding events listeners to each cells. Since the variable gameState is initialized to 0, the condition if (gameState === 0) { ... } will always be true within that loop.

Instead of checking the status of the game during the initialization, just add an event listener as you did, to each cells.

To make sure you pass the correct parameter to the event, you need to wrap the callback within function() { chasvi(param); }, and the whole body of the loop inside another anonymous function, setting another variable to the coordinate i :

for (var i = 0; i <= action.length - 1; i++) {
    (function(){
          let coor = i;
          let getClassName = "." + action[i].classList.value;
          document.querySelector(getClassName).addEventListener("click", function() { chasvi(coor); });
    }());
}

Note that I changed the name of the function to chasvi because you can manage the case X or O in that function.

In each of the functions chasviX and chasviO you are again adding an event listener. This is not good, because on each clicks, you'll add one more event.

Before clicking, there's already an event.

After clicking once, there are 2 events. Another click, and there's 3 events, and so on...

I suggest you to change thoses functions to 1 function that handles both cases :

function chasvi(cord)
{
    let TextToDisplay;

    if (gameState === 0)
    {
        TextToDisplay = "X";
        gameState = 1;
    }
    else
    {
        TextToDisplay = "O";
        gameState = 0;
    }
    document.querySelector("." + action[cord].classList.value).textContent = TextToDisplay;
}

Since there's only 2 states for gameState, you can use a boolean value. You can initialize it to var gameState = false; I randomly choose false for the X.

And then, the function can be simplified to :

function chasvi(cord)
{
    let TextToDisplay;

    if (gameState === false)
    {
        TextToDisplay = "X";
    }
    else
    {
        TextToDisplay = "O";
    }
    gameState = !gameState; // This swaps the state true/false
    document.querySelector("." + action[cord].classList.value).textContent = TextToDisplay;
}

This kind of notation :

let TextToDisplay;

if (gameState === false)
{
    TextToDisplay = "X";
}
else
{
    TextToDisplay = "O";
}

Can be simplified using ternary expressions :

let TextToDisplay = gameState ? "O" : "X"; // This means if gameState is true, then "O" is returned, else "X"

Final code can looks like this :

var action = document.querySelectorAll('td');
    var gameState = false;
    for (var i = 0; i <= action.length - 1; i++) {
        (function(){
          let coor = i;
          getClassName = "." + action[i].classList.value;
          document.querySelector(getClassName).addEventListener("click", function() { chasvi(coor); });
         }());
    }
    
    function chasvi(cord) {
        let TextToDisplay = gameState ? "O" : "X";
        gameState = !gameState;
        document.querySelector("." + action[cord].classList.value).textContent = TextToDisplay;
    }

.main {
        padding: 100px 0;
        width: 360px;
        margin: 0 auto;
    }
    
    table, tbody {
        margin: 0;
        padding: 0;
        width: 360px;
        height: 360px;
    }
    
    tr {
        width:360px;
        height: 120px;
        margin: 0;
        padding: 0;
    }
    
    td {
        text-align: center;
        width:120px;
        height: 120px;
        border: 1px solid #333;
        margin: 0;
        padding: 0;
        font-size: 50px;
    }

<!DOCTYPE html>
    <html lang="en">
    <head>
        <meta charset="UTF-8">
        <meta name="viewport" content="width=device-width, initial-scale=1.0">
        <title>Document</title>
        <link rel="stylesheet" href="style.css">
    </head>
    <body>
    <div class="main">
    <table>
        <tr>
            <td class="b1-1"></td>
            <td class="b1-2"></td>
            <td class="b1-3"></td>
        </tr>
        <tr>
            <td class="b2-1"></td>
            <td class="b2-2"></td>
            <td class="b2-3"></td>
        </tr>
        <tr>
            <td class="b3-1"></td>
            <td class="b3-2"></td>
            <td class="b3-3"></td>
        </tr>
    </table>
    </div>
    <script src="script.js" type="text/javascript"></script>
    </body>
    </html>

Finally i fount difference between var and let. You are good man — Levan Sakvarelidze, Feb 19 '20 at 14:23
@LevanSakvarelidze check [this](https://stackoverflow.com/questions/762011/whats-the-difference-between-using-let-and-var) question — Cid, Feb 19 '20 at 14:39

score 0 · Answer 5 · answered Feb 19 '20 at 13:45

Your mistake is that you create the events in such a manner that they depend only on the current value of gameState, before any moves were made. You need to check and change the value of gameState inside the event:

var action = document.querySelectorAll('td');
var gameState = 0;
for (var i = 0; i <= action.length - 1; i++) {
    action[i].addEventListener("click", function() {
      this.textContent = (gameState = (gameState + 1) % 2) ? 'x' : '0';
  })
}

https://jsfiddle.net/fby3g12v/

Change global variable from event function

5 Answers5