I've created this code which can call functions/ajax requests or pieces of code sequentially, using jQuery. The code not only can call functions/ajax requests or pieces of code sequentially but it also tracks when those calls fail or succeed and when the code fails it stops and doesn't exectue the rest of the queue.
(Bold italic are the names of the queues)
In my example i've 5 timeouts who get queued, the 2 first share a queue named first (1 and 1b) get executed at the same time and when those 2 finish and succeed(fire resolve) the next on the queue gets executed (next in queue is two which contains only 1 timeout).
After two gets executed and succeed the third gets executed. I've coded in a way that the third fires a reject which changes the state of promise to reject(fails the request) and thanks to that the last timeout doesn't get called(to simulate a failed ajax request).
I know the code is pretty messy (it's my first version). I would like some ideas on how I could structure this better or reduce the code considerably.
A problem I have with my code is that when I add a setTimeout
, I have to include the deferred.resolve()
in it. I don't see a way around this. Does anyone have an idea how to resolve this problem?
Code: http://jsfiddle.net/g55PC/
function myqueue(){
var myself=this;
var ord=[];// contains names used in promiseArr
var funcArr=[];// contains setTimeout functions
var me = $(document);
this.add = function (func, name) {
if (typeof funcArr[name] !== "object") funcArr[name] = [];
funcArr[name].push(func);
if($.inArray(name, ord)==-1){
ord.unshift(name);
}
}
this.call= function (name) {
me.queue("deferQueue", function () {
var promiseArr=[];
for(func in funcArr[name]){
promiseArr[func]=(function(){
var dfd = new jQuery.Deferred();
funcArr[name][func](dfd);
return dfd.promise();
})();
}
$.when.apply(,ドルpromiseArr).then(function () {
console.log("Success "+name);
me.dequeue("deferQueue");
}, function () {
console.log("Fail "+name);
});
});
}
this.start = function () {
while(ord.length>0) {
this.call(ord.pop());
}
me.dequeue("deferQueue");
}
};
myPlugin = new myqueue();
myPlugin.add(function (dfd) {
setTimeout(function () {
$("#result").append("<div>1</div>");
//console.log("1");
dfd.resolve();
}, 2000);
}, "first");//first = queue Name
myPlugin.add(function (dfd) {
setTimeout(function () {
$("#result").append("<div>1b</div>");
//console.log("1b");
dfd.resolve();
}, 1000);
}, "first");//first = queue Name
myPlugin.add(function (dfd) {
setTimeout(function () {
$("#result").append("<div>2</div>");
//console.log("2");
dfd.resolve();
}, 1000);
}, "second");//second = queue Name
myPlugin.add(function (dfd) {
setTimeout(function () {
$("#result").append("<div>3</div>");
//console.log("3");
dfd.reject();
}, 500);
}, "third");//third = queue Name
//forth will not be fired since third failed
myPlugin.add(function (dfd) {
setTimeout(function () {
$("#result").append("<div>4</div>");
//console.log("4");
dfd.resolve();
}, 3000);
}, "forth");//forth = queue Name
myPlugin.start();
-
\$\begingroup\$ If you could be kind enough to summarize what the queue does, and how it works. Because improving the code is one thing. Providing you a better solution is another. \$\endgroup\$Joseph– Joseph2014年01月02日 14:47:51 +00:00Commented Jan 2, 2014 at 14:47
2 Answers 2
Naming
First off, I'd name my variables in the purpose they serve. Rather than ord
, funcHash
or even me
.
Queue adding and removing
You should remove the items that have executed from the queue. That's what a queue is all about, and that's what your code lacks at the moment.
Also, unshift
and shift
are more costly operations than 'pop' and 'push' because they need to move the array contents to new indices. You're correct in your implementation, that enqueue
should use the unshift
while execution uses pop
.
Collection of callbacks
You don't need 2 arrays to store the order of named queues. You can have one array, but each item is named. One advantage of this is that you only have one array to push to, and you keep the absolute order, regardless of name. And no more nested loops.
function enqueue(fn,name){
queue.push({
name : name,
fn : fn
})
}
You can then use Array.prototype.filter
to filter out the queue, if you want to only unload certain queued items with a certain name. Here's how you can do it:
function dequeue(name){
var fns = queue.filter(function(current){
return (current.name === name);
});
// pop-off and run
}
This code defaults to all items when name
is not supplied
function dequeueAll(name){
var fns = (!name) ? queue : queue.filter(function(current){
return (current.name === name);
});
// pop-off and run all
}
Async control
Now, you don't have total control over the process that takes place in the queued function. The user could pop-in an async operation, and your library won't even know it, and will run wild. That's where your deferreds come in to play.
However, you don't really need deferreds. You can pass in a function to the queued function that calls the next function. This is similar to ExpressJS's middlewares, where there's a magical next
function supplied to each middleware. Here's how it's implemented
// Lets assume you have a queue like the one above, filtered optionally
(function recursive(index){
// get the item at the index
var currentItem = queue[index];
// This is to check if there's no more functions in the queue
if(!currentItem) return;
//Otherwise, run providing the instance of your object, and the magic "next"
currentItem.fn.call(instance,function(){
//This function gets called when the current queued item is done executing
// Splice off this item from the queue. We use splice since the
// item we might be operating on is from a filtered set, not in the same
// index as the original queue
queue.splice(queue.indexOf(currentItem),1);
// Run the next item
recursive(++index);
});
// start with 0
}(0));
Here's how to use it:
// So you can have something synchronous
enqueue(function(next){
//do something
next();
});
// Or something asynchronous
enqueue(function(next){
somethingAsync(function(){
// done?
next();
});
});
Fluent API
Consider fluent, aka "jQuery-like" APIs. It's pretty simple, just have the methods return the current instance, and that's it:
enqueue : function(){
...
return this;
}
Then you can do something like jQuery and other libraries:
myQueue.enqueue(function(){...}).enqueue(function(){...}).enqueue(function(){...})
Demonstration
Here's a small demo I have made here. It has some bugs in some corner cases and needs a few fixing. But given your code example, it works quite similarly. Not to mention, it is a bit shorter, and uses no libraries.
function Queue() {
this.queue = []
}
Queue.prototype = {
constructor: Queue,
enqueue: function (fn, queueName) {
this.queue.push({
name: queueName || 'global',
fn: fn || function (next) {
next()
}
});
return this
},
dequeue: function (queueName) {
var allFns = (!queueName) ? this.queue : this.queue.filter(function (current) {
return (current.name === queueName)
});
var poppedFn = allFns.pop();
if (poppedFn) poppedFn.fn.call(this);
return this
},
dequeueAll: function (queueName) {
var instance = this;
var queue = this.queue;
var allFns = (!queueName) ? this.queue : this.queue.filter(function (current) {
return (current.name === queueName)
});
(function recursive(index) {
var currentItem = allFns[index];
if (!currentItem) return;
currentItem.fn.call(instance, function () {
queue.splice(queue.indexOf(currentItem), 1);
recursive(index)
})
}(0));
return this
}
};
var myQueue = new Queue();
myQueue.enqueue(function (next) {
console.log('D1: first test1');
next()
}, 'first').enqueue(function (next) {
setTimeout(function () {
console.log('D1: first test2');
next()
}, 2000)
}, 'first').enqueue(function (next) {
console.log('D1: second test1');
next()
}, 'second').enqueue(function (next) {
console.log('D1: second test2');
next()
}, 'second').dequeueAll();
-
\$\begingroup\$ I like your code but the thing about your demonstration is that i'ts a completely different thing than mine. My code is able to queue functions/ajax requests or pieces of codes and execute them sequentially. And the thing about it is that some pieces of code can fail or succeed. If one piece/function fails the code ends. I like your code and i will try to take some of your ideas and improve my code thx to you. \$\endgroup\$Marcio– Marcio2014年01月03日 10:00:35 +00:00Commented Jan 3, 2014 at 10:00
-
\$\begingroup\$ @Marcio Actually, if the queued code fails to call
next()
, then the next queued code doesn't run. \$\endgroup\$Joseph– Joseph2014年01月03日 12:20:46 +00:00Commented Jan 3, 2014 at 12:20
I like the idea of your code but you defenitely have some bugs and convention issues that should be dealt with:
- Major bug: call does not remove
funcArr[name]
after iterating them so if you add something to the "first" queue and call the first queue, adding something to the first queue will still have the functions from before you called the queue earlier. I created a (demo of the issue); as you see the alert will be shown twice asfuncArr[name]
is never emptied - You're storing keys (function names) in the array
funcArr
.funcArr
should be a hash object rather than an array. You're using
for..in
on an array (why this is bad) and moreover, the variablefunc
will be global. Use a regular for loop instead here. I'd recommend using$.each
here. With$.each
you'd write the loop$.each(funcArr, function(i, fn) { var dfd = $.Deferred(); fn(dfd); promiseArr.push(dfd.promise()); });
You're module pattern seems off... 1) its common convention that a class name should be capitalized, ie
MyQueue
, 2) theres no real reason to use thethis
keyword if you're not planning to add these functions on prototype, 3) Inthis.start
your linethis.call(ord.pop());
is a bit confusing.
As you were not making use of prototype in your function I adapted your code to use the "module pattern". This allows your module to be created without thenew
keyword whereas before it would of caused major issues.
Here's how I would write your queue. I added comments on most of my cahnges
function myqueue(){
var ord=[];
var funcHash = {}; // Use a hash as you're using keys rather than indicies
var me = $(document);//(minor)convention would be to name this $doc
var plugin = {
add: function (func, name) {
if(!funcHash.hasOwnProperty(name)) {//name does not exist yet
funcHash[name] = [];
ord.unshift(name);
}
funcHash[name].push(func);//now just queue the function
},
call: function (name) {
me.queue("deferQueue", function () {
var promiseArr=[];
var funcArr = funcHash[name];
//funcArr is a hash of arrays don't use for in
//also func was a global variable!]
$.each(funcArr, function(i, fn) {
var dfd = new jQuery.Deferred();
fn(dfd);
promiseArr.push(dfd.promise());
});
//I assume you want to remove the functions after you call them so requeing the same name doesn't add to the old queue
delete funcHash[name];
$.when.apply(,ドルpromiseArr).then(function () {
console.log("Success "+name);
me.dequeue("deferQueue");
}, function () {
console.log("Fail "+name);
});
});
},
start: function () {
while(ord.length>0) {
plugin.call(ord.pop());
}
me.dequeue("deferQueue");
}
};
return plugin;
};
As for the second question of having to do the .resolve()
in the timeout, you may want to consider adding a helper to your myqueue
module along the lines of
//helper function for automatically managing a delay using timeout
addDelayed: function(fn, name, delay) {
this.add(function(dfd) {
setTimeout(function () {
fn(dfd);
dfd.resolve();
}, delay || 250);
}, name);
}
So your old pattern code be reduced to just:
myPlugin.addDelayed(function (dfd) {
$("#result").append("<div>1</div>");
}, "first", 1000);
-
\$\begingroup\$ Welcome to CR, excellent review! I would have chosen a different name for
ord
. \$\endgroup\$konijn– konijn2014年01月02日 18:06:23 +00:00Commented Jan 2, 2014 at 18:06 -
\$\begingroup\$ Thx for your help. Some names came from past codes so at that point some variables names don't make much sense. \$\endgroup\$Marcio– Marcio2014年01月03日 09:20:50 +00:00Commented Jan 3, 2014 at 9:20