Tell me more ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

I was hoping someone out there could help me make my 'carousel' slideshow more efficient. I have it functioning, but I'm using very primitive methods for accomplishing this. I also have it in a 'loop' by using setInterval, but the click() actions don't occur, because when I tried using clearInterval(), the function broke.

The JQuery -

$(document).ready(function() {

var interval = setInterval(function() { 
$('.slide1').animate({ 'left' : '0px' }).addClass('current');
$('.slide2').animate({ 'left' : '769px' }).removeClass('current');
$('.slide3').animate({ 'left' : '1538px' }).removeClass('current');
$('.slide1').delay(3000).animate({ 'left' : '-769px' }).removeClass('current');
$('.slide2').delay(3000).animate({ 'left' : '0px' }).addClass('current');
$('.slide3').delay(3000).animate({ 'left' : '769px' }).removeClass('current');
$('.slide1').delay(3000).animate({ 'left' : '-1538px' }).removeClass('current');
$('.slide2').delay(3000).animate({ 'left' : '-769px' }).removeClass('current');
$('.slide3').delay(3000).animate({ 'left' : '0px' }).addClass('current');

}, 9000);

$('#slide1').click(function() {
    clearInterval(interval);
    $('.slide2').animate({ 'left' : '769px' }).removeClass('current');
    $('.slide3').animate({ 'left' : '1538px' }).removeClass('current');
    $('.slide1').animate({ 'left' : '0' });
    $('.slide1').addClass('current');
});
$('#slide2').click(function() {
    clearInterval(interval);
    $('.slide1').animate({ 'left' : '-769px' }).removeClass('current');
    $('.slide3').animate({ 'left' : '769px' }).removeClass('current');
    $('.slide2').animate({ 'left' : '0' });
    $('.slide2').addClass('current');
});
$('#slide3').click(function() {
    clearInterval(interval);
    $('.slide1').animate({ 'left' : '-1538px' }).removeClass('current');
    $('.slide2').animate({ 'left' : '-769px' }).removeClass('current');
    $('.slide3').animate({ 'left' : '0' });
    $('.slide3').addClass('current');
});
});

My fiddle - http://jsfiddle.net/HCEmw/1/

I'm thinking there should be a function which takes the current class and determines it's siblings, their order, etc, and then performs an animation on that. Still unsure about how to have it loop AND animate on click. Many thanks SO.

share|improve this question
1  
One key element to efficient code is code-reuse...your particular code would be a hellstorm of maintenance when you consider the manual additions and subtractions to get each animation's left value. – hexparrot Oct 23 '12 at 15:41
1  
Also, clearInterval takes an argument--which should reflect the value that the setInterval returns--a lot of this must be rewritten and refactored...you'll need to attack this problem from scratch, I'd venture. – hexparrot Oct 23 '12 at 15:43
These are fair criticisms. I really wrote the code this way because I'm unfamiliar with the appropriate way, that being how can I use setInterval() correctly so I can clear it on an event, etc. – RCNeil Oct 23 '12 at 15:50

migrated from stackoverflow.com Oct 23 '12 at 22:21

2 Answers

For one, you can start by not hardcoding integers for the slides; instead use variables.

var currentSlide = "slide-1";
var nextSlide = parseInt((currentSlide.split("-"))[1]) + 1
var prevSlide = parseInt((currentSlide.split("-"))[1]) - 1

Then you go ahead and check if nextSlide/prevSlide are either first/last. etc. etc. etc.

share|improve this answer

For a generic version with any number of slides you can change go with:

var width = $('.container').outerWidth(),
    $btn = $('.btn'),
    timeout;

$('.slide').each(function(slideNr) {
    $(this).css({left: (slideNr)*width});
});

$btn.each(function(btnNr) {
    $(this).click(function() {        
        $('.slide').each(function(slideNr) {
            $(this).animate({left: (slideNr-btnNr)*width})
                .toggleClass('current', btnNr != slideNr);
        });
        clearTimeout(timeout);
        timeout = setTimeout(function(){
            $btn.eq((btnNr+1) % $btn.length).click();
        }, 3000);
    });
}).first().click();

​ See as fiddle: http://jsfiddle.net/creativecouple/Bvey3/

This will continue the timeout from the clicked slide.

share|improve this answer
You might want to explain what you've done and why you are suggesting the edits to make this answer more valuable. – Jeff Vanzella Oct 24 '12 at 15:33

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.