3

I'm making a form where the user can RSVP for themselves and up to 5 others. Each guest has a text field for their names and then radio buttons (yes or no) as to whether they are attending or not. By default Guest 1 & 2 are displayed and then there is a button the user can click to add another guest up until guest #6.

Guest's 3 - 6 are contained in their own objects like this:

var guestThree = {
  name : document.querySelector('label[for="nameOfThirdGuest"]'),
  attendingYes : document.querySelector('label[for="guestThreeAttendingYes"]'),
  attendingNo : document.querySelector('label[for="guestThreeAttendingNo"]')
};
var guestFour = {
  name : document.querySelector('label[for="nameOfFourthGuest"]'),
  attendingYes : document.querySelector('label[for="guestFourAttendingYes"]'),
  attendingNo : document.querySelector('label[for="guestFourAttendingNo"]')
};
var guestFive = {
  name : document.querySelector('label[for="nameOfFifthGuest"]'),
  attendingYes : document.querySelector('label[for="guestFiveAttendingYes"]'),
  attendingNo : document.querySelector('label[for="guestFiveAttendingNo"]')
};
var guestSix = {
  name : document.querySelector('label[for="nameOfSixthGuest"]'),
  attendingYes : document.querySelector('label[for="guestSixAttendingYes"]'),
  attendingNo : document.querySelector('label[for="guestSixAttendingNo"]')
};

I am hiding guest's 3 - 6 using the following function which are then converted to methods for the above objects:

function hideFields() {
  this.name.style.display = "none";
  this.attendingYes.style.display = "none";
  this.attendingNo.style.display = "none";
}

I also have the opposite of the above function in showFields:

function showFields() {
  this.name.style.display = "block";
  this.attendingYes.style.display = "block";
  this.attendingNo.style.display = "block";
}

So the idea here is that when the add guest button (which is assigned to the variable addGuest) is clicked guest #3 will be added. Then when it's clicked again guest #4 will be added and so on until #6. I've added the guestThree - guestSix objects to the following array:

var extraGuests = [guestThree, guestFour, guestFive, guestSix];

and then using the following for loop:

for (var guest = 0; guest < extraGuests.length; guest++) {
  addGuest.onclick = function() {
      extraGuests[guest].showFields();
  };
}

But this doesn't work. I have discovered through testing that guest loops all the way round to 4 without executing the showFields methods. If I was to add a break after the onclick event then guestThree will display but that's it (obviously because that's how break works) but this atleast shows that the loop works as I hoped - to an extent.

So what I am asking of you guys is - what am I doing wrong? How do I make it so that the guest variable doesn't loop straight to 4? I need each iteration to be 0, 1, 2 and finally 3 and as far as I can see that is exactly what should be happening?

Cheers!

Space Goat
  • 93
  • 1
  • 1
  • 5
  • 11
    You've closed over a loop variable. See : http://stackoverflow.com/questions/750486/javascript-closure-inside-loops-simple-practical-example – spender Nov 14 '14 at 17:03
  • 1
    Why are you writing separate functions to show elements? There are easier ways of doing it with less copy/paste in your methods. – epascarello Nov 14 '14 at 17:05
  • I think what happens is that it loops 0-3 but every time it re-writes the previous function. In the end you only get the final result because all the other ones were overwritten. If the length of the array was 5 then the function would you only display element 5 etc. – Code Whisperer Nov 14 '14 at 17:07

3 Answers3

3

There are two problems:

The first is that you're overwriting the onclick on addGuest on every loop, so only the last assignment survives. So if we fix the second problem (below), you'll still only show the last set of fields. Probably, we want the handler assigned once, and have it reveal the "next" set of fields.

The second problem is that the function you're assigning to onclick has an enduring reference to the guest variable, not a copy of it as of when the function was created. So later, when the function gets called, it sees the value guest has after the end of the for loop. Your main choices for solving that are a builder function, and Function#bind as described under this question. But since we don't need or want the loop, it doesn't matter.

Instead, I'd suggest a variable telling you what the next set of fields to reveal is, and then simply one function that uses it:

var extraGuests = [guestThree, guestFour, guestFive, guestSix];
var nextGuest = 0;
addGuest.onclick = function() {
    var guest = extraGuests[nextGuest];
    if (guest) {
        ++nextGuest;
        guest.showFields();
    }
};
Community
  • 1
  • 1
T.J. Crowder
  • 1,031,962
  • 187
  • 1,923
  • 1,875
  • Thanks for this but as with the other guys solution both of these just make guestSix display and none of the ones before it? – Space Goat Nov 14 '14 at 17:30
  • @SpaceGoat: I just realized: You're overwriting the click handler on the `addGuest` object on each pass. The correct implementation of it will need to find the first un-displayed set of fields and show that. Note that using a bunch of fields with names like "nameOfSixthGuest" is usually a sign that you want to do things differently. In this case, you probably just want to add fields dynamically when needed, rather than pre-create fields you may not use and then show them. – T.J. Crowder Nov 14 '14 at 17:41
  • @SpaceGoat: Now that I better understand the problem, I've updated the solution. – T.J. Crowder Nov 14 '14 at 17:46
  • Ah yes, that does make sense actually. I could just write guests 3 - 6 to the DOM using Javascript instead of creating them in the HTML. That way I wouldn't have to go through the hassle of hiding them and all that jazz! I'll have a go at re-writing my code... – Space Goat Nov 14 '14 at 17:49
  • 1
    Just tried your new solution and it works perfectly. Thanks dude! – Space Goat Nov 14 '14 at 17:50
2

You've closed over a loop variable, which means that at the time they are called, your click handlers are all looking at a single value of guest, which will be extraGuests.length (the value it settles on when the loop is complete).

You need to capture the loop variable at the time the loop executes, which can be done easily using an Immediately-invoked function expression. :

for (var guest = 0; guest < extraGuests.length; guest++) {
  (function(ind){
     addGuest.onclick = function() {
        extraGuests[ind].showFields();
     };
  })(guest);
}

There's probably better/more succinct ways to do this using newer language features.

Community
  • 1
  • 1
spender
  • 117,338
  • 33
  • 229
  • 351
0

Maybe this.

var guest = 0;
addGuest.addEventListener('click', function() {
  if(guest < extraGuests.length)
    extraGuests[++guest].showFields();
}, false);
Quentin Engles
  • 2,744
  • 1
  • 20
  • 33