Is there a way of optimizing that "addEventListener" block of code?

Question

in an HTML page, i've got 7 buttons whit the following IDs: #I, #II, #III, #IV, #V, #VI, #VII placed in a rContainer div. I want to set the iActiveButton variable in correlation with the touched button. My code seems to work properly but i feel that it could be enhanced and shortened. Any idea?

var iActiveButton;

function touchHandler(){
  console.log(iActiveButton);
}

rContainer.addEventListener("touchstart", function(){

  I.addEventListener("touchstart", function(){iActiveButton = 1;}, false);
  II.addEventListener("touchstart", function(){iActiveButton = 2;}, false);
  III.addEventListener("touchstart", function(){iActiveButton = 3;}, false);
  IV.addEventListener("touchstart", function(){iActiveButton = 4;}, false);
  V.addEventListener("touchstart", function(){iActiveButton = 5;}, false);
  VI.addEventListener("touchstart", function(){iActiveButton = 6;}, false);
  VII.addEventListener("touchstart", function(){iActiveButton = 7;}, false);

  touchHandler();

}, false);

Possible duplicate of [How to add event listeners to an array of objects](https://stackoverflow.com/questions/17981437/how-to-add-event-listeners-to-an-array-of-objects) — VirxEC, Sep 06 '19 at 13:01
_“My code seems to work properly”_ - but it isn’t really “proper”, because every time touchstart occurs on rContainer, you add a _new_ touchstart listener to all those buttons. Since you’re only assigning a value to a variable, that doesn’t actually hurt - but it doesn’t make much sense to begin with. — misorude, Sep 06 '19 at 13:01
All those standalone variable names are quite a code smell too - if possible, change them so you can grab them all at once with `querySelectorAll`, then iterate through them — CertainPerformance, Sep 06 '19 at 13:02
Adding to the comments above, iterate over them and use a `for` loop with `let` keyword to assign the listeners. This is important that you use `let` and not `var`. — Nabeel Mehmood, Sep 06 '19 at 13:05
If this is code you've written, and you want a review of all aspects of the code, and you're willing to share a [mre] of the code, you should read [A guide to Code Review for Stack Overflow users](https://codereview.meta.stackexchange.com/q/5777/13492) and [their help center](https://codereview.stackexchange.com/help/dont-ask) to see if the question is on-topic for [codereview.se]. — Heretic Monkey, Sep 06 '19 at 13:05
[Don't rely on element IDs becoming global variables!](https://stackoverflow.com/q/25325221/1048572) — Bergi, Sep 06 '19 at 13:22

score 0 · Accepted Answer · answered Sep 06 '19 at 13:14

One way to approach this is to look at the event target of the outer event listener.

rContainer.addEventListener("touchstart", function(event){
  const targetId = event.target.getAttribute('id');

  // don't set iActiveButton if you clicked empty space inside `rContainer`
  if (targetId) {
    // romanNumeralMap looks like { 'I': 1, 'II': 2, etc... }
    iActiveButton = romanNumeralMap[targetId];
  }

  touchHandler();
}, false);

While this does require mapping from roman numerals to regular numbers, this way you are only adding one event listener!

score 0 · Answer 2 · answered Sep 06 '19 at 13:18

If you really want to keep the logic of your code like that and just want a refactor, I would go with this:

const buttonArray = [I, II, III, IV, V, VI, VII];
rContainer.addEventListener("touchstart", function(){
  buttonArray.forEach((button, index) => {
    button.addEventListener("touchstart", function(){iActiveButton = index;}, false);
  }

  touchHandler();
}, false);

But I completely agree with the comments, adding new EventListeners on every event seems strange.

Is there a way of optimizing that "addEventListener" block of code?

2 Answers2