Is it possible to beautify this JavaScript code?

Question

I have done my research but i cant find anything useful that are related to my question.

Original JavaScript Code

$("#furniture1").keyup(function(){
  samefunction();
});
$("#furniture2").keyup(function(){
  samefunction();
});
$("#furniture3").keyup(function(){
  samefunction();
});

$("#color1").keyup(function(){
  samefunction();
});
$("#color2").keyup(function(){
  samefunction();
});
$("#color3").keyup(function(){
  samefunction();
});

I can only come up with this solution, is there better solution?

var index_no = 0;

for(var i=1; i<=3; i++)
{
    index_no++;
    var name = '#furniture';
    name += index_no;

    $(name).keyup(function () {
        samefunction();
    });

    var name = '#color';
    name += index_no;

    $(name).keyup(function () {
        samefunction();
    });
}

Suren Srapyan · Accepted Answer · 2017-11-20T07:53:30.493

3

Just separate selectors with ,.

$("#furniture1, #furniture2, #furniture3, #color1, #color2, #color3").keyup(function(){
  samefunction();
});

Example

$("#furniture1, #furniture2, #furniture3, #color1, #color2, #color3").keyup(function(){
    console.log(`Fired from the input ${this.id}`);
});

<script src="https://ajax.googleapis.com/ajax/libs/jquery/2.1.1/jquery.min.js"></script>
<input id="furniture1">
<input id="furniture2">
<input id="furniture3">

For dynamic you need a loop. You can just decrease the code written in the loop like

for(var i = 1; i <= 3; i++)
{
    $(`#furniture${i}, #color${i}`).keyup(function () {
        samefunction();
    });
}

Or as @Satpal have mentioned in the comments you can add a common class for all elements and use that class to bind the event handler like.

$(".someClass").keyup(function(){
    console.log(`Fired from the input ${this.id}`);
});

<script src="https://ajax.googleapis.com/ajax/libs/jquery/2.1.1/jquery.min.js"></script>
<input id="furniture1" class='someClass'>
<input id="furniture2" class='someClass'>
<input id="furniture3" class='someClass'>

edited Nov 20 '17 at 07:53

answered Nov 20 '17 at 07:42

Suren Srapyan

66,568
14
114
112

what if i have the index number to 24? – LearnProgramming Nov 20 '17 at 07:45
You will have dynamic indexes ? – Suren Srapyan Nov 20 '17 at 07:45
1

@learnprogramming, Add a CSS common class then use class selector to bind event handler – Satpal Nov 20 '17 at 07:46
@SurenSrapyan not dynamic...just curious what if the index number is greater than 10...would it be really messy if i put the name and followed by comma until it reaches 10 – LearnProgramming Nov 20 '17 at 07:51
@learnprogramming Yes it will be messy. I have edited the answer. Please read again – Suren Srapyan Nov 20 '17 at 07:52
@Satpal ermm i dont really understand, still learning in programming js... – LearnProgramming Nov 20 '17 at 07:52
@learnprogramming see [kumar_harsh's answer](https://stackoverflow.com/a/47387204/) – Satpal Nov 20 '17 at 07:54
the edited answer is quite nice, i will use this for now...if i have time i will use the one @kumar_harsh does – LearnProgramming Nov 20 '17 at 08:07

kumarharsh · Answer 2 · 2017-11-20T13:14:51.660

3

Option 1: Use event delegation. This solution is pure javascript:

document.querySelector('body').addEventListener('keyup', (e) => {
  if (e.target.classList.contains('keyup-capturer')) {
    somefunction();
  }
});

Demo

$ctr = document.getElementById('counter');
document.querySelector('body').addEventListener('keyup', (e) => {
  if (e.target.classList.contains('keyup-capturer')) {
    somefunction();
  }
});
function somefunction() {
    console.log(`Fired from the input ${this.id}`);
    $ctr.innerHTML = parseInt($ctr.innerHTML, 10) + 1;
};

<input id="furniture1" class="keyup-capturer">
<input id="furniture2" class="keyup-capturer">
<input id="furniture3" class="keyup-capturer">
<input id="color1" class="keyup-capturer">
<input id="color2" class="keyup-capturer">
<input id="color3" class="keyup-capturer">

<div>Key Ups: <span id="counter">0</span></div>

Option 2 ^[*]: If you do need to use jQuery, you can either use event delegation, or use a class for calling the same function on when keyup is captured in any element on that class:

$('.keyup-capturer').keyup(function() { samefunction(); });

And assign the .keyup-capturer class to all the #furniture-x and #color-x element IDs. I don't recommend this approach anymore. See the discussion below.

^{[*] Some observations:}

^{Use event
delegation over
class-based listeners for best results.}
^{Don't use for loops or forEach or map to attach event handlers. You'll just be adding a lot of event handlers, and eventually, you'll find your page growing slower with more complexity.}
^{Avoid attaching listeners to a set of selected elements (such as shown in Option #2). See this line in jQuery's source code. jQuery hides the implementation detail from you, but under the hoods, it still attaches one event listener per element. So, if you're using non-ID-based selectors such as class selectors $('.c1'), or attribute selectors $('[disabled]'), etc), keep in mind this fact. Thanks to @t.niese for pointing this out in the comments.}

edited Nov 20 '17 at 13:14

answered Nov 20 '17 at 07:45

kumarharsh

18,961
8
72
100

Why do you complain about the bad performance of a `map` or `forEach` in the other question and in this answer, while suggesting the same in yours? A `$(".keyup-capturer").keyup(...)` will also iterate over each matched element to attach an individual event listener to each of them. – t.niese Nov 20 '17 at 08:05
Yes, it is. Use addEventListener – kumarharsh Nov 20 '17 at 08:08
Using `$(".keyup-capturer").keyup(...)` will add the same amount of event handlers compared to a solution that uses `forEach` and `addEventListener`, the jQuery api just hides this from you. – t.niese Nov 20 '17 at 08:12
I'm not recommending the `$classSelector.keyup` approach. I'm recommending the event-delegation approach. – kumarharsh Nov 20 '17 at 08:13
At the time I wrote the comments you didn't recommend the event-delegation, and the answer was contradictory, now you have corrected most parts. The `[...]you can either use event delegation, or use a class for calling the same function on when keyup is captured in any element on that class[...]` in option 2 and the `Don't use for loops` is still misleading, because it indicates that using class and delegate events would be similar, but using classes will use a loop. – t.niese Nov 20 '17 at 09:15
@t.niese actually, I had put the recommendation before your first comment came in (check the timestamps), but yeah, SO does not do auto-refresh sometimes, so it can seem all weird when answers are evolving quickly. At least we do agree on the approach ;-) – kumarharsh Nov 20 '17 at 10:43
Plus, I didn't know about the implicit use of for-loop in jQuery for class-based selectors. Thanks for the info. Is there some documentation or reference for it, so that I may refer to it in my answer? – kumarharsh Nov 20 '17 at 10:45
JS only allows to directly attache event handlers, therefore, jQuery is logically also limited to this. The relevant part in the docs `[...]The .on() method attaches event handlers to the currently selected set of elements in the jQuery object[...]` and github [jquery/event.js](https://github.com/jquery/jquery/blob/3e902a812014e8a6980e08599c4840b1cb969f7c/src/event.js#L93). The `.on` will always add an event handler to every element in the jQuery result set on that it is called. – t.niese Nov 20 '17 at 12:49

bobharley · Answer 3 · 2017-11-21T08:10:02.293

2

try adding a class furniture on all furnitures, class color on all colors, and so on... so that you can select them by group:

let furnitures = document.querySelectorAll('.furnitures');
furnitures.forEach((furniture) => {
   furniture.addEventListener('keyup', function() {
      // your code here
   })
})

or you can pass your function directly:

let furnitures = document.querySelectorAll('.furnitures');
furnitures.forEach((furniture) => {
   furniture.addEventListener('keyup', samefunction)
})

edited Nov 21 '17 at 08:10

answered Nov 20 '17 at 07:45

bobharley

662
3
17

1

```furnitures.forEach((furniture) => { furniture.addEventListener('keyup', samefunction()) })``` Won't work because you'll be executing the handler and not binding it. It should be ``` let furnitures = document.querySelectorAll('.furnitures'); furnitures.forEach((furniture) => { furniture.addEventListener('keyup', samefunction) }) ``` – josephnvu Nov 20 '17 at 07:53

Is it possible to beautify this JavaScript code?

3 Answers3

Demo

Linked

Related