JS Use for loop variable in function definition, without the function changing during iterations

Question

I am coding a game in which the user must select one of the cards sent from the back-end, but I don't know what cards, or how many, until I recieve them. I must make a button for each, and sent the chosen one to the back end. Since I don't know how many cards there will be, I define each button's action in a for loop.

Since I use the loop variable to acces the numerical code that must be sent to the back-end, after each iteration of the loop the numerical code sent by the function changes.

Here's how I solved the problem:

(cards is an array with the cards, and for any card in it, card['code'] is it's numerical code. The 11 in sendJSON means that the info sent is the chosen card)

for (var i = 0; i < cards.length; i++) {
    container.innerHTML += `<button><img src="Cards/${cards[i]['code'].toString()}.png" width="346" height="529" id="${cards[i]['code'].toString()}"></button>`;

    // Here is the problem:
    eval('document.getElementById(' + cards[i]['code'].toString() + ').onclick = async function f() {await sendJSON(11, ' + cards[i]['code'] + ')}')
}

The problem is the eval() since not only it is unsafe, it also has bad performance, so I was wondering...

Is there a better way to do this?

Just write a function expression! But [close over the right variables](https://stackoverflow.com/questions/750486/javascript-closure-inside-loops-simple-practical-example) — Bergi, Jan 04 '21 at 22:43

score 1 · Accepted Answer · answered Jan 04 '21 at 22:41

Since you're putting the code in the id attribute, the event listener can simply use this.id to get it, you don't need to copy it into the function.

You also need to use insertAdjacentHTML() rather than concatenating to innerHTML. When you concatenate, all the HTML in the container is re-parsed, and any event listeners on previous images will be lost. insertAdjacentHTML() only parses the new HTML and leaves all the old elements unchanged.

for (var i = 0; i < cards.length; i++) {
  container.insertAdjecentHTML('beforeend', `<button><img src="Cards/${cards[i]['code']}.png" width="346" height="529" id="${cards[i]['code']}"></button>`);

  document.getElementById(cards[i]['code']).addEventListener("click", async function f() {
    await sendJSON(11, this.id)
  });
}

There's also no need for all the toString() calls. Substituting into a template literal automatically converts it to a string.

Felix Kling · Answer 2 · 2021-01-04T22:43:07.070

You could store the code as a data-* property and use the same event handler for every element (or use event delegation as mentioned in the comments). I'd also suggest to not use innerHTML for such simple markup, especially not inside a loop (see Barmar's answer for why it won't work anyway):

function handler(event) {
  sendJSON(11, event.dataset.code);
}

for (var i = 0; i < cards.length; i++) {
  const img = document.createElement('img');
  img.src = `Cards/${cards[i]['code']}.png`;
  img.dataset.code = 'code';
  img.width = 346;
  img.height = 529;
  img.onclick = handler;

  const button = document.createElement('button');
  button.appendChild(img);
  container.appendChild(button);
}

JS Use for loop variable in function definition, without the function changing during iterations

2 Answers2