Как можно оптимизировать данный код?

Question

[Capitan_S]

В целях изучения jQuery делаю простую "карусельку" с помощью jQuery. Как можно улучшить этот код?

$(function(){
    var slider = $('.slider-inner'),
        slides = $('.slide'),
        dotes = $('.slider-dot');

    $('.arrow-left').on('click', function(){
        var slideNumber = slides.index($('.slide-active')),
            slideWidth = slides.eq(slideNumber).width();

        slides.eq(slideNumber).removeClass('slide-active');
        dotes.eq(slideNumber).removeClass('dot-active');

        if(slideNumber === 0) {
            slides.eq(slides.length-1).addClass('slide-active');
            dotes.eq(slides.length-1).addClass('dot-active');
            slider.animate({marginLeft:-slideWidth*(slides.length-1) + "px"}, 600);
        } else {
            slides.eq(slideNumber-1).addClass('slide-active');
            dotes.eq(slideNumber-1).addClass('dot-active');
            slider.animate({marginLeft:-slideWidth*(slideNumber-1) + "px"}, 600);
        }
    });

    $('.arrow-right').on('click', function(){
        var slideNumber = slides.index($('.slide-active')),
            slideWidth = slides.eq(slideNumber).width();

        slides.eq(slideNumber).removeClass('slide-active');
        dotes.eq(slideNumber).removeClass('dot-active');

        if(slideNumber === (slides.length-1)) {
            slides.eq(0).addClass('slide-active');
            dotes.eq(0).addClass('dot-active');
            slider.animate({marginLeft:"0px"}, 600);
        } else if (slideNumber < (slides.length-1)) {
            slides.eq(slideNumber+1).addClass('slide-active');
            dotes.eq(slideNumber+1).addClass('dot-active');
            slider.animate({marginLeft:-slideWidth*(slideNumber+1) + "px"}, 600);
        }
    });

    dotes.on('click', function(){
        var oldSlide = $('.slide-active'),
            oldDote = $('.dot-active'),
            dotNumber = dotes.index($(this)),
            slideWidth = slides.eq(dotNumber).width();


        oldSlide.removeClass('slide-active');
        oldDote.removeClass('dot-active');

        slides.eq(dotNumber).addClass('slide-active');
        dotes.eq(dotNumber).addClass('dot-active');

        slider.animate({marginLeft:-slideWidth*dotNumber + "px"}, 600);
    })
});

Answer 1

[Денис Инешин]

Вы подошли к проблеме не с той стороны, смотрите в DOM, чтобы понять что происходит, а должно быть наоборот.
Логика примерно такая:
1. Вы знаете что у вас 10 картинок
2. У вас есть индекс текущей картинки
3. Клики на стрелочки влево/вправо или на точки навигации лишь изменяют этот индекс
4. После изменения индекса, приложение пересчитывает координаты сдвига, и назначает элемент, которому добавить класс active

Ну и никаких повторений кода не будет в этой ситуации.

Comments

[Capitan_S]

Спасибо огромное за совет. После прочтения документации получилось написать рабочий код и хотелось бы узнать ваше мнение о нём. Можно ли его еще где-нибудь подправить, улучшить?

$(function(){
    var slider = $('.slider-inner'),
        slides = $('.slide'),
        dotes = $('.slider-dot');

    $('.arrow-left').on('click', function(){
        var slideNumber = slides.index($('.slide-active')),
            slideWidth = slides.eq(slideNumber).width();

        slides.eq(slideNumber).removeClass('slide-active');
        dotes.eq(slideNumber).removeClass('dot-active');

        if(slideNumber === 0) {
            slides.eq(slides.length-1).addClass('slide-active');
            dotes.eq(slides.length-1).addClass('dot-active');
            slider.animate({marginLeft:-slideWidth*(slides.length-1) + "px"}, 600);
        } else {
            slides.eq(slideNumber-1).addClass('slide-active');
            dotes.eq(slideNumber-1).addClass('dot-active');
            slider.animate({marginLeft:-slideWidth*(slideNumber-1) + "px"}, 600);
        }
    });

    $('.arrow-right').on('click', function(){
        var slideNumber = slides.index($('.slide-active')),
            slideWidth = slides.eq(slideNumber).width();

        slides.eq(slideNumber).removeClass('slide-active');
        dotes.eq(slideNumber).removeClass('dot-active');

        if(slideNumber === (slides.length-1)) {
            slides.eq(0).addClass('slide-active');
            dotes.eq(0).addClass('dot-active');
            slider.animate({marginLeft:"0px"}, 600);
        } else if (slideNumber < (slides.length-1)) {
            slides.eq(slideNumber+1).addClass('slide-active');
            dotes.eq(slideNumber+1).addClass('dot-active');
            slider.animate({marginLeft:-slideWidth*(slideNumber+1) + "px"}, 600);
        }
    });

    dotes.on('click', function(){
        var oldSlide = $('.slide-active'),
            oldDote = $('.dot-active'),
            dotNumber = dotes.index($(this)),
            slideWidth = slides.eq(dotNumber).width();


        oldSlide.removeClass('slide-active');
        oldDote.removeClass('dot-active');

        slides.eq(dotNumber).addClass('slide-active');
        dotes.eq(dotNumber).addClass('dot-active');

        slider.animate({marginLeft:-slideWidth*dotNumber + "px"}, 600);
    })
});

Спасибо за внимание.

Answer 2

[Владимир Голубев]

Может, стоит попробовать по другому? Все дивы с картинками, кроме первого, спрятать, по клику или таймеру видимый див скрывать с анимацией "откатывания" вбок, одновременно с этим "проявлять" следующий. По моему будет меньше кода)

Comments

[Владимир Голубев]

Не нужны классы slide-1....slide-N. Вы просто ищите активный\видимый элемент и его ближайшего "соседа" справа или слева.

[Capitan_S]

Владимир Голубев: Подобный слайдер с видимыми и невидимыми дивами я уже сделал и разобрал для себя. Теперь хотелось реализовать карусель со сдвигом.

[Владимир Голубев]

Capitan_S: В любом случае - классы с нумерацией избыточны, проще вычислять элементы от текущего. Если в карусельке три слайда видны за раз - вам нужно знать главный, предыдущий и следующий. Главный у вас отмечен классом slide-active соседей можно найти селекторами jquery без проблем.