HiveBrain v1.2.0
Get Started
← Back to all entries
patternjavascriptMinor

Review my "delayed for-loop" in JavaScript

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
javascriptloopdelayedforreview

Problem

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?

Solution

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.

Code Snippets

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();
};

Context

StackExchange Code Review Q#19235, answer score: 3

Revisions (0)

No revisions yet.