Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

I created a Queue object when answering this question and I was wondering if it could do with some improvement.

Here is the current code:

var Queue = (function () {

    Queue.prototype.autorun = true;
    Queue.prototype.running = false;
    Queue.prototype.queue = [];

    function Queue(autorun) {
        if (typeof autorun !== "undefined") {
            this.autorun = autorun;
        }
        this.queue = []; //initialize the queue
    };

    Queue.prototype.add = function (callback) {
        var _this = this;
        //add callback to the queue
        this.queue.push(function () {
            var finished = callback();
            if (typeof finished === "undefined" || finished) {
                //  if callback returns `false`, then you have to 
                //  call `next` somewhere in the callback
                _this.dequeue();
            }
        });

        if (this.autorun && !this.running) {
            // if nothing is running, then start the engines!
            this.dequeue();
        }

        return this; // for chaining fun!
    };

    Queue.prototype.dequeue = function () {
        this.running = false;
        //get the first element off the queue
        var shift = this.queue.shift();
        if (shift) {
            this.running = true;
            shift();
        }
        return shift;
    };

    Queue.prototype.next = Queue.prototype.dequeue;

    return Queue;

})();

Is there anything that I can expand on or improve in this Object?
Should anything be changed?

share|improve this question

1 Answer 1

Overall, the code is quite simple. That's a good thing already.

Some things can be improved though:

  • Always create the autorun property. It will help your compiler. See here for more details. Besides, it's simpler to read. Just go with this:

    function Queue(autorun) {
        this.autorun = autorun;
    }
    

On another note, adding the properties on the prototype is just as bad, read the link I already showed to understand why.

  • Maybe it's only me, but the first lines are disturbing because they're before the "class" declaration. So it looks like they belong to the external object, while they actually belong to the hoisted function.
  • You don't need typeof variable === "undefined". variable === undefined is enough because you know for sure the identifier exists.
  • var shift = this.queue.shift(); w-w-w-what's this variable name?! You probably mean var first .... Use descriptive variable names.
  • I'm not sure you want to return shift in dequeue. Don't you mean this? Chaining is fun!
share|improve this answer
    
1 -- Well I want the option for a user not to pass in the autorun option (so by default it will be true). 2 -- And the variables on the prototype are hoisted to the function call within the scope -- At least I believe that they are. 3 -- Yes, My variable naming is terrible 4 -- CHAINING IS FUN 2.5 --- I find the speed of typeof vs the other route is minimal. –  Neal Jul 11 '13 at 20:14
    
@Neal 1. It's ok, the property will just be undefined. But at least the generated class (by the compiler) will be the same no matter how you instantiate the object. 2. They are. But it's just wrong on a readability level. Principle of least surprise. –  Florian Margaine Jul 11 '13 at 20:15
    
If the property is undefined then my check if autorun is true will fail in the add function so the queue will never autorun unless you explicitly tell it to. –  Neal Jul 11 '13 at 20:19
    
@Neal if you don't pass anything, it will be undefined. If you pass true, it will be true. So your add function will work just as well. Or is there something else I'm not getting? –  Florian Margaine Jul 11 '13 at 20:20
1  
OH I SEE ITS BECAUSE OF THE PROTOTYPE. Oh god, change this. See how much you had to say to convince me how the code actually works? You just had the best proof that your code is definitely not readable. –  Florian Margaine Jul 11 '13 at 20:32

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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