HiveBrain v1.2.0
Get Started
← Back to all entries
patternjavascriptMinor

Trying to learn idiomatic JavaScript and jQuery

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
javascripttryingidiomaticjquerylearnand

Problem

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

Solution

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 )

Code Snippets

$.fn.sliderize = function (options) {
    ... code
    if (this.length === 0 || this.find('img').length === 0) {
        return [];
    };
    ... more code
$.fn.sliderize = function (options) {
    if (this.length === 0 || this.find('img').length === 0) {
        return [];
    };
    ... more code
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
if (images[currentIndex].data('loaded') === false ) {
if (!images[currentIndex].data('loaded')) {

Context

StackExchange Code Review Q#15988, answer score: 8

Revisions (0)

No revisions yet.