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

Async Implementation

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

Problem

I'm seeking code review comments for the following implementation.

jsFiddle

/**
* You need to run a number of async tasks, but you're being rate limited.
* Run as many concurrent tasks as you can, queueing the rest for when the
* tasks are complete.
*/

/**
* @param concurrency: number of concurrent tasks that can be run
*/
var ConcurrencyQueue = function (concurrency) {
    this.concurrency = concurrency;
    this.queue = [];
    this.count = 0;
};

/**
* add an asynchronous task to the queue. it will get executed immediately
* until the concurrent task count reaches this.concurrency. then, queue it until
* tasks are complete.
* @param task: function which takes a callback argument (done)
*/
ConcurrencyQueue.prototype.enqueue = function (task) {
    var self = this;
    self.count = self.count + 1;

    var done = function () {
        self.count = self.count - 1;
        if (self.queue.length) {
            self.queue.splice(0, 1)[0](done);
        }
    };

    if (self.count > self.concurrency) {
        self.queue.push(task);
    }

    if (self.count <= self.concurrency) {
        task(done);
    }
};

/**
 * tasks passed to concurrency queue are async and call the callback "done"
 * after they complete.
 */

function exampleTask (done) {
    console.log('started');
    setTimeout(function () {
        console.log('completed');
        done();
    }, Math.floor(Math.random() * 2000));
};

var queue = new ConcurrencyQueue(2);
queue.enqueue(exampleTask);
queue.enqueue(exampleTask);
queue.enqueue(exampleTask);
queue.enqueue(exampleTask);
queue.enqueue(exampleTask);
queue.enqueue(exampleTask);
queue.enqueue(exampleTask);

Solution

I have very little to say, maybe others will have more.

if (self.count > self.concurrency) {
    self.queue.push(task);
}

if (self.count <= self.concurrency) {
    task(done);
}


Why not if / else? At least it'll be clear it's one or the other.

this.queue.shift() is cleaner than this.queue.splice(0, 1)[0].

There's also a nitpick about self.count = self.count + 1; where += 1 could be used instead, but it really depends on the coding style.

About the callbacks, you could move to the newer Promise which is part of ECMAScript 2015.

ConcurrencyQueue.prototype.enqueue = function (task) {
    this.count += 1;

    if (this.count > this.concurrency) {
        this.queue.push(task);
    } else {
        this.execute(task);
    }
};

ConcurrencyQueue.prototype.execute = function(task) {
    var p = new Promise(task);

    /* `() => {` is a simpler notation for `function() {` in recent JS,
        but `this` refers to the encompassing `this` */
    var done = () => {
        this.count -= 1;
        if (this.queue.length) {
            this.execute(this.queue.shift());
        }
    };

    /* First argument is in case of success, second in case of errors */
    p.then(done, done);
}


jsFiddle

Code Snippets

if (self.count > self.concurrency) {
    self.queue.push(task);
}

if (self.count <= self.concurrency) {
    task(done);
}
ConcurrencyQueue.prototype.enqueue = function (task) {
    this.count += 1;

    if (this.count > this.concurrency) {
        this.queue.push(task);
    } else {
        this.execute(task);
    }
};

ConcurrencyQueue.prototype.execute = function(task) {
    var p = new Promise(task);

    /* `() => {` is a simpler notation for `function() {` in recent JS,
        but `this` refers to the encompassing `this` */
    var done = () => {
        this.count -= 1;
        if (this.queue.length) {
            this.execute(this.queue.shift());
        }
    };

    /* First argument is in case of success, second in case of errors */
    p.then(done, done);
}

Context

StackExchange Code Review Q#129894, answer score: 2

Revisions (0)

No revisions yet.