A review of my jQuery Slider

Question

I have created a jQuery slider for my personal website and I wanted to get some feedback on how it looks and if there are things I can do to make it better.

You can see the live demo here: http://www.nickmeagher.com

And here is the code:

 var sliderWidth = 0,
 marginLeft = 0,
 containerWidth = 1040,
 slideSpeed = 600,
 slideDuration = 6000,
 autoSlide = 0; // (0) = off (1) = on
function slide_right() {
 if ( (sliderWidth - Math.abs(marginLeft)) > containerWidth ) {
 $('.portfolio-inner-wrapper').stop().animate({
 'margin-left': '+=-' + containerWidth
 }, slideSpeed, function() {
 marginLeft += -containerWidth;
 });
 } else {
 $('.portfolio-inner-wrapper').stop().animate({
 'margin-left': '0px'
 }, slideSpeed, function() {
 marginLeft = 0;
 });
 }
}
function slide_left() {
 if ( (sliderWidth - Math.abs(marginLeft)) < containerWidth ) {
 $('.portfolio-inner-wrapper').animate({
 'margin-left': '+=+' + containerWidth
 }, slideSpeed, function() {
 marginLeft += containerWidth;
 });
 } else {
 $('.portfolio-inner-wrapper').animate({
 'margin-left': '0'
 }, slideSpeed, function() {
 marginLeft = 0;
 });
 }
}
// Iterate through each container to calculate width
$('.work-container').each(function() {
 sliderWidth += $(this).outerWidth( true );
});
// Set width of inner wrapper
$('.portfolio-inner-wrapper').css('width', sliderWidth);
$('a.next').click(function() {
 slide_right();
});
$('a.prev').click(function() {
 slide_left();
});
if (autoSlide == 1) {
 setInterval(function() {
 slide_right();
 }, slideDuration);
}

Answer 1

Here are some general things you should be doing differently. These suggestions don't change the functionality of your slider, and are suggestions you can apply to your future projects.

First and most important

var wrapper = $('.portfolio-inner-wrapper');
//As a rule of thumb, if you use a selector more than once, you should cache it.
 //Now use 'wrapper' everywhere you used $('portfilo...') instead.
 //Cache it outside if you want to use it in other functions as well.
//Ex:
wrapper.css('width', sliderWidth);

Next:

if (autoSlide) { //Don't need == 1. Zero is a falsy value.
 setInterval(function() {
 slide_right();
 }, slideDuration); //You don't need setInterval either. jQuery has a built in queue.
}

When you have multiple 'animate' calls, jQuery puts them in a queue to execute them one by one. This goes for the rest of your slider as well.

Save function calls. Do it like this:

$('a.next').on('click', function() {
 if ( (sliderWidth - Math.abs(marginLeft)) > containerWidth ) {
 wrapper.stop().animate({
 'margin-left': '+=-' + containerWidth
 }, slideSpeed, function() {
 marginLeft += -containerWidth;
 });
 } else {
 wrapper.stop().animate({
 'margin-left': '0' //Zero doesn't need a unit of measure (ie. px, em, %, etc)
 }, slideSpeed, function() {
 marginLeft = 0;
 });
 }
});

When you use the .click() method, jQuery calls the .on() method. Then if you still call another function inside that, that's three function calls that could be spared if you simply used this:

$('a').on('click', function() {
 do.stuff
});

Finally, I'll be happy to answer any other questions you might have.

• Nick Meagher: Very good feedback! I didn't know jQuery had a built in queue instead of setInterval? What is it called?
• Jonny Sooter: The queue is built in and is automatically used with .animate(). If you want to alter the order or play with the queue in general use the .queue() method.