Tell me more ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote 3 down vote favorite
2

I have written a small JavaScript class that makes use of setInterval to create a delayed loop for iterating an array. I've used this technique in the past but never utilised a library to do so (and as a result it produced quite messy spaghetti code), and so I wrote up this:

var DelayedLoop = function() { };

DelayedLoop.forEach = function(collection, delay, callback, completedCallback) {
    var index = 0;
    var timerObject;

    var executor = function() {
        // Stop the delayed loop.
        clearInterval(timerObject);

        // Executes the callback, and gets the returned value to indicate what the next delay will be.
        var newInterval = callback(collection[index++]);

        // If they returned false, quit looping.
        if(typeof(newInterval) == "boolean" && newInterval == false) {
            return;
        }

        // If nothing / non-number is provided, re-use initial delay.
        if(typeof(newInterval) != "number") {
            newInterval = delay;
        } else if(newInterval < 0) { // If a negative number is returned, quit looping.
            return;
        }

        // If there are more elements to iterate, re-set delayed loop. Otherwise, call the "completed" callback.
        if(index < collection.length) {
            timerObject = setInterval(executor, newInterval);
        } else {
            completedCallback();
        }
    };

    // Initial loop setup.
    timerObject = setInterval(function() {
        executor();
    }, delay);
};

An example usage can be seen here:

$(document).ready(function() {
    var myArray = [ "aaa", "bbb", "ccc" ];
    DelayedLoop.forEach(myArray, 1000, function(arrayElement) {
        console.log(arrayElement);
    }, function() {
        console.log("Finished all!");
    });
});

How does it look? Are there any browser compatibility issues? Is there any potential flaw I'm not seeing?

share|improve this question

2 Answers

up vote 2 down vote accepted

Jeff Mercado touched on many important things, so here are some other things to consider.

1: There's no need to define DelayedLoop as a function; you're not using it as one. Instead you can just do a simple object literal:

var DelayedLoop = { ... };

or simply have a function called delayedForEach. If forEachis the only function in DelayedLoop, there's not much need for the DelayedLoop namespace.

2: You could consider copying the collection array right away. Otherwise the array your forEach function is using can be changed elsewhere, outside the loop. This shouldn't cause any errors with your code, but could lead to confusion.

3: Maybe do some type-checking and throw a TypeError if, for instance, collection isn't an array instance, or callback isn't a function.

4: No need to both check newInterval's type and value when checking if it's false; just use a strict comparison operator: newInterval === false. I'd also encourage you to use only false as your "stop looping" value; if a callback returns a negative number, just ignore it. This is more in line with how e.g. event handlers or jQuery each callbacks work; they can return anything they want, but boolean false - and only boolean false - is treated as "stop".

5: No need to wrap the timed call to executor in a function; just write executor (as Jeff Mercado shows). I'd probably name it "iterator" as that's a bit more descriptive.

6: Minor syntax thing: typeof is an operator - not a function - so there's no need to write typeof(variable). In fact the more common way is to write typeof variable

7: Another minor syntax thing: You don't have to declare the executor as an expression. A straight-up function executor() { ... } syntax would work just as well. Of course, since both work it's mostly a matter of personal preference.

share|improve this answer
I was also considering making the same change with using the strict comparison but since the return value could have been of differing types, I like seeing specific wording that indicates the type in all the different cases. If it was expected to be of a single type, then yes definitely. – Jeff Mercado Dec 2 '12 at 22:32
@JeffMercado The code's identical, though. The wording is just as specific, since the strict comparison literally means "it must be a boolean and it must be false". To be honest, I find the original code less clear, because it's a two-step comparison rather than just saying "if it's exactly false, then..." – Flambino Dec 3 '12 at 1:41
Also might be useful pass current index into callback: index++; var newInterval = callback(collection[index], index); – Victor Dec 3 '12 at 18:52
up vote 3 down vote

My experience with Javascript isn't quite as developed as it is for other languages, but here's what I have to say about the code.

Why use setInterval() and clearInterval()? You'd use those methods if you wanted to call a function repeatedly. Since all you do is set it and immediately clear it, there's no point in really using it. Better to use setTimeout() instead.

I'd consider rearranging your parameters so the delay was last. That way you can make the delay optional, possibly provide a default delay instead. Also consider making complatedCallback optional too. It could be possible that you don't need the callback so you're forcing yourself to have to provide one.

I don't think you'd want to put a delay of the first invocation on the first item. So probably should remove that initial timeout and just call the executor() directly.

DelayedLoop.forEach = function(collection, callback, completedCallback, delay) {
    var index = 0;

    var executor = function() {
        // Executes the callback, and gets the returned value to indicate what the next delay will be.
        var newInterval = callback(collection[index++]);

        // If they returned false, quit looping.
        if (typeof(newInterval) == "boolean" && newInterval == false) {
            return;
        }

        // If nothing / non-number is provided, re-use initial delay.
        if (typeof(newInterval) != "number") {
            newInterval = delay;
        } else if (newInterval < 0) { // If a negative number is returned, quit looping.
            return;
        }

        // If there are more elements to iterate, re-set delayed loop. Otherwise, call the "completed" callback.
        if (index < collection.length) {
            setTimeout(executor, newInterval);
        } else if (typeof(completedCallback) == "function") {
            completedCallback();
        }
    };

    // Initial loop setup.
    executor();
};

Otherwise I don't see anything glaringly wrong with the code.

share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.