Критика чистого js

#javascript #инспекция_кода

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


Максимальная структурированость кода с расчётом на то, что проект будет поддерживаться
много лет и очень часто дорабатываться разными участниками.
Проект высоконагруженный и поэтому использование библиотек типа 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 (prozaik81-2@yandex.ru)
    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);
}

    

Ответы

Ответ 1

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

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

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

Структура кода
Мне кажется имеет смысл вынести в отдельные методы получение ширины элемента (строки
19-28) и установку css свойств (аналог функции css в jquery). Это сделает код более
читабельным.
Определите значения для таких вещей как тип отображения стрелок и направление ротации,
в отдельные константы и используйте их в коде, а не сами значения напрямую. (DIRECTION_LEFT/RIGHT,
ARROW_TYPE_HIDDEN/VISIBLE/AUTO)