views:

591

answers:

4
    for(var i=0; i<barValues.length; i++) {


 actualBarHeight = Math.floor((barValues[i]/chartMaxY)*barchartHeight);

 var barChartID = "#barChart" + (i+1)
 $(barChartID + " .value span").css('background-color','transparent');
 $(barChartID + " img").animate({ 
   height: actualBarHeight
  }, 500, function(){$(barChartID + " .value span").css('background-color','white');}
 );

 $(barChartID + " .value span").html("$"+Math.floor(barValues[i]));
 $(barChartID + " .value").css("bottom",actualBarHeight+"px");
 $(barChartID + " .ylabel").html(chartMaxY);

};

The above bit of jQuery is inside a for loop. Each iteration of the loop does the following:

  • sets the background of a span
  • animates an object
  • upon finishing, resets the background of the span

I'm using a call back function to reset the background so it finishes the animation before doing so. However, it only ends up affecting the last span referenced in the for loop.

If I move that bit of code outside of the callback, then it effects every span through every iteration of the for loop, (but doesn't wait for the animation in that case)

I'm guessing the issue has to do with building the selector INSIDE the function INSIDE the animate function. Is there some bad syntax in my markup?

EDIT (per Russ's suggestion, I now include the full loop in the above sample)

A: 

You could try caching the span selector, like so:

var barChartID = "#barChart" + (i+1)
var span = $(barChartID + " .value span");
span.css('background-color','transparent');
$(barChartID + " img").animate({ 
    height: actualBarHeight
}, 500, function() {
    span.css('background-color','white');
});
Scott Bale
Hmm...same results. It only affects the last span of the loop only.
DA
If I do this: span.css('background-color','white');alert(span.css('background-color');I then get 3 alerts (as I'm looping 3 times) and each time it's returning a value of white for each.However, only the last one in the loop is rendered that way. When I go into fireBug to look at each of the 3 spans, they each have inline background colors...the first two being transparent, the last white. Very odd.
DA
+7  A: 

This is a common problem experienced when combining closures with loops. JavaScript is a late-binding language and loops do not introduce a new scope. So:

for (var i= 0; i<5; i++) {
    $('#thing'+i).click(function() {
        alert(i);
    });
}

There is only one i variable in this code. It starts at 0 and once the assignment-loop is finished is left at 5. The click event on the #thing0 element is only ever going to be fired after the loop has finished executing, by which point the value of i will be 5. You will not get the define-time value 0 which you might have expected.

This applies not only to the loop variable itself but to any other variables you are re-assigning for each time round the loop too. So in your example the value of barChartID inside the animation callback function will always be the id associated with the last element in your loop.

The usual workaround is to take a copy of the loop variable's value at define-time by using a structure that does introduce a new scope, namely another function:

$(barChartID + " img").animate({height: actualBarHeight}, 500, function(barChartID) {
    return function() {
        $(barChartID + " .value span").css('background-color','white');
    };
}(barChartID));

More on looped closures.

bobince
bobince: WOW! Thanks for that excellent explanation! Your example code works perfectly. I don't quite follow the syntax (yet) but I now understand what was happening. To trace the logic, it looks like my callback function is, itself, calling a function which returns itself?
DA
Means that your callback function is evaluated after the for cycles are finished, giving barChartID the value of the last cycle.
Alex Bagnolini
It's a function-factory: you pass in things you want to remember to the outer function and an inner function that can see those things is created, then returned out from the outer function. There will be a more standardised/readable way to do this in the future (and now if you hack it in), namely `Function.prototype.bind`.
bobince
+1- I was 99% sure the problem would be the loop :)
Russ Cam
Thanks, all! I'll be reading up on these things.
DA
+2  A: 

This is because your callback function has an implicit pointer to the barChartID variable. This is just how closures work. What you want is to create a new copy of the current value of barChartID in each iteration. One pattern for fixing this is to run the body of the for loop inside a function. I saw this pattern in an except from John Resig's upcomming book Secrets of the JavaScript Ninja

for(var i=0; i<barValues.length; i++) function(i){
    ...
}(i);
Gabe Moothart
+1  A: 

Since you are using jQuery you can use the $.each function instead of a for loop. The callback used instead of the loop will create a new closure.

$.each(barValues, function(i, barValue){
    actualBarHeight = Math.floor((barValue/chartMaxY)*barchartHeight);

    var barChartID = "#barChart" + (i+1)
    $(barChartID + " .value span").css('background-color','transparent');
    $(barChartID + " img").animate({ 
                    height: actualBarHeight
            }, 500, function(){$(barChartID + " .value span").css('background-color','white');}
    );

    $(barChartID + " .value span").html("$"+Math.floor(barValue));
    $(barChartID + " .value").css("bottom",actualBarHeight+"px");
    $(barChartID + " .ylabel").html(chartMaxY);
});
mcrumley