Trying to learn idiomatic JavaScript and jQuery

Question

I have been doing server-side development for a couple of years, but I am just now getting into doing some client-side programming. As a JavaScript exercise, I created a dead-simple jQuery image slider plugin, but I don't know if I did it the "right" way.

Specifically, I was wondering what improvements ought to be made to make the code clearer and more self-documenting. Also, are their any places where I have "re-invented the wheel" where I could have used the JS (or jQuery) standard library? Feel free to point out any glaring deficiencies, egregious oversights, or logical aberrations.

(function ($) {
    $.fn.sliderize = function( options ) {
        var settings = $.extend({ 
            srcAttrib: "src", // data attribute containing image source
            delayTime: 6000,
            transitionTime: 1000,
            randomize: false, // "randomize" the slides
            width: 700,
            height: 276
        }, options);

        // If we don't do this, then the plugin can throw the browser into an infinite loop :-o
        if (this.length === 0 || this.find('img').length === 0) {
            return new Array();
        };

        var images = new Array(),
        statePlaying = true,
        currentIndex = 0;

        enqueue = function($image) {
            images.push($image);
        }

        nextImg = function() {
            // Check to see if random setting is on
            if (settings.randomize) {
                // Selects a "random" index... ensures that the same slide does not display 2x in a row
                while(true) {
                    candidateIndex = Math.floor(Math.random() * (images.length - 1));
                    if (candidateIndex !== currentIndex) {
                        currentIndex = candidateIndex;
                        break;
                    }
                }
            } else if (currentIndex === images.length - 1) {
                // If we're at the end, then get the first image again
                currentIndex = 0;
            } else {
                // Otherwise, increment the index
                currentIndex++;
            }

            // Implement a crude form of preloading, loading 1 image in advance
            if (images[currentIndex].data('loaded') !== true) {
                theSrc = images[currentIndex].data(settings.srcAttrib);
                images[currentIndex].attr('src', theSrc)
                    .data('loaded', true)
                    .css({
                        position: "absolute",
                        top: 0,
                        left: 0,
                        margin: 0
                    });
            }

            return images[currentIndex];
        }

        playShow = function($img) {
            if (statePlaying === false) return;

            $img.fadeIn(settings.transitionTime / 2, function() {
                $nextImg = nextImg();
                setTimeout(function() {
                    $img.fadeOut(settings.transitionTime / 2, function() {
                        playShow($nextImg);
                    });
                }, settings.delayTime - settings.transitionTime);
            });
        };

        // LOOP THROUGH IMAGES AND ADD THEM TO THE QUEUE
        this.find('img').each(function() {
            enqueue($(this));
            $(this).hide();
        });

        // Ensure that the container element is position relatively
        this.css({
            width: settings.width,
            height: settings.height,
            position: 'relative'
        }).addClass('loading');

        currentIndex = -1;
        setTimeout(playShow(nextImg()), 0);

        // Maintain chainability
        return this;
    };
})(jQuery);

And here is an example of the usage:

<div id="slider-container">
    <img src="data:image/gif;base64,R0lGODlhAQABAAAAACH5BAEKAAEALAAAAAABAAEAAAICTAEAOw=="
         data-src="img/1.jpg" />
    <!-- ...More images -->
</div>
<!-- ...cut... -->
<script type="text/javascript">
    jQuery(document).ready(function() {
        $('#slider-container').sliderize({randomize: true});
    });
</script>

Peter Taylor · Accepted Answer · 2012-09-28 07:02:14Z

It's more idiomatic to use [] rather than new Array()
You should use var with your functions too. E.g.
```
var enqueue = function($image) {
    images.push($image);
}
```
Otherwise you're defining them in the global scope. Similarly candidateIndex, theSrc, ... JSLint is your friend.

candidateIndex = Math.floor(Math.random() * (images.length - 1));

has an out-by-one bug: it cannot select the last image. Of course, if you want to be clever you can replace the (fixed)

while(true) {
    candidateIndex = Math.floor(Math.random() * (images.length));
    if (candidateIndex !== currentIndex) {
        currentIndex = candidateIndex;
        break;
    }
}

with

candidateIndex = Math.floor(Math.random() * (images.length - 1));
if (candidateIndex >= currentIndex) candidateIndex++;

Although not many people do it, it's best practice to use string literals for object keys. E.g. instead of

{
    position: "absolute",
    top: 0,
    left: 0,
    margin: 0
}

you would have

{
    "position": "absolute",
    "top": 0,
    "left": 0,
    "margin": 0
}

Variable names prefixed with $ are somewhat unusual.
In defining the plugin you used the name avoidance strategy, but when using it you don't. The reason for wrapping the plugin definition in
```
(function($){
    ...
})(jQuery);
```
is that some other library may have defined $. It would be advisable to use the same boilerplate when invoking the plugin:
```
(function($){
    $(document).ready(function() {
        $('#slider-container').sliderize({randomize: true});
    });
})(jQuery);
```

Thank you. That was very helpful! About #4, why is it preferred to use string literals for keys?
@Andrew, because if your keys are language keywords, you have to use string literals; if you always use string literals you don't have to remember which are the special cases.
re: #5, it's very common to prefix variables with a $ when they reference jquery objects. See here: stackoverflow.com/questions/205853/…
@pulazzo, thanks for the link. I'm not a big fan of Hungarian notation, but I suppose it has its place in dynamically typed languages.

Larry Battle · Answer 2 · 2012-09-29 18:52:46Z

Here are some tips:

1) Fail as soon as possible.

Place your if guard at the top of the function.

Old Code:

$.fn.sliderize = function (options) {
    ... code
    if (this.length === 0 || this.find('img').length === 0) {
        return [];
    };
    ... more code

New Code:

$.fn.sliderize = function (options) {
    if (this.length === 0 || this.find('img').length === 0) {
        return [];
    };
    ... more code

2) Use truthy and falsely

Boolean(undefined); // => false
Boolean(null); // => false
Boolean(false); // => false
Boolean(0); // => false
Boolean(""); // => false
Boolean(NaN); // => false

Boolean(1); // => true
Boolean([1,2,3]); // => true
Boolean(function(){}); // => true

Take from http://james.padolsey.com/javascript/truthy-falsey/

Old Code:

if (images[currentIndex].data('loaded') === false ) {

New Code:

if (!images[currentIndex].data('loaded')) {

3) Use `jQuery.delay()` instead of `window.setTimeout()`.

Doc for jQuery.delay()

Old Code:

$img.fadeIn(settings.transitionTime / 2, function () {
    $nextImg = nextImg();
    setTimeout(function () {
        $img.fadeOut(settings.transitionTime / 2, function () {
            playShow($nextImg);
        });
    }, settings.delayTime - settings.transitionTime);
});

New Code:

var fadeDelay = settings.transitionTime / 2;
$img.fadeIn(fadeDelay, function () {
    $nextImg = nextImg();
})
.delay(settings.delayTime - settings.transitionTime)
.fadeOut(fadeDelay, function () {
    playShow($nextImg);
});

4) `window.setTimeout()` requires a function.

The function passed to .setTimeout() auto starts, which doesn't make any sense.

Old Code:

setTimeout(playShow(nextImg()), 0);

New Code:

playShow(nextImg());

5) `candidateIndex` and `theSrc` are global. Make sure to define them.

6) Only set `currentIndex` once.

Old Code:

var currentIndex = 0;
...
currentIndex = -1;

New Code:

var currentIndex = -1;

7) Have a function that returns the current image wrapped inside a jQuery.

This way if you have a bug in your program you can fail silently instead of crashing the javascript environment.

Code:

Sliderize.prototype.getCurrentImage = function () {
    return $(this.images[this.currentIndex]);
};

8) `settings.randomize` should always be false if there is only one image.

Otherwise there will be an infinite loop when trying to get a random index.

9) Don't crame everything inside `$.fn.sliderize`.

The reasons is because each call to .sliderize() will recreate everything inside that function. And plus functions longer than 10 lines cause confusion. You could make an object to store all that functionality. Refer to the final code as a reference. Example:

$.fn.sliderize = function (options) {
    if ($(this).find('img').length) {
        var obj = new Sliderize(options);
        obj.attachTo($(this));
        obj.playShow();
    }
    return this;
};

10) Return `this` in jQuery plugins.

Old Code:

if (!this.length || !this.find('img').length) {
    return [];
}

New Code:

if( !$(this).find("img").length ){
    return this;
}

11) Make complex functions testable.

nextImg is too long and hard to understand. Break this up into smaller functions.

Here's one part you can break up. Old Code:

if (settings.randomize) {
    // Selects a "random" index... ensures that the same slide does not display 2x in a row
    while(true) {
        candidateIndex = Math.floor(Math.random() * (images.length - 1));
        if (candidateIndex !== currentIndex) {
            currentIndex = candidateIndex;
            break;
        }
    }
} else if (currentIndex === images.length - 1) {
    // If we're at the end, then get the first image again
    currentIndex = 0;
} else {
    // Otherwise, increment the index
    currentIndex++;
}

New Code:

Sliderize.getNextIndex = function (i, len, isRandom) {
    if (isRandom && 1 < len) {
        var oldI = i;
        while (i === oldI) {
            i = Math.floor(Math.random() * len);
        }
    } else {
        i++;
    }
    return (len <= i) ? 0 : i;
};
Sliderize.prototype.updateIndex = function () {
    this.currentIndex = Sliderize.getNextIndex(this.currentIndex, this.images.length, this.settings.randomize);
};

Sample Testcase for Sliderize.getNextIndex(). If you're really serious about testing then use qUnit or another javascript testing framework.

var fn = Sliderize.getNextIndex;
console.log( "test `Sliderize.getNextIndex` without randomness" );
console.log( fn(0,5, false) === 1 );
console.log( fn(1,5, false) === 2 );
console.log( fn(4,5, false) === 0 );

console.log( "test `Sliderize.getNextIndex` with randomness" );
console.log( fn(0,5, true) !== 0 );
console.log( fn(1,5, true) !== 1 );
console.log( fn(4,5, true) !== 4 );

console.log( "test `Sliderize.getNextIndex` at odd conditions" );
console.log( fn(5,5, false) === 0 );
console.log( fn(0,1, true) === 0 );
console.log( fn(0,1, false) === 0 );

12) Delete `enqueue` since it's too simple and doesn't seem useful.

Old Code:

enqueue = function ($image) {
    images.push($image);
}
...
// LOOP THROUGH IMAGES AND ADD THEM TO THE QUEUE
this.find('img').each(function () {
    enqueue($(this));
    $(this).hide();
});

New Code A:

// LOOP THROUGH IMAGES AND ADD THEM TO THE QUEUE
this.find('img').each(function () {
    images.push($(this));
    $(this).hide();
});

New Code A can be further simplified by remembing that jQuery methods operate on a collection of jQuery objects and that collections can be converted to an array.

New Code B:

images = images.concat( 
    this.find('img').hide().toArray() 
);

Final Code:

(function ($) {
    var Sliderize = function (options) {
        this.images = [];
        this.statePlaying = true;
        this.currentIndex = 0;
        this.settings = $.extend({
                srcAttrib : "src",
                delayTime : 1000,
                transitionTime : 2000,
                randomize : false,
                width : 700,
                height : 276
            }, options);
    };
    Sliderize.getNextIndex = function (i, len, isRandom) {
        if (isRandom && 1 < len) {
            var oldI = i;
            while (i === oldI) {
                i = Math.floor(Math.random() * len);
            }
        } else {
            i++;
        }
        return (len <= i) ? 0 : i;
    };
    Sliderize.prototype.updateIndex = function () {
        this.currentIndex = Sliderize.getNextIndex(this.currentIndex, this.images.length, this.settings.randomize);
    };

    Sliderize.prototype.nextImg = function () {
        this.updateIndex();
        var currentImage = this.getCurrentImage();
        if (!currentImage.data('loaded')) {
            currentImage.attr('src', currentImage.data(this.settings.srcAttrib))
            .data('loaded', true).css({
                position : "absolute",
                top : 0,
                left : 0,
                margin : 0
            });
        }
    };
    Sliderize.prototype.getCurrentImage = function () {
        return $(this.images[this.currentIndex]);
    };
    Sliderize.prototype.playShow = function () {
        if (this.statePlaying) {
            var that = this,
            fadeDelay = this.settings.transitionTime / 2;
            this.getCurrentImage()
            .fadeIn(fadeDelay)
            .delay(this.settings.delayTime - this.settings.transitionTime)
            .fadeOut(fadeDelay, function () {
                that.nextImg();
                that.playShow();
            });
        }
    };
    Sliderize.prototype.addImagesToQueue = function ($imgs) {
        this.images = this.images.concat( 
            $imgs.hide().toArray() 
        );
    };
    Sliderize.prototype.changeElementToLoading = function ($el) {
        $el.css({
            width : this.settings.width,
            height : this.settings.height,
            position : 'relative'
        }).addClass('loading');
    };
    Sliderize.prototype.attachTo = function( $el ){
        this.addImagesToQueue($el.find('img'));
        this.changeElementToLoading($el);
    };
    $.fn.sliderize = function (options) {
        if ($(this).find('img').length) {
            var obj = new Sliderize(options);
            obj.attachTo( $(this) );
            obj.playShow();
        }
        return this;
    };
})(jQuery);

Demo: http://jsfiddle.net/Whtad/1/

Thank you so much! Your suggestions were extremely helpful! Having never used JavaScript as an OO language, thinking in terms of objects is not to natural for me in JS. It certainly makes for cleaner, more extensible code though.

asked	8 months ago
viewed	335 times
active	7 months ago

Trying to learn idiomatic JavaScript and jQuery

2 Answers

1) Fail as soon as possible.

2) Use truthy and falsely

3) Use `jQuery.delay()` instead of `window.setTimeout()`.

4) `window.setTimeout()` requires a function.

5) `candidateIndex` and `theSrc` are global. Make sure to define them.

6) Only set `currentIndex` once.

7) Have a function that returns the current image wrapped inside a jQuery.

8) `settings.randomize` should always be false if there is only one image.

9) Don't crame everything inside `$.fn.sliderize`.

10) Return `this` in jQuery plugins.

11) Make complex functions testable.

12) Delete `enqueue` since it's too simple and doesn't seem useful.

Final Code:

Your Answer

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

Trying to learn idiomatic JavaScript and jQuery

2 Answers

1) Fail as soon as possible.

2) Use truthy and falsely

3) Use jQuery.delay() instead of window.setTimeout().

4) window.setTimeout() requires a function.

5) candidateIndex and theSrc are global. Make sure to define them.

6) Only set currentIndex once.

7) Have a function that returns the current image wrapped inside a jQuery.

8) settings.randomize should always be false if there is only one image.

9) Don't crame everything inside $.fn.sliderize.

10) Return this in jQuery plugins.

11) Make complex functions testable.

12) Delete enqueue since it's too simple and doesn't seem useful.

Final Code:

Your Answer

Sign up or log in

Post as a guest

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

Related

3) Use `jQuery.delay()` instead of `window.setTimeout()`.

4) `window.setTimeout()` requires a function.

5) `candidateIndex` and `theSrc` are global. Make sure to define them.

6) Only set `currentIndex` once.

8) `settings.randomize` should always be false if there is only one image.

9) Don't crame everything inside `$.fn.sliderize`.

10) Return `this` in jQuery plugins.

12) Delete `enqueue` since it's too simple and doesn't seem useful.