How can I reduce the copy-paste code? Refactor?

Question

How can I refactor this piece of code so it contains less copy-paste code?

$("#hnv_4").click(function(e){
    manipulate(4);
    e.stopPropagation();
});
$("#hnv_3").click(function(e){
    manipulate(3);
    e.stopPropagation();
});
$("#hnv_2").click(function(e){
    manipulate(2);
    e.stopPropagation();
});
$("#hnv_1").click(function(e){
    manipulate(1);
    e.stopPropagation();
});

Can I use a loop here to minimize the code or maybe some jQuery?

I tried:

for (i = 1; i <= 4; i++) {
 $("#hnv_" + i).click(function (e) {
    alert(i);

});
}

but at the end .. alert shows 5 always

Seems straight-forward; count from 4 to 1, capture the loop variable, use it to build the selector and call to `manipulate`. What have you tried so far? — Dave Newton, Jul 19 '13 at 16:54
i tried this ---> for (i = 1; i <= 4; i++) { $("#hnv_" + i).click(function (e) { alert(i); }); } — Zafta, Jul 19 '13 at 16:56
possible duplicate of [Javascript closure inside loops - simple practical example](http://stackoverflow.com/questions/750486/javascript-closure-inside-loops-simple-practical-example) — Bergi, Jul 19 '13 at 17:15
This question is a better fit for [codereview](http://codereview.stackexchange.com) — Ja͢ck, Jul 19 '13 at 17:26

score 12 · Accepted Answer · edited Jul 19 '13 at 16:58

12

Yes,

$("[id^='hnv_']").click(function(e) {
    var number = Number(this.id.split("_")[1]);
    manipulate(number);
    e.stopPropagation();
});

edited Jul 19 '13 at 16:58

Ian

50,146
13
101
111

answered Jul 19 '13 at 16:55

tymeJV

103,943
14
161
157

Is e automatically passed in the callback or does that need to be specified as an argument? – ernie Jul 19 '13 at 16:58
@tymeJV I added the `Number()` part because the OP's code is passing a `Number` to `manipulate`, while `this.id` returns a string – Ian Jul 19 '13 at 17:01
@Ian -- Thanks for the edit, was just about to add the `event` param, didn't even realize the Number cast :D – tymeJV Jul 19 '13 at 17:02
Don't want to be nitpicking, but isn't it sub optimal performance-wise ? (the selector) – Eran Medan Jul 19 '13 at 17:03
@EranMedan True, but it's probably better than looping and having to query the DOM multiple times. – Ian Jul 19 '13 at 17:04
@Ian Perhaps :) I'm tempted for a jsPerf , but I'm fighting the urge :) – Eran Medan Jul 19 '13 at 17:05
1

I'd expect the browser to have a hash table of IDs, so looking them up directly should be fast. The prefix selector has to scan the entire DOM, doing string matches. – Barmar Jul 19 '13 at 17:07
@Barmar I think you're right; specifically since this has to do with the `id` attribute, it might be quicker to query the DOM a set number of times, instead of letting it scan all elements and matching the string. It might be different for other attributes – Ian Jul 19 '13 at 17:09
@EranMedan Actually, that would be a great idea. I think I'm wrong; Barmar convinced me (only because we're dealing with `id` though). – Ian Jul 19 '13 at 17:10
@tymeJV Yeah, it's no big deal, I just thought it would match up with the OP's code better :) – Ian Jul 19 '13 at 17:11
@EranMedan Here's the jsperf that I came up with: http://jsperf.com/getelementbyid-loop-vs-attribute-selector . I think it mirrors what we're talking about. So of course, I'm wrong :) I'm gonna try with a different attribute – Ian Jul 19 '13 at 18:02
1

@Ian -- 86% slower?!?! Lies! – tymeJV Jul 19 '13 at 18:34
@tymeJV Go back to the jsperf and tell me what surprises you in the results (I tested in more browsers). Make sure you look at each browser's results – Ian Jul 19 '13 at 18:38
@Ian -- Just surprised it was THAT much slower, other than that, just humoring you. – tymeJV Jul 19 '13 at 18:39
@tymeJV Haha no I agree. But look at IE8's results. `querySelectorAll` is faster. WTF – Ian Jul 19 '13 at 18:39
@Ian -- IE9 in IE8 compat mode is giving me `querySelectorAll` as 91% slower, hmm – tymeJV Jul 19 '13 at 18:41
@tymeJV I'm using a native IE8 :) – Ian Jul 19 '13 at 18:42

score 9 · Answer 2 · edited May 23 '17 at 11:56

9

Change your HTML to this:

<div class="hnv" data-hnv="1">...</div>
<div class="hnv" data-hnv="2">...</div>
and so on

Then change the jQuery to:

$(".hnv").click(function(e) {
    manipulate($(this).data("hnv"));
    e.stopPropagation();
}

If you want to do it with a for loop, you have to capture the index in closure, using an immediately-executing function:

for (var i = 1; i <= 4; i++)
{
    (function (i) {
        $("#hnv_" + i).click(function(e){
            manipulate(i);
            e.stopPropagation();
        })
    })(i);
}

See JavaScript closure inside loops – simple practical example for the explanation of why your loop doesn't work, and the extra function is needed.

edited May 23 '17 at 11:56

Community

1
1

answered Jul 19 '13 at 16:55

Barmar

741,623
53
500
612

2

Whilst this is probably a better way of laying out the HTML, it's not the correct answer given the question IMO. – Ian Clark Jul 19 '13 at 16:57
1

@IanClark And that's why this shouldn't be accepted, but should be upvoted as a great/valid alternative :) – Ian Jul 19 '13 at 17:00
OK, I added the for-loop solution, you happy? – Barmar Jul 19 '13 at 17:01
2

I like this much, much better than my comment; +1. IMO it should be accepted--it answers the *real* question. – Dave Newton Jul 19 '13 at 17:04
@DaveNewton The question isn't "How can I change my HTML so my JavaScript doesn't need to be duplicated". I **agree** that this is what I wish people did more often (Barmar's first suggestion with the `data-*` attributes and common `class`) and I normally suggest it too, but the immediate question was just about condensing the JS. I know I know, we should always suggest the best practice...but since the OP was already storing the value in the `id`, I don't see a problem working off that – Ian Jul 19 '13 at 17:07
1

He asked how to refactor his code. Unless he specifically says that the HTML can't be changed, I consider them to go hand in hand, and refactoring the code includes the possibility of refactoring the HTML. – Barmar Jul 19 '13 at 17:08
@Barmar Unfortunately, he did **not** ask to refactor his code. He asked how to **minimize** his code. Check the edit history. I don't know why we're even arguing about this; I'll gladly take back my claim that this shouldn't be accepted. The "refactor" text was added by another person. And your answer assumes the `id`s aren't required. Maybe there's other code that needs them (like `` elements). If so, why duplicate the number at the end of the `id` and put it in another attribute? – Ian Jul 19 '13 at 17:12
@Ian I know about the edit history. That's just because he didn't know the correct word for what he wanted. – Barmar Jul 19 '13 at 17:13
@Ian There's no reason why he can't keep the IDs if they're required for some other reason. Although any time you see IDs like this, it means you're not using proper DRY principles. Probably all the other code that uses the IDs can be refactored to use the class instead. – Barmar Jul 19 '13 at 17:14
@Barmar You read his mind and knew what he wanted? Maybe you know what he **should** want, and that's what this answer shows. In case you didn't see my last comment's edit, there's no reason to argue about this. I take back my "this shouldn't be accepted" because I don't know what I was thinking :) This is definitely the setup that should be used – Ian Jul 19 '13 at 17:14
@Barmar No you're right about the whole `id`/`class` thing. Sometimes I get caught up in what the OP has and forget about how I'd set it up so this type of problem wouldn't happen (like your first example). – Ian Jul 19 '13 at 17:16
@Barmar And not to be a pest, but I wouldn't suggest your second example how it is (although I know you were doing it to satisfy the first comment on your answer, and mine); I'd suggest that method, but not specifically putting any functions inside of the loop. It could easily be changed to not create functions in the loop, and use/reuse some instead. Not a big deal, I know it was an example, and it works fine as is – Ian Jul 19 '13 at 17:20
1

@Ian You have to create functions in the loop to capture `i` in the closure. There are other ways to write it, but they're effectively the same. If you think another method is preferable, post an answer. – Barmar Jul 19 '13 at 17:24
The two comment threads in the two highly up-voted answers are why I really like SO . . . lots of stuff to be learned for those of us who want to dig a little deeper. – ernie Jul 19 '13 at 17:30
@Barmar This: http://jsfiddle.net/TeMRj/ is more or less what I meant. You're right, it accomplishes the same thing (which I understood), I was just saying I thought it was the better/cleaner way to do what you were showing. I wasn't trying to argue against your method, and I don't think it's substantial enough to create a new answer, because your answer and the accepted answer provide enough – Ian Jul 22 '13 at 16:43
@Ian I would use your method if the function were large, as embedding long functions is unesthetic to me. But for simple functions that would not be reused, I prefer to put them inline. What JS is really missing is something like Scheme/Lisp's `let`, which is syntactive sugar replacing the function definition but keeping its scope rules. – Barmar Jul 22 '13 at 18:00

score 0 · Answer 3 · answered Jul 19 '13 at 17:17

I haven't tested this, but this should approximately do what you want to do.

for(var x=1; x<5; x++) { 
  $("#hnv_"+x).click(function(e){
    manipulate( e.target.id.substr(e.target.id.search('_')+1) );    
    e.stopPropagation();
  }) 
}

I'm doing the poor man's solution of scrubbing the "id" number out of the HTML id attribute when I:

e.target.id.substr(e.target.id.search('_')+1)

But if you are using jQuery, you may have the ".data" method where you could store the id in, and then not have to do that scrubbing hack. e.g.

for(var x=1; x<5; x++) { 
  $("#hnv_"+x).data("id",x).click(function(e){
    manipulate( $(e.target).data("id") );    
    e.stopPropagation();
  }) 
}

Not tested! feel free to correct me.

How can I reduce the copy-paste code? Refactor?

3 Answers3