6
\$\begingroup\$

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();
\$\endgroup\$
1
  • \$\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
    Commented Jan 2, 2014 at 14:47

2 Answers 2

4
\$\begingroup\$

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();
\$\endgroup\$
2
  • \$\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
    Commented 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
    Commented Jan 3, 2014 at 12:20
3
\$\begingroup\$

I like the idea of your code but you defenitely have some bugs and convention issues that should be dealt with:

  1. 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 as funcArr[name] is never emptied
  2. You're storing keys (function names) in the array funcArr. funcArr should be a hash object rather than an array.
  3. You're using for..in on an array (why this is bad) and moreover, the variable func 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()); 
    });
    
  4. 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 the this keyword if you're not planning to add these functions on prototype, 3) In this.start your line this.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 the new 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;
};

http://jsfiddle.net/g55PC/2/

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);
\$\endgroup\$
2
  • \$\begingroup\$ Welcome to CR, excellent review! I would have chosen a different name for ord. \$\endgroup\$
    – konijn
    Commented 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
    Commented Jan 3, 2014 at 9:20

Your Answer

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

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