Responsive image slider

Question

I have been doing some Code School classes on jQuery, and I needed a image slider for my site. Right now, there are only two responsive states, but that will most likely change (and/or become fluid later).

I was just wondering if there are any best practices or general clean up I can do to the code. I'm still a newbie with jQuery, so any advice would be helpful.

Plugins used: Fastclick and hammer.js

$(function() {
FastClick.attach(document.body);
var slide = $(".ImgSection li");
var ImgSec = $(".ImgSection").hammer();
var CrntPos = 0;
var Width;
var Time;
var PlusPos;
$(window).on("load resize", function(e) {
 Width = slide.outerWidth(true);
 });
ImgSec.on("dragleft dragright", "li", function(ev) {
 ev.gesture.preventDefault();
});
function changeImg (e){
 CrntPos = $(this).index();
var ClkWth = Width * .1;
var NewPos = (CrntPos * Width) - ClkWth;
 slide.css("transform", "translate3d(-"+ NewPos +"px,0,0)");
 if (CrntPos === 1 ){
 $("li:eq(0)").on("click", function() {
 slide.css("transform", "translate3d(0,0,0)");
 }); 
 }
} 
slide.click(changeImg);
ImgSec.on("swipe", "li", function(ev) {
 if(ev.gesture.direction == 'left'){ 
 slide.eq(CrntPos + 1).trigger("click");
 }
 if(ev.gesture.direction == 'right'){ 
 slide.eq(CrntPos - 1).trigger("click");
 }
 if($(this).is(':last-child') && ev.gesture.direction == 'left'){ 
 slide.eq(0).trigger("click");
 }
});
 $(window).resize(function() {
 clearTimeout(Time);
 Time = setTimeout(Resize, 100);
 });
function Resize(){
 if ($('img', slide).css('max-width') == '245px' ){
 slide.eq(CrntPos).trigger('click');
 } 
 else {
 slide.eq(CrntPos).trigger('click');
 }
}
 });

Answer 1

• For variable and function names, use camelCase. For constants, or at least variables that are used as constants, use ALLCAPS_AND_UNDERSCORES. For constructor functions, use TitleCaps (I forgot how they called this). This is a convention which is common among programming languages and you should follow this to avoid misunderstandings.

• Never forget semi-colons, even when JS inserts them for you at certain cases.

• As much as possible, use strict comparison (===,!== etc.)

• Use a tool to check for code quality. Most of the bad practices will be weeded-out by using such tools. You can use JSHint to check your syntax.

As for code:

$(function () {
 //A handy tip in keeping things clean is that you can use comma-separated
 //variable declarations. It's best you use this style when declaring
 //variables without assigning anything.
 var width, time, plusPos;
 //As for variable declaration with assignments, it's best they are var'ed
 //individually. Commas tend to get messy when assignments are done.
 var slide = $(".imgSection li");
 var imgSec = $(".imgSection").hammer();
 var crntPos = 0;
 function changeImg(e) {
 crntPos = $(this).index();
 var clkWth = width * .1;
 var newPos = (crntPos * width) - clkWth;
 slide.css("transform", "translate3d(-" + newPos + "px,0,0)");
 //I notice that changeImg is called every click of `slide`. This means
 //that if currentPos === 1, a handler gets attached? You might want to
 //review this one
 if(crntPos === 1) {
 $("li:eq(0)").on("click", function () {
 slide.css("transform", "translate3d(0,0,0)");
 });
 }
 }
 //I found that your resize function does the same thing regardless of
 //the condition. We'll remove the condition instead.
 function resize() {
 slide.eq(crntPos).trigger('click');
 }
 //.on() accepts a map of events and handlers. You can use this style to
 //bind events on an object in one place neatly. You also avoid wrapping
 //the same object more than once with jQuery. In this case, it's avoiding
 //the call of $(window) more than once.
 $(window).on({
 'load resize': function () {
 width = slide.outerWidth(true);
 },
 'resize': function () {
 clearTimeout(time);
 time = setTimeout(resize, 100);
 }
 });
 imgSec.on({
 'dragleft dragright': function (ev) {
 ev.gesture.preventDefault();
 },
 'swipe': function (ev) {
 var index;
 //You can cache repeatedly accessed values
 var direction = ev.gesture.direction;
 //As you can see, the only difference the conditions do is determine the
 //value for .eq(). You can DRY the code by extracting the common calls.
 if(direction === 'left') {
 //taking advantage that the direction in this block is already left
 //we merge in the last child check
 if($(this).is(':last-child')) {
 index = 0;
 } else {
 index = crntPos + 1;
 }
 } else if(direction === 'right') {
 //Use an else to avoid further execution of the condition. That way,
 //when direction is already 'left', it won't check if it is 'right'.
 index = crntPos - 1;
 }
 //Our factored-out call
 slide.eq(index).trigger('click');
 }
 }, 'li');
 FastClick.attach(document.body);
 slide.click(changeImg);
 });

• Flambino: @HaydenTarr I would add: Don't abbreviate everything. current is more readable than crnt (which I actually misread as "ctrl", when I first skimmed you code). Letters are cheap :)
• Hayden Tarr: @Flambino ahh thats really good to know, for some reason thought it's the sames rules as css :) thanks man!