Form iteration function not working properly

Question

My function is supposed to iterate all forms in the document, and bind an onclick function to each 'calculate' element int he form. The problem is, the function that executes on any of the click events executes in the context of the the last i in the loop.

Here is the JavaScript that I'm using:

window.onload = function(){ 
    calculateSavings();
}

function calculateSavings(){
    for (i = 0; i < document.forms.length; i++) {
        var e = document.forms[i];
        e.calculate.onclick = function() { 
            var hours = e.hours.value;
            var rate = e.rate.value;
            alert(hours * rate);
        }
    }
}

And here is the HTML it is attached to:

<!doctype html>
<html>
<body>
<form>
  <label for="hours">Hours: </label><input type="text" id="hours" name="hours">
  <label for="rate">Rate: </label><input type="text" id="rate" name="rate">
  <input type="button" name="calculate" value="Calculate">
  <div id="savings"></div>
</form>
<form>
  <label for="hours">Hours: </label><input type="text" id="hours" name="hours">
  <label for="rate">Rate: </label><input type="text" id="rate" name="rate">
  <input type="button" name="calculate" value="Calculate">
  <div id="savings"></div>
</form>
</body>
</html>

I'm sure this is a really basic question but the solution is completely eluding me at this point.

Here is the demo of the problem I'm experiencing: http://jsfiddle.net/KcNWy/

Are you open to using jQuery? Would ease the DOM manipulation. — meder omuraliev, Jan 06 '11 at 21:50
do all different elements in all forms have the same ID's/per form? — Caspar Kleijne, Jan 06 '11 at 21:51
@meder Honestly, I'd rather avoid jQuery. Aside from adding a ('savings').innerHTML statement, the little JavaScript you see here is all I will be using on the page. — Moses, Jan 06 '11 at 21:55
the way you are doing `document.forms` and `document.calculate` is very oldschool and I would advise throwing a class on the calculate elements and implementing a getElementsByClassName or use jQuery to grab them, then iterate and attach the event listener. — meder omuraliev, Jan 06 '11 at 21:59

score 4 · Accepted Answer · edited May 23 '17 at 10:33

You have the Closure Loop Problem. See, I don't know, this or about a million other SO questions caused by the same gotcha.

i and e are the same variables each time around the loop: the loop merely changes i and e, it doesn't give you a new e. So when the function() { ... } fires, long after the loop has finished iterating, e is left with the final value it had, the last matching input.

(Also, you haven't declared var i, so the i is worse than that, it's an accidental global.)

The typical way around it is to add another function scope to retain a copy of the value at creation time, in a closure:

e.elements.calculate.onclick = function(e) { 
    return function() {
        var hours = e.elements.hours.value;
        var rate = e.elements.rate.value;
        alert(hours * rate);
    }
}(e);

You can do it more cleanly in ECMAScript Fifth Edition with the Function#bind method:

e.elements.calculate.onclick = function(e) { 
    var hours = this.elements.hours.value;
    var rate = this.elements.rate.value;
    alert(hours * rate);
}.bind(e);

but that means adding support for this method to existing browsers that don't support it yet.

Alternatively putting the closure in the loop mechanism itself avoids the problem:

forEach(document.forms, function(form) {
    form.elements.calculate.onclick = function() { 
        var hours = form.elements.hours.value;
        var rate = form.elements.rate.value;
        alert(hours * rate);
    }
});

function forEach(list, callback) {
    for (var i= 0, n= list.length; i<n; i++)
        callback(list[i]);
}

Alternatively alternatively, just use the same function each time and grab your form reference from the clicked element:

for (var i= document.forms.length; i-->0;)
    document.forms[i].elements.calculate.onclick= calculate_click;

function calculate_click() {
    var hours = this.form.elements.hours.value;
    var rate = this.form.elements.rate.value;
    alert(hours * rate);
}

I never understood closures when I read through JS: The Definitive Guide, yet you solved my problem and helped me finally understand how closures work in just a few minutes! Also, thanks for catching the global i :) — Moses, Jan 06 '11 at 22:17

score 0 · Answer 2 · answered Jan 06 '11 at 21:51

0

Could you use "this" to reference within the onclick function instead of using "e"?

answered Jan 06 '11 at 21:51

polarblau

17,649
7
63
84

Form iteration function not working properly

2 Answers2