Skip to main content
replaced http://stackoverflow.com/ with https://stackoverflow.com/
Source Link
  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 badwhy 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.

  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.

  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.

Use $.each
Source Link
megawac
  • 2.3k
  • 1
  • 15
  • 26
  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

    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.

    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.

    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.

    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.

    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.
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!]
                for(var i = 0; i < funcArr.length; i++){//you can use .forEach or $.each nicely here
                    promiseArr$.push(each(funcArr, function(i, fn) {
                        var dfd = new jQuery.Deferred();
                        funcArr[i]fn(dfd);
                        return 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;
};
  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.
  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.
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!]
                for(var i = 0; i < funcArr.length; i++){//you can use .forEach or $.each nicely here
                    promiseArr.push((function(){
                        var dfd = new jQuery.Deferred();
                        funcArr[i](dfd);
                        return 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;
};
  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.

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;
};
added 560 characters in body
Source Link
megawac
  • 2.3k
  • 1
  • 15
  • 26

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

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);
added 161 characters in body
Source Link
megawac
  • 2.3k
  • 1
  • 15
  • 26
Loading
Source Link
megawac
  • 2.3k
  • 1
  • 15
  • 26
Loading