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 — Bergi
– Bergi, Commented Jan 4, 2021 at 22:43

Barmar · Accepted Answer · 2021-01-04 22:41:49Z

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 · Accepted Answer · 2021-01-04 22:43:07Z

0

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);
}

edited Jan 4, 2021 at 22:43

answered Jan 4, 2021 at 22:33

Felix Kling

820k181 gold badges1.1k silver badges1.2k bronze badges

Collectives™ on Stack Overflow

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

2 Answers 2

Comments

Comments

Your Answer

Linked

Hot Network Questions

Collectives™ on Stack Overflow

2 Answers 2

Comments

Comments

Your Answer

Sign up or log in

Post as a guest

Linked

Related