2
\$\begingroup\$

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');
    }
}
  });
\$\endgroup\$
1
\$\begingroup\$
  • 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);

  });
\$\endgroup\$
  • 1
    \$\begingroup\$ @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 :) \$\endgroup\$ – Flambino Jun 1 '13 at 13:00
  • \$\begingroup\$ @Flambino ahh thats really good to know, for some reason thought it's the sames rules as css :) thanks man! \$\endgroup\$ – Hayden Tarr Jun 1 '13 at 19:31

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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