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>

Answer 1

1. It's more idiomatic to use [] rather than new Array()

2. 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.

3. 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++;
   

4. 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
   }
   

5. Variable names prefixed with $ are somewhat unusual.

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

• Andrew: Thank you. That was very helpful! About #4, why is it preferred to use string literals for keys?
• Peter Taylor: @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.
• andykellr: re: #5, it's very common to prefix variables with a $ when they reference jquery objects. See here: stackoverflow.com/questions/205853/…
• Peter Taylor: @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.

Answer 2

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/

• Andrew: 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.