Simplifying a javascript function with repeated similar lines (with a loop?)

Question

Okay, I hope you don't all facepalm when you see this - I'm still finding my way around javascript.

I am putting together an RSVP form for a wedding website.

I want the guests to be able to add their names to the RSVP form, but only have as many fields showing as required. To this end, after each name field, there is a link to click, which will, when clicked, show a name field for the next guest.

The code below works... but I am sure it can be tidier.

I have tried to insert a for() loop into the code in several different ways, I can see that the for() loop increments correctly to the last value - but when it does so, it leaves only the last addEventListener in place. I can only assume, that I should be using a different kind of loop - or a different approach entirely.

How should I tidy up the following?

<script>
function showNextGuest(i) {
document.getElementsByTagName(\'fieldset\')[i].style.display = \'block\';
}

function initiateShowNextGuest() {
        document.getElementsByTagName('fieldset')[0].getElementsByTagName('a')[0].addEventListener('click',function(){showNextGuest(1);},false);
        document.getElementsByTagName('fieldset')[1].getElementsByTagName('a')[0].addEventListener('click',function(){showNextGuest(2);},false);
        document.getElementsByTagName('fieldset')[2].getElementsByTagName('a')[0].addEventListener('click',function(){showNextGuest(3);},false);
        document.getElementsByTagName('fieldset')[3].getElementsByTagName('a')[0].addEventListener('click',function(){showNextGuest(4);},false);
        document.getElementsByTagName('fieldset')[4].getElementsByTagName('a')[0].addEventListener('click',function(){showNextGuest(5);},false);
}

window.onload = initiateShowNextGuest();
</script>

Can't be certain without seeing the for loop implementations you tried but it sounds like a duplicate of [the JS closure problem](http://stackoverflow.com/questions/750486/javascript-closure-inside-loops-simple-practical-example) — caffeinated.tech, Jan 06 '15 at 11:41
Yes, that's it - that's exactly what I am seeing. The loop has already finished iterating before the function executes for the first time. — Rounin, Jan 06 '15 at 11:48
Great, that solves your loop problem, check out this [resource](https://developer.mozilla.org/en/docs/Web/JavaScript/Guide/Closures) on closures. But from what code you have supplied, it seems you are creating the same event listener for the same types of buttons/links and passing different numbers. There are better ways of doing this using [event delegation](http://davidwalsh.name/event-delegate). — caffeinated.tech, Jan 06 '15 at 11:53

score 1 · Accepted Answer · answered Jan 06 '15 at 11:38

Your intuition is right - a for loop could indeed simplify it and so could a query selector:

var fieldsSet = document.querySelectorAll("fieldset"); // get all the field sets
var fieldss = [].slice.call(asSet); // convert the html selection to a JS array.
fields.map(function(field){
    return field.querySelector("a");  // get the first link for the field
}).forEach(function(link, i){
    // bind the event with the right index.
    link.addEventListener("click", showNextGuest.bind(null, i+1), false); 
});

This can be shortened to:

var links = document.querySelectorAll("fieldset a:first-of-type");
[].forEach.call(links, function(link, i){
     link.addEventListener("click", showNextGuest.bind(null, i+1), false); 
});

Thank you Benjamin, this is a great answer, so I am accepting it, even though it is a little advanced for me at my current level of javascript. Please see below for my own answer - this is what I went with in the end, based on Zaheer Ahmed's Loops and Closures Walk-through (http://conceptf1.blogspot.co.uk/2013/11/javascript-closures.html). — Rounin, Jan 06 '15 at 13:38

score 0 · Answer 2 · edited Jan 06 '15 at 11:39

0

function nextGuest () {
    for(var i = 0; i < 5; i++){
        document.getElementsByTagName('fieldset')[i]
                .getElementsByTagName('a')[0]
                .addEventListener('click',function(){ 
                       showNextGuest(parseInt(i + 1));
                 }, false);
    }
}

edited Jan 06 '15 at 11:39

MackieeE

11,751
4
39
56

answered Jan 06 '15 at 11:37

atmd

7,430
2
33
64

I'd love to vote for this but this will fail for the closure-loop problem - if you run it you'll see they all alert the last `i` value – Benjamin Gruenbaum Jan 06 '15 at 11:38
See http://stackoverflow.com/questions/750486/javascript-closure-inside-loops-simple-practical-example – Benjamin Gruenbaum Jan 06 '15 at 11:39
Thanks atmd, this syntax more or less reproduces the problem I was already seeing. This was as far as I got before I got stuck. As Benjamin states, it doesn't work because of the closure-loop problem. Now I see I have a new topic to get to grips with as I continue to learn .js: Scope and Closures. – Rounin Jan 06 '15 at 13:33
yeah sorry about that, I went off a little half cocked, read the optimise with a loop bit and started writting a loop before reading about the closure issue. My bad. Good look in your continued JS learning – atmd Jan 06 '15 at 13:34

Rounin · Answer 3 · 2015-01-06T14:05:42.893

Benjamin's answer above is the best given, so I have accepted it.

Nevertheless, for the sake of completeness, I wanted to show the (simpler, if less elegant) solution I used in the end, so that future readers can compare and contrast between the code in the question and the code below:

<script>
var initiateShowNextGuest = [];

function showNextGuest(j) {
    document.getElementsByTagName('fieldset')[j].style.display = 'block';
}

function initiateShowNextGuestFunction(i) {
    return function() {
        var j = i + 1;
        document.getElementsByTagName('fieldset')[i].getElementsByTagName('a')[0].addEventListener('click',function(){showNextGuest(j);},false);
    };
}

function initiateShowNextGuests() {
    for (var i = 0; i < 5; i++) {
        initiateShowNextGuest[i] = initiateShowNextGuestFunction(i);
        initiateShowNextGuest[i]();
    }
}

window.onload = initiateShowNextGuests();
</script>

In summary, the function initiateShowNextGuests() loops through (and then executes) initiateShowNextGuestFunction(i) 5 times, setting up the 5 anonymous functions which are manually written out in the code in the original question, while avoiding the closure-loop problem.

Simplifying a javascript function with repeated similar lines (with a loop?)

3 Answers3