5

Друзья, покритикуйте пожалуйста код, он писался для тренировки. Условия следующие:

  1. Максимальная структурированость кода с расчётом на то, что проект будет поддерживаться много лет и очень часто дорабатываться разными участниками.
  2. Проект высоконагруженный и поэтому использование библиотек типа jquery, mootools и т.д. не допускается.

Буду благодарен за советы, которые помогут сделать код и стиль понятнее и рациональнее

Даже если не можете предложить что конкретно исправить, но видите слабое место, то всё равно скажите — я сам подумаю что можно сделать.

// ------------ PLUGIN kalininCarousel INITIALIZATION ------------
var kalininCarousel = new KalininCarousel({
    carouselWrap: document.getElementById('carousel_wrap'),
    offsetElements: 0,
    autoScroll: false,
    effectSpeed: 300,
    autoScrollSpeed: 1000,
    arrowsType: 'auto', //'visible', 'hidden', 'auto'
    rotateBy: 1
});
/* 
    INFO: 
    project: kalininCarousel
    browsers: opera/chrome/FF/ie9/safari
    coding: Sergey Kalinin 07/2013 ([email protected])
    desc: графический компонент для преобразования списка в карусель с возможностью прокрутки при помощи стрелок-указателей. слайды карусели должны быть одинаковой длины
    version 1.1
*/

// ------------ PLUGIN kalininCarousel IMPLEMENTATION ------------
function KalininCarousel(options) {
    // --------- properties ---------   
    var carouselWrap = options.carouselWrap,
        carousel = document.getElementById('carousel'),
        wrapperElements = carousel.getElementsByTagName('ul')[0],
        carouselElements = wrapperElements.getElementsByTagName('li'),
        carouselElementsWidth;
    if (window.getComputedStyle) {
        carouselElementsWidth = carouselElements[0].offsetWidth
                                + parseInt(getComputedStyle(carouselElements[0], '').marginLeft, 10)
                                + parseInt(getComputedStyle(carouselElements[0], '').marginRight, 10);
    }
    else {
        carouselElementsWidth = carouselElements[0].offsetWidth
                                + parseInt(carouselElements[0].currentStyle.marginLeft, 10)
                                + parseInt(carouselElements[0].currentStyle.marginRight, 10);
    }

    var carouselLength = carouselElements.length,
        running = false,
        autoScroll = 0,
        prev,
        next;

    init();

    // --------- methods ---------  
    function init() {
        wrapperElements.style.width = carouselElementsWidth * carouselLength * 2 + 'px';

        prev = prepend(carouselWrap, 'div');
        prev.id = 'prev';
        prev.className = 'prev';

        next = prepend(carouselWrap, 'div');
        next.id = 'next';
        next.className = 'next';


        if ((options.arrowsType == 'auto') || (options.arrowsType == 'hidden')) {
            toggleControls('none', 'none');
        }

        setIntervalAutoScroll();
    }

    function shift(direction) {
        var offset,
            cloneSlides,
            lastSlide,
            i;

        if (!running) {
            running = true;

            if (options.autoScroll) {
                window.clearInterval(autoScroll);
            }

            if (direction == -1) {
                offset = (carouselElementsWidth * options.rotateBy * direction);

                for (i = 0; i < options.rotateBy; i++) {
                    cloneSlides = carouselElements[i].cloneNode(true);

                    wrapperElements.appendChild(cloneSlides);
                }

                shiftAction();
            }
            else {
                offset = 0;

                for (i = (carouselLength - 1) ; i > (carouselLength - options.rotateBy - 1) ; i--) {
                    cloneSlides = carouselElements[carouselLength - 1].cloneNode(true);

                    wrapperElements.insertBefore(cloneSlides, wrapperElements.firstChild);
                }

                wrapperElements.style.left = (-1 * options.rotateBy * carouselElementsWidth) + 'px';

                shiftAction();
            }
        }

        function shiftAction() {
            var interval,
                offset;

            if (direction == -1) {
                offset = 0;

                function animateLeft() {
                    offset -= 13;

                    wrapperElements.style.left = offset + 'px';

                    if (offset <= (-1 * options.rotateBy * carouselElementsWidth)) {
                        clearInterval(interval);

                        for (i = 0; i < options.rotateBy; i++) {
                            wrapperElements.removeChild(document.getElementsByTagName('li')[0]);
                        }

                        wrapperElements.style.left = 0;

                        running = false;
                    }
                }

                setIntervalCustom(animateLeft);
            }
            else {
                offset = parseInt(wrapperElements.style.left, 10);

                function animateRight() {
                    offset += 13;

                    wrapperElements.style.left = offset + 'px';

                    if (offset >= 0) {
                        clearInterval(interval);

                        for (i = options.rotateBy; i > 0; i--) {
                            wrapperElements.removeChild(document.getElementsByTagName('li')[carouselLength + i - 1]);
                        }

                        wrapperElements.style.left = 0;

                        running = false;
                    }
                }

                setIntervalCustom(animateRight);

            }

            function setIntervalCustom(animate) {
                return interval = setInterval(animate, 25);
            }
        }
    }

    function toggleControls(prevState, nextState) {
        prev.style.display = prevState;

        next.style.display = nextState;
    }

    function prepend(id, tag) {
        var first = id.firstChild,
            newNode = document.createElement(tag);

        id.insertBefore(newNode, first);

        return newNode;
    }

    function setIntervalAutoScroll() {
        if (options.autoScroll) {
            autoScroll = window.setInterval(function () {
                shift(-1);
            }, options.autoScrollSpeed);
        }
    }

    function addEvent(elem, type, handler) {
        if (elem.addEventListener) {
            elem.addEventListener(type, handler, false);
        }
        else {
            elem.attachEvent('on' + type, handler);
        }
    }

    // --------- handlers ---------     
    function onClickControls(direction) {
        shift(direction);
    }

    function onHoverWrap() {
        if (options.arrowsType == 'auto') {
            toggleControls('block', 'block');
        }
    }

    function onLeaveWrap() {
        if (options.arrowsType == 'auto') {
            toggleControls('none', 'none');
        }
    }

    // --------- events ---------   
    addEvent(prev, 'click', function (e) {
        onClickControls(-1);
    }, false);

    addEvent(next, 'click', function (e) {
        onClickControls(1);
    }, false);

    addEvent(carouselWrap, 'mouseover', function (e) {
        onHoverWrap();
    }, false);

    addEvent(carouselWrap, 'mouseleave', function (e) {
        onLeaveWrap();
    }, false);
}
15
  • 4
    а jquery действительно так сильно влияет на производительность? я думаю у Вас в основном на сервере будет проблемы? а не на клиенте. в хроме красные квадратики один раз появляются и больше не пропадают. в фаерфоксе - пропадают.
    – KoVadim
    11 июл 2013 в 14:37
  • 1
    Если производительность и правда важна, надо думать не о том, использовать jQuery или нет, а о том, использовать javascript или C.
    – VladD
    11 июл 2013 в 15:35
  • 3
    код не должен быть на вашем сайте, код должен быть здесь
    – zb'
    11 июл 2013 в 18:16
  • 1
    кроме того, как производительность клиентского софта влияет на высоконагруженность сервера ? Кроме того, поддерживать много лет многими сменяющимися людьми намного проще с использованием популярных библиотек. А оптимизация карусели, таким способом вообще странная идея. У вас их на одной странице будет 10000 одновременно крутящихся ?
    – zb'
    11 июл 2013 в 18:18
  • 3
    будет красивее и быстрее, но как я указал в первом посте(чтобы не было недопонимания) код писался для тренировки в чистом js. сейчас много любителей использовать готовые решения, но они(готовые решения) в определенном смысле вредны. ваш темперамент выдаёт ваш возраст. культура кода это далеко не глупости. и вопрос вполне в формате этого сайта. вы наверное ни разу не работали с большими проектами
    – cyklop77
    11 июл 2013 в 18:47

1 ответ 1

3

Мельком посмотрел код, в глаза бросились несколько моментов:

Настройки карусели

Не увидел у Вас стандартных настроек для быстрого запуска карусели например с указанием одного лишь id элемента на странице. Думаю имеет смысл их добавить.


Создание карусели. привязка к DOM элементам

У Вас идет привязка карусели к конкретным элементам на странице, в частности элементу с id="carousel". Также Вы всегда исходите из того, что карусель будет создана на основе ul. Как на счет нескольких каруселей на странице? Рекомендую дать возможность задавать через настройки какой элемент является каруселью, какие теги являются элементами итд.


Структура кода

Мне кажется имеет смысл вынести в отдельные методы получение ширины элемента (строки 19-28) и установку css свойств (аналог функции css в jquery). Это сделает код более читабельным.

Определите значения для таких вещей как тип отображения стрелок и направление ротации, в отдельные константы и используйте их в коде, а не сами значения напрямую. (DIRECTION_LEFT/RIGHT, ARROW_TYPE_HIDDEN/VISIBLE/AUTO)

4
  • 2
    плюс дублирование кода в shift() идет. 11 июл 2013 в 16:00
  • определить константы типа DIRECTION_LEFT/RIGHT я сначала хотел, но потом подумал, что это лишняя операция будет. почему бы сразу не использовать options.arrowsType ? думаю, это и быстрее, и программисту запоминать лишнего не нужно. нет?... от дублирования в shift() не получилось избавиться. была мысль вынести в отдельный метод, но там настолько разный код, что я растерялся. если кто-нибудь хотя бы приблизительным кодом поможет, то буду благодарен
    – cyklop77
    11 июл 2013 в 17:10
  • 2
    Ну, о константах Вы судите зная свой код. Я вот не смог сразу сказать что означет условие direction == -1. А Вы в свою очередь сказали, что код будет поддерживаться разными людьми. 11 июл 2013 в 17:30
  • спасибо. действительно нужно исправить
    – cyklop77
    11 июл 2013 в 17:50

Ваш ответ

By clicking “Отправить ответ”, you agree to our terms of service and acknowledge you have read our privacy policy.

Всё ещё ищете ответ? Посмотрите другие вопросы с метками или задайте свой вопрос.