1
\$\begingroup\$

Since I'm new to web development, I've decided to create this carousel, to practice a bit of JavaScript, more specifically jQuery. I'm really happy with the result, but I'm sure that to an expert my code would look like a disaster, so I would love if someone could help me make the JavaScript of this slider a bit "prettier".

$(document).ready(function(){

//Configuration
    var width = 720;

    var animationSpeed = 1000;

    var pause = 4000;

    var currentSlide = 1;

    $('#dot1').addClass('activedot');


//Cache DOM

    var $slider = $('#slider');

    var $slideContainer = $slider.find('.slides');

    var $slides = $slideContainer.find('.slide');




//interval needs to be initially set to undefined since it must be a global variable
//to be used in the setInterval method
    var interval;




//slider starting motion
//right arrow function
    var moveForward = function() {


            $slideContainer.animate({'margin-left': '-='+width}, animationSpeed, function() {


               currentSlide++;


               if (currentSlide == 1) {

                    $('#dot1').addClass('activedot');
                    $('.dot').not('#dot1').removeClass('activedot');

                }else if (currentSlide == 2) {

                    $('#dot2').addClass('activedot');
                    $('.dot').not('#dot2').removeClass('activedot');

                }else if (currentSlide == 3) {

                    $('#dot3').addClass('activedot');
                    $('.dot').not('#dot3').removeClass('activedot');

                }else if (currentSlide == 4) {

                    $('#dot4').addClass('activedot');
                    $('.dot').not('#dot4').removeClass('activedot');

                }else if (currentSlide == 5) {

                    $('#dot5').addClass('activedot');
                    $('.dot').not('#dot5').removeClass('activedot');

                }else if (currentSlide == 6) {

                    $('#dot1').addClass('activedot');
                    $('.dot').not('#dot1').removeClass('activedot');

                };

                if(currentSlide === $slides.length) {

                    currentSlide = 1;
                   $slideContainer.css('margin-left',0);

                };

            });

        };



//left arrow function
    var moveBack = function(){


        if (currentSlide == 1) {

                $slideContainer.css('margin-left','-3600px');

                currentSlide=6;

        };

        $slideContainer.animate({'margin-left': '+='+width}, animationSpeed, function(){


            currentSlide--;

            if (currentSlide == 1) {

                $('#dot1').addClass('activedot');
                $('.dot').not('#dot1').removeClass('activedot');

            }else if (currentSlide == 2) {

                $('#dot2').addClass('activedot');
                $('.dot').not('#dot2').removeClass('activedot');

            }else if (currentSlide == 3) {

                $('#dot3').addClass('activedot');
                $('.dot').not('#dot3').removeClass('activedot');

            }else if (currentSlide == 4) {

                $('#dot4').addClass('activedot');
                $('.dot').not('#dot4').removeClass('activedot');

            }else if (currentSlide == 5) {

                $('#dot5').addClass('activedot');
                $('.dot').not('#dot5').removeClass('activedot');

            }else if (currentSlide == 6) {

                $('#dot1').addClass('activedot');
                $('.dot').not('#dot1').removeClass('activedot');

            };

        });
    };  



//starting function
function startSlider(){

    interval = setInterval( moveForward, pause);

};



//pause function
function stopSlider() {

    clearInterval(interval);

}




//listen for mouseenter and pause
//resume on mouseleave
$slider.on('mouseenter', stopSlider).on('mouseleave', startSlider);




//start slider when page is loaded
startSlider();


//dot onclick functions 
$('#dot1').on('click', function(){

    $(this).addClass('activedot');

    $('.dot').not('#dot1').removeClass('activedot');

    $slideContainer.css('margin-left','0px');

    stopSlider();

    currentSlide = 1;
});


$('#dot2').on('click', function(){

    $(this).addClass('activedot');

    $('.dot').not('#dot2').removeClass('activedot');

    $slideContainer.css('margin-left','-720px');

    stopSlider();

    currentSlide = 2;
});


$('#dot3').on('click', function(){

    $(this).addClass('activedot');

    $('.dot').not('#dot3').removeClass('activedot');

    $slideContainer.css('margin-left','-1440px');

    stopSlider();

    currentSlide = 3;
});


$('#dot4').on('click', function(){

    $(this).addClass('activedot');

    $('.dot').not('#dot4').removeClass('activedot');

    $slideContainer.css('margin-left','-2160px');

    stopSlider();

    currentSlide = 4;
});


$('#dot5').on('click', function(){

    $(this).addClass('activedot');

    $('.dot').not('#dot5').removeClass('activedot');

    $slideContainer.css('margin-left','-2880px');

    stopSlider();

    currentSlide = 5;
});

//listen for mouse hovering over dots
$('.nav-dots').on('mouseenter', stopSlider).on('mouseleave', startSlider);



//arrow functions
$('.back').on('mouseenter', stopSlider).on('mouseleave', startSlider).on('click', moveBack);

$('.forward').on('click', moveForward).on('mouseenter',stopSlider).on('mouseleave', startSlider);






//arrow keys function when key is pressed and focus is on html
$('html').keydown(function (key) {

    stopSlider();

    var pressedkey = (key.which);

    if (pressedkey == 37) {

        moveBack();

        startSlider();

    }else if (pressedkey == 39){

        moveForward();

        startSlider();  

     };

 });   



});
\$\endgroup\$
1
\$\begingroup\$

If you are wanting a more DRY approach, I suggest thinking about what is different and what is the same about the operations you are performing. Take this block, for example:

if (currentSlide == 1) {

    $('#dot1').addClass('activedot');
    $('.dot').not('#dot1').removeClass('activedot');

}else if (currentSlide == 2) {

    $('#dot2').addClass('activedot');
    $('.dot').not('#dot2').removeClass('activedot');

}else if (currentSlide == 3) {

    $('#dot3').addClass('activedot');
    $('.dot').not('#dot3').removeClass('activedot');

}else if (currentSlide == 4) {

    $('#dot4').addClass('activedot');
    $('.dot').not('#dot4').removeClass('activedot');

}else if (currentSlide == 5) {

    $('#dot5').addClass('activedot');
    $('.dot').not('#dot5').removeClass('activedot');

}else if (currentSlide == 6) {

    $('#dot1').addClass('activedot');
    $('.dot').not('#dot1').removeClass('activedot');

};

The only thing that is different is the index on #dot for the vast majority of iterations, so that could easily be boiled down to:

if(currentSlide < $slides.length) {
   var $curSlide = $('#dot'+currentSlide);
   $('.dot').removeClass('activedot');
   $curSlide.addClass('activedot');
}

Then you have the case of the last slide, which you are targeting with 2 conditions (currentSlide == 6 and currentSlides == $slides.length). You can just handle both operations together.

$('.dot').removeClass('activedot');

if (currentSlide < $slides.length) {
   var $curSlide = $('#dot'+currentSlide);
   $curSlide.addClass('activedot');
} else {
   $('#dot1').addClass('activedot');
   currentSlide = 1;
   $slideContainer.css('margin-left', 0);
}

Having said all of that, I think you should abandon the id targeting, use classes only and use currentSlide as the index for which element you want to target:

var $dots = $('.dot');
$dots.removeClass('activedot');

if (currentSlide === $slides.length) {
   currentSlide = 1;
   $slideContainer.css('margin-left', 0);
}

$('.dot').eq(currentSlide-1).addClass('activedot');

http://jsfiddle.net/9n8avyk7/11/

The same could be said about your series of event handlers you are adding:

$('#dot2').on('click', function(){

    $(this).addClass('activedot');

    $('.dot').not('#dot2').removeClass('activedot');

    $slideContainer.css('margin-left','-720px');

    stopSlider();

    currentSlide = 2;
});

This could be simplified to one event handler that either got the margin based on image width and the index of the image at the index of the clicked dot dot(4) -> image(4):

$dots.on('click', function() {
    var $this = $(this);
    $dots.removeClass('activedot');
    $this.addClass('activedot');
    stopSlider();
    currentSlide = $this.index(); 
});

I didn't add the part setting the margin because I'm out of time at the moment, but you can get $('.slide').eq(currentSlide).find('img') and parse the image width (multipled by currentSlide) to get the pixel offset.

\$\endgroup\$
  • \$\begingroup\$ Great @AlexTaietti, glad to help :) \$\endgroup\$ – Rob M. May 21 '15 at 18:50
  • \$\begingroup\$ I think I've fixed it, if you want you can check the final code here jsfiddle.net/aletaie/9n8avyk7 :)...do you see anything else that could be cut from the code? \$\endgroup\$ – Alex Taietti May 23 '15 at 11:44

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.