Issue with Javascript performance (Odin Project, Etch-a-Sketch)-CodePudding

I'm currently learning Javascript on the Odin Project and I'm doing the Etch-a-Sketch exercise in which you're supposed to create a Etch-a-Sketch board you can draw on with your cursor. You're supposed to implement a resize button which allows the user to enter how many tiles they want on each side of the board, with a max value of 100 (for a 100x100 board). I did just that, and it technically works... except my browser starts chugging around 60 tiles, and crashed if I try to go for the max.

The exercise teaches you how to use Javascript to manipulate the DOM, so I got my script to create a div container for the entire board, and a default 16x16 board is created when starting the page. Each tile is a div with an event listener assigned to it which detects when it's hovered. A new class is given to the tile, which changes its color according to the CSS rule tied to that class.

The reset button calls a function when it's clicked. That function resets the classes for all tiles and prompts the user for a value. Once it gets a proper value, the CSS rule associated with the original div container gets updated with the requested amount of rows and columns, a function is called which removes all existing tiles, and another function fills the div container with new tiles.

HTML

<!DOCTYPE html>
<html>
<head>
  <title>Page Title</title>
  <meta charset="UTF-8"/>
  <link rel="stylesheet" href="style.css">
  <script src="script.js" defer></script>
</head>
<body>
  <button >Reset and resize</button>
</body>
</html>

CSS

body {
    display: flex;
    flex-direction: column;
}

.reset {
    align-self: flex-start;
}

.sketchContainer {
    width: 900px;
    height: 900px;
    border: solid 1px black;
    display: grid;
    align-self: center;
    grid-template-rows: repeat(16, auto);
    grid-template-columns: repeat(16, auto);
}
div.tileHovered {
    background-color: #c3a375;
}

JavaScript

generateDefaultGrid();
placeEventListenersOnTiles();
const button = document.querySelector(".reset");
button.addEventListener("click", reset);

function generateDefaultGrid() {
    const sketchContainer = document.createElement("div");
    document.body.appendChild(sketchContainer);
    sketchContainer.classList.add("sketchContainer");
    const gridTile = document.createElement("div");
    sketchContainer.appendChild(gridTile);
    gridTile.classList.add("gridTile");
    let clonedTile;
    for (let i = 0; i < 16*16-1; i  ) {
        clonedTile = gridTile.cloneNode();                  //On ne peut pas copier un node en répétant les appendChild, autrement ça ne manipulera qu'une seule instance du node. IL faut le cloner.
        sketchContainer.appendChild(clonedTile);
    }
}

function placeEventListenersOnTiles() {
    let allTiles = document.querySelectorAll(".gridTile");
    allTiles.forEach(element => {
        element.addEventListener("mouseover", changeColor);
    });
}

function changeColor() {
    this.classList.add("tileHovered");                      //"this" permet de faire référence à l'élément sur lequel le listener a été placé.
}

function reset() {
    let allTiles = document.querySelectorAll(".tileHovered");
    allTiles.forEach(element => {
        element.classList.remove("tileHovered");
    });
    resizeGrid();
}

function resizeGrid() {
    let newSize = prompt("How many squares do you want per side of the Etch-a-Sketch?", "Please insert a number inferior or equal to 100.");
    if (newSize > 100) {
        newSize = prompt("Sorry, that number is too high!", "Please insert a number inferior or equal to 100.");
    } else if (Math.sign(newSize) == 0 || Math.sign(newSize) == -1) {                                                                           //Ne pas oublier que les returns de prompt sont des strings, pas des chiffres !
        newSize = prompt("Only integers between 1 and 100, please!", "Please insert a number inferior or equal to 100.");
    } else if (newSize === null) {
        generateDefaultGrid();
    } else {
        const styleSheet = document.styleSheets[0];
        styleSheet.cssRules[2].style.gridTemplateRows=`repeat(${newSize}, auto)`;
        styleSheet.cssRules[2].style.gridTemplateColumns=`repeat(${newSize}, auto)`;
        removeAllTiles();
        generateUserGrid(newSize);
    }
}

function removeAllTiles() {
    let container = document.querySelector(".sketchContainer");
    while (container.firstChild) {
        container.removeChild(container.firstChild);
    }
}

function generateUserGrid(newSize) {
    const sketchContainer = document.getElementsByClassName("sketchContainer");
    const gridTile = document.createElement("div");
    sketchContainer[0].appendChild(gridTile);
    gridTile.classList.add("gridTile");
    let clonedTile;
    for (let i = 0; i < newSize*newSize-1; i  ) {
        clonedTile = gridTile.cloneNode();                  //On ne peut pas copier un node en répétant les appendChild, autrement ça ne manipulera qu'une seule instance du node. IL faut le cloner.
        sketchContainer[0].appendChild(clonedTile);
        placeEventListenersOnTiles();
    }
}

I hope I'm doing the whole asking questions right, I'm pretty new to this. I've got the current state of my project pushed to my GitHub if it helps. Can someone tell me why my script is so inefficient, and how I could go about to fix it? That sort of thing wasn't really covered in the course up to that point, I'm afraid.

Thanks a lot!

CodePudding user response：

The performance problem is caused by placeEventListenersOnTiles(); being called within the for loop in generateUserGrid function.
So each iteration adds more and more event listeners again and again.
E.g. in 100x100 setup the first tile ends up with 10000 event listeners, the second one with 9999, and so on.

Just move it outside of the loop:

generateDefaultGrid();
placeEventListenersOnTiles();
const button = document.querySelector(".reset");
button.addEventListener("click", reset);

function generateDefaultGrid() {
  const sketchContainer = document.createElement("div");
  document.body.appendChild(sketchContainer);
  sketchContainer.classList.add("sketchContainer");
  const gridTile = document.createElement("div");
  sketchContainer.appendChild(gridTile);
  gridTile.classList.add("gridTile");
  let clonedTile;
  for (let i = 0; i < 16 * 16 - 1; i  ) {
    clonedTile = gridTile.cloneNode(); //On ne peut pas copier un node en répétant les appendChild, autrement ça ne manipulera qu'une seule instance du node. IL faut le cloner.
    sketchContainer.appendChild(clonedTile);
  }
}

function placeEventListenersOnTiles() {
  let allTiles = document.querySelectorAll(".gridTile");
  allTiles.forEach(element => {
    element.addEventListener("mouseover", changeColor);
  });
}

function changeColor() {
  this.classList.add("tileHovered"); //"this" permet de faire référence à l'élément sur lequel le listener a été placé.
}

function reset() {
  let allTiles = document.querySelectorAll(".tileHovered");
  allTiles.forEach(element => {
    element.classList.remove("tileHovered");
  });
  resizeGrid();
}

function resizeGrid() {
  let newSize = prompt("How many squares do you want per side of the Etch-a-Sketch?", "Please insert a number inferior or equal to 100.");
  if (newSize > 100) {
    newSize = prompt("Sorry, that number is too high!", "Please insert a number inferior or equal to 100.");
  } else if (Math.sign(newSize) == 0 || Math.sign(newSize) == -1) { //Ne pas oublier que les returns de prompt sont des strings, pas des chiffres !
    newSize = prompt("Only integers between 1 and 100, please!", "Please insert a number inferior or equal to 100.");
  } else if (newSize === null) {
    generateDefaultGrid();
  } else {
    const styleSheet = document.styleSheets[0];
    styleSheet.cssRules[2].style.gridTemplateRows = `repeat(${newSize}, auto)`;
    styleSheet.cssRules[2].style.gridTemplateColumns = `repeat(${newSize}, auto)`;
    removeAllTiles();
    generateUserGrid(newSize);
  }
}

function removeAllTiles() {
  let container = document.querySelector(".sketchContainer");
  while (container.firstChild) {
    container.removeChild(container.firstChild);
  }
}

function generateUserGrid(newSize) {
  const sketchContainer = document.getElementsByClassName("sketchContainer");
  const gridTile = document.createElement("div");
  sketchContainer[0].appendChild(gridTile);
  gridTile.classList.add("gridTile");
  let clonedTile;
  for (let i = 0; i < newSize * newSize - 1; i  ) {
    clonedTile = gridTile.cloneNode(); //On ne peut pas copier un node en répétant les appendChild, autrement ça ne manipulera qu'une seule instance du node. IL faut le cloner.
    sketchContainer[0].appendChild(clonedTile);
  }
  placeEventListenersOnTiles();
}

body {
  display: flex;
  flex-direction: column;
}

.reset {
  align-self: flex-start;
}

.sketchContainer {
  width: 450px;
  height: 450px;
  border: solid 1px black;
  display: grid;
  align-self: center;
  grid-template-rows: repeat(16, auto);
  grid-template-columns: repeat(16, auto);
}

div.tileHovered {
  background-color: #c3a375;
}

<button >Reset and resize</button>

Probably you want to go further and learn event delegation. So you'd have just 1 event listener set on the container to handle all the tiles:

generateDefaultGrid();
const button = document.querySelector(".reset");
button.addEventListener("click", reset);

function generateDefaultGrid() {
  const sketchContainer = document.createElement("div");
  document.body.appendChild(sketchContainer);
  sketchContainer.classList.add("sketchContainer");

  // SEE HERE:
  sketchContainer.addEventListener("mouseover", changeColor);

  const gridTile = document.createElement("div");
  sketchContainer.appendChild(gridTile);
  gridTile.classList.add("gridTile");
  let clonedTile;
  for (let i = 0; i < 16 * 16 - 1; i  ) {
    clonedTile = gridTile.cloneNode(); //On ne peut pas copier un node en répétant les appendChild, autrement ça ne manipulera qu'une seule instance du node. IL faut le cloner.
    sketchContainer.appendChild(clonedTile);
  }
}

// AND HERE:
function changeColor(event) {
  event.target.classList.add("tileHovered");
}

function reset() {
  let allTiles = document.querySelectorAll(".tileHovered");
  allTiles.forEach(element => {
    element.classList.remove("tileHovered");
  });
  resizeGrid();
}

function resizeGrid() {
  let newSize = prompt("How many squares do you want per side of the Etch-a-Sketch?", "Please insert a number inferior or equal to 100.");
  if (newSize > 100) {
    newSize = prompt("Sorry, that number is too high!", "Please insert a number inferior or equal to 100.");
  } else if (Math.sign(newSize) == 0 || Math.sign(newSize) == -1) { //Ne pas oublier que les returns de prompt sont des strings, pas des chiffres !
    newSize = prompt("Only integers between 1 and 100, please!", "Please insert a number inferior or equal to 100.");
  } else if (newSize === null) {
    generateDefaultGrid();
  } else {
    const styleSheet = document.styleSheets[0];
    styleSheet.cssRules[2].style.gridTemplateRows = `repeat(${newSize}, auto)`;
    styleSheet.cssRules[2].style.gridTemplateColumns = `repeat(${newSize}, auto)`;
    removeAllTiles();
    generateUserGrid(newSize);
  }
}

function removeAllTiles() {
  let container = document.querySelector(".sketchContainer");
  while (container.firstChild) {
    container.removeChild(container.firstChild);
  }
}

function generateUserGrid(newSize) {
  const sketchContainer = document.getElementsByClassName("sketchContainer");
  const gridTile = document.createElement("div");
  sketchContainer[0].appendChild(gridTile);
  gridTile.classList.add("gridTile");
  let clonedTile;
  for (let i = 0; i < newSize * newSize - 1; i  ) {
    clonedTile = gridTile.cloneNode(); //On ne peut pas copier un node en répétant les appendChild, autrement ça ne manipulera qu'une seule instance du node. IL faut le cloner.
    sketchContainer[0].appendChild(clonedTile);
  }
}

body {
  display: flex;
  flex-direction: column;
}

.reset {
  align-self: flex-start;
}

.sketchContainer {
  width: 450px;
  height: 450px;
  border: solid 1px black;
  display: grid;
  align-self: center;
  grid-template-rows: repeat(16, auto);
  grid-template-columns: repeat(16, auto);
}

div.tileHovered {
  background-color: #c3a375;
}

<button >Reset and resize</button>