0

I got this javascript recursive function:

function doStuff(graphName) {
    var groupArray = new Array();
    groupArray[0] = "hour";
    groupArray[1] = "day";
    groupArray[2] = "month";
    for(var i = 0; i < groupArray.length; i++) {
        $.get("getchartdata", {"graphName" : graphName, "subgroup" : groupArray[i]})
              .done(function(jsonData){
                  var data = eval(jsonData);
                  drawChart(data, data[0][0], data[0][1]);
              });

    }
    setTimeout(doStuff, 10000);
}

Now the problem is that it works great the first time, but after 10 seconds when tries again, it shows an error:

TypeError: data[0] is undefined in drawChart(data, data[0][0], data[0][1]);

Why could this be happening?

If I add the parameter in setTimeout(doStuff(graphName), 10000);

The browser crashes.

Thanks.

Improve this question
5
  • 1
    Do never ever use eval there! (Not for JSON parsing, and not when it is already parsed) Commented Apr 29, 2013 at 16:13
  • it's not a recursive function. Commented Apr 29, 2013 at 16:16
  • Remember that $.get is asynchronous so calling doSomething before all calls complete can cause problems if the calls take longer than 10 seconds to complete. You may want to use $.ajax and set async = false. Commented Apr 29, 2013 at 16:21
  • @rontornambe no, never ever use async: false ! Commented Apr 29, 2013 at 16:22
  • Although I do not believe in hard and fast rules, in this case I do agree async would be a bad idea. A better solution is to include a counter should and not call doSOmething until groupArray.length is reached. Commented Apr 29, 2013 at 16:31

3 Answers 3

Reset to default
8

I think what you want is this:

setTimeout(function() { 
    doStuff(graphName); 
}, 10000);

It is also worth noting that if your AJAX call takes more than 10 seconds to complete you may start to see some 'glitches'. Perhaps consider moving the timeout to inside the .done callback (this would mean it run again 10 seconds after the ajax has completed). However, this is just a suggestion, if this doesn't suit your needs then you can keep it as it is. Also, this may not be suitable as you are calling the ajax in a for loop, and you may end up with more timeouts than you want, if you don't implement it correctly

Improve this answer
Sign up to request clarification or add additional context in comments.

1 Comment

Thank you very much for your reply ! since you were the first to post it, i will give you the tick! :)
3

If you want to pass on the graphname parameter, you'll need to explicitly do so! Seems like you want

function doStuff(graphName) {
    var groupArray = ["hour", "day", "month"];
    for(var i = 0; i < groupArray.length; i++) {
        $.get("getchartdata", {"graphName" : graphName, "subgroup" : groupArray[i]})
              .done(function(data){
                  drawChart(data, data[0][0], data[0][1]);
              });
    }
    setTimeout(function() {
        doStuff(graphName); // again
    }, 10000);
}

Another possibility would be to bind the argument for the next doStuff invocation:

setTimeout(doStuff.bind(this, graphName), 10000);
Improve this answer

5 Comments

Thanks! this was the correct answer, but musefan answered first! i'd like to give you the tick too :'( you've been very helpful!
@Alnitak let me check yours.
@Bergi no, there's a perfectly good way of doing this without repeatedly passing the same parameters (or using .bind)
@Alnitak: Not sure whether the extra closure is much cleaner. Depends on the number and variability of parameters and on personal style I'd say…
I use this pattern a lot for pseudo-recursion. It also allows you to maintain loop counters and stuff nicely without suffering from loop/closure scope issues.
3

It's usual in this case to do the hard work in an inner function, with the originally supplied parameter available via the closure:

function doStuff(graphName) {
    (function loop() {
         // draw the graph using "graphName" from the outer scope
         ...
         setTimeout(loop, 10000);
    })();  // invoke immediately to start the process
}

using the closure avoids the repetition of passing the parameters over and over, and the additional function wrapper around that call, since you can just pass the reference to the inner function.

This also works well with AJAX - just put the setTimeout call inside the .done handler. As you're making three AJAX calls, try this in the inner function which will wait for all three AJAX calls to complete before starting the timer:

var def = [];

for (var i = 0; i < groupArray.length; i++) {
    def[i] = $.get("getchartdata", {"graphName" : graphName, "subgroup" : groupArray[i]})
          .done(function(data) {
              drawChart(data, data[0][0], data[0][1]);
          });
}

// wait for all three deferred objects to be resolved
$.when.apply($, def).done(function() { setTimeout(loop, 10000) });
Improve this answer

4 Comments

Putting the timeout into the ajax callback gets a bit complicated here since he has 3 of them running in parallel :-) 10s should be fine, and he could use the timeout parameter for avoiding glitches…
@Bergi nah, that's trivial with a $.when call.
$.when.apply($, ["hour", "day", "month"].map(function(g) { return $.get("getchartdata", {"graphName":graphName,"subgroup":g}).done(function(d){ drawChart(d, d[0][0], d[0][1]);}); })).done(setTimeout.bind(window, doStuff.bind(this, graphName), 10000)); might be trivial to us, but not to beginners :-)
@Bergi well, if you will insist on complicating it with those two .bind calls...

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.