1

What I want is this (news ticker type functionality):

  1. Get a list of li's from a ul tag
  2. Loop through all the li's and get the text
  3. display the text in the console via firefox console.log()
  4. get the next li and repeat till all li's have been displayed

That's the goal, but setTimeout is not running as I thought it would. Only the LAST iteration ("Post Four") is showing. And that ("Post Four") is showing four times in a row. 

<body>
<ul id="post_list">
 <li>Post One</li>
 <li>Post Two</li>
 <li>Post Three</li>
 <li>Post Four</li>
</ul>

<script type="text/javascript">
var ul = document.getElementById('post_list');
var li = ul.getElementsByTagName('li');

for(var x=0; x < li.length; x++){
    var li_text = li[x].childNodes[0].nodeValue;
    setTimeout(function(){showText(li_text)}, 1000);
}

function showText(text) {
    console.log(text);
}           
</script>
</body>
sleeper
  • 3,446
  • 7
  • 33
  • 50
  • 3
    Classic function-in-a-loop problem. This question has been asked over and over and over and over... – Matt Ball Nov 09 '11 at 20:13
  • 1
    Check the "Related" list on the right column (which is essentially the same list as you got while typing the question title, did you see it?). – BalusC Nov 09 '11 at 20:17
  • possible duplicate of [How do JavaScript closures work?](http://stackoverflow.com/questions/111102/how-do-javascript-closures-work) – Ivo Wetzel Nov 09 '11 at 20:38
  • Your loop basically requests 4 timers to start at nearly the same time, each ending at nearly the same time 1000ms later. – Incognito Nov 09 '11 at 20:38
  • @MattBall Huh, must have selected the wrong one then, actually wanted to suggest one of the setTimeout thing on the side. – Ivo Wetzel Nov 09 '11 at 21:04
  • possible duplicate of [setTimeout in a for-loop and pass i as value](http://stackoverflow.com/questions/5226285/settimeout-in-a-for-loop-and-pass-i-as-value) – Bergi Aug 29 '12 at 16:00

4 Answers4

4

The reason this is happening is because of closures. The for loop block has closures around it, so when you reference 'li_text' it was always equal the last value that li_text was set to. The for loop does not create a separate closure for each iteration through the loop.

GAgnew
  • 3,847
  • 3
  • 26
  • 28
  • 1
    Hmm.. seems like I've stumbled into a hornets nest! After reviewing the answers, this is the best explanation of what is wrong **AND WHY**. I'm not interested in just getting a code fix, rather I need to **understand** WHY I'm not getting the correct results. In this case, seems that the answer is *closures*. So im going to go off and try to more clearly understand this problem. – sleeper Nov 09 '11 at 21:07
  • @Sleeper, If you didn't want the timeouts to be sequential, why didn't you just create one timeout that does the looping after it's timed out? No closures required and same functionality. sigh. :) – Esailija Nov 09 '11 at 21:14
  • @sleeper +1 for being a real programmer and not a script kiddie – GAgnew Nov 09 '11 at 21:21
2

As Greg mentioned the problem is with the closure only evaluating once. Nobody posted a solution for this so here is one. This uses adding a function that generates the callback function each time:

Add: 

function getShowTextCallback(text) {
    return function(){showText(text)}
}

Then use it in loop like this:

for(var x=0; x < li.length; x++){
    var li_text = li[x].childNodes[0].nodeValue;
    setTimeout(getShowTextCallback(li_text), 1000);
}
Lycha
  • 9,937
  • 3
  • 38
  • 43
0

Change this:

for(var x=0; x < li.length; x++){
    var li_text = li[x].childNodes[0].nodeValue;
    setTimeout(function(){showText(li_text)}, 1000);
}

To:

for(var x=0; x < li.length; x++) (function() {
    var li_text = li[x].childNodes[0].nodeValue;
    setTimeout(function(){showText(li_text)}, x * 1000);
})()
qwertymk
  • 34,200
  • 28
  • 121
  • 184
  • 2
    This is a terribly convoluted answer, a real case of treating the symptom and not the problem. – GAgnew Nov 09 '11 at 20:16
0

How about we just move some of the code around a bit... take out the closure issue... 

var ul = document.getElementById('post_list');
var li = ul.getElementsByTagName('li');

for (var x = 0, xl = li.length; x < xl; x++) {
    var li_text = li[x].innerText || li[x].textContent; // does IE support textContent??
    showText(li_text, x * 1000);
}

function showText(text, delay) {
    setTimeout(function() {
        console.log(text);
    }, delay);
}

I assume the delay you want to be sequential (hence the loop). Because setTimeout is not blocking you will need to have a callback on the function to invoke the next setTimeout or you will need to specifically increment each function call with a new delay.

rlemon
  • 17,518
  • 14
  • 92
  • 123
  • Y U NO CACHE LENGTH. You realise `li` is life right? and that `.length` is expensive – Raynos Nov 09 '11 at 20:41
  • I'm not going to address ALL of the problems with the code... otherwise I would just answer half of the jQuery questions with "Y U NO USE DOM" – rlemon Nov 09 '11 at 20:44
  • no excuse to copy paste bad code. You also use `.childNodes` without filtering out text nodes. Also `.nodeValue` does not do what you think it does. I think you want `.textContent` – Raynos Nov 09 '11 at 20:46
  • LOL @Raynos, you are speaking to the OP right? To be honest I looked at the issue at hand and half ignored the rest of the code.. but if you must be Raynos and break balls then I will fix my answer. – rlemon Nov 09 '11 at 20:50
  • IE doesn't support `.textContent` :( – Raynos Nov 09 '11 at 20:55
  • @Raynos how is .length expensive? – GAgnew Nov 09 '11 at 21:19