9
\$\begingroup\$

I was just wanting a code review on this image slider I made. I don't have anyone else to give me their opinion on the code I write outside of work.

Is this sloppy? Is it wrong? Is it ineffective? Does it have bad performance?

You can see my repo on the slider here.

deitySlider.js

var deitySlider = { 

    init: function(userDefinedOptions) {
        options = userDefinedOptions;
        settings.images = document.querySelectorAll(options.slider + " > img");
        settings.stages = settings.images.length;
        slider = document.getElementById(options.slider.slice(1));

        initCaptionElement();
        initNavigation();
        initHoverEvent();
        deitySlider.cycle(); //Load first slide.

        setTimerInterval(); 

        function initHoverEvent() {
            if(!options.pauseOnHover) return;
            slider.addEventListener("mouseover", function() {
            clearInterval(settings.interval);
            }, false)

            slider.addEventListener("mouseout", function() {
            if (settings.interval != null)
                setTimerInterval();
            }, false)
        }

        function initCaptionElement() {
            if (!options.captions) return;
            slider.appendChild(document.createElement("div")).className = "deity-captions";
            slider.getElementsByClassName('deity-captions')[0].innerHTML += '<div class="deity-title"></div>';
        }

        function initNavigation() {
            if(!options.directionNav) return;
            slider.appendChild(document.createElement("div")).className = "deity-directionNav";
            slider.getElementsByClassName('deity-directionNav')[0].innerHTML += '<a class="deity-prevNav">'+ options.prevText +'</a><a class="deity-nextNav">'+ options.nextText +'</a>';

            slider.getElementsByClassName('deity-prevNav')[0].addEventListener('click', function() {
            if(settings.stage > 0) settings.stage -= 2;
            else settings.stage = (settings.stages - 2);
            deitySlider.cycle("play");
            setTimerInterval();
            }, false);

            slider.getElementsByClassName('deity-nextNav')[0].addEventListener('click', function() {
            deitySlider.cycle();
            setTimerInterval();
            }, false);
        }

        function setTimerInterval() {
        if (settings.interval) { clearInterval(settings.interval) };
        settings.interval = setInterval(deitySlider.cycle, options.pauseTime);
        };

        console.log("\nslider: " + options.slider + "\npauseTime: " + options.pauseTime + "\nTransitionTime: " + options.transitionTime);
        console.log(settings.stages);
    },

    cycle: function() {
        console.log("cycle");

        var cSettings = settings;
        var cOptions = options;

        cSettings.stage = ++cSettings.stage % cSettings.stages;
        for (var i = 0, length = cSettings.stages; i < length; i++)
            cSettings.images[i].classList.add("hidden");

        cSettings.images[cSettings.stage].classList.remove("hidden");

        if(cOptions.captions) 
            insertImageCaption();

        function insertImageCaption() {
            var title_text = cSettings.images[cSettings.stage].getAttribute('title');
            var description_text = '<div class="deity-description">' + cSettings.images[cSettings.stage].getAttribute('description') + '</div>';
            var title_div = slider.getElementsByClassName('deity-title')[0];

            if(title_text != null && title_text != '')
                title_div.innerHTML = title_text + description_text;
            else title_div.innerHTML = "";
        }   
    }
};

var options = ({
    slider: "#deitySlider",
    pauseTime: 3000,
    transitionTime: 1000,
    directionNav: false,
    pauseOnHover: false,
    captions: false,
    prevText: 'Prev',
    nextText: 'Next',
});

var settings = ({
    stage: -1, //Starting Image
    stages: null,
    interval: null,
    images: null,
});

var slider;

index.html

<html>
<head>
<link rel="stylesheet" type="text/css" href="deity-style.css"/>
<title>Deity Slider Demo</title>
</head>
<body>
<div id="container">
    <div id="slider_container">
        <div id="slider_one" class="deity-Slider">
            <img src="slide_1.jpg" title="Experienced" description="Lorem ipsum dolor sit amet, consectetur adipiscing elit. Vestibulum libero lacus, feugiat sit amet auctor ut, porttitor quis justo. " />
            <img src="slide_2.jpg" title="An Idea" description="Cras id metus luctus, tristique nibh eu, cursus nulla. Etiam a neque nec erat fringilla sagittis a vulputate diam. Fusce vulputate mauris condimentum tortor scelerisque, non congue nisl luctus. Sed ac nibh non sapien tristique consectetur sit amet vitae libero. Sed porttitor lobortis pretium. Sed viverra lorem dolor, sit"/>
            <img src="slide_3.jpg" title="Deity Design" description=" Donec lorem mi, vehicula ut pharetra in, tempus venenatis ante. Donec in dui auctor justo vehicula congue." />
            <img src="slide_4.jpg" title="Knowledgeable" description="Aliquam erat volutpat. Nullam blandit, mi ac viverra mollis, nibh purus auctor mauris, sed dapibus ante lectus quis nibh. Interdum et malesuada fames ac ante ipsum primis in faucibus. Nulla facilisi. "/>
        </div>
    </div>  
</div>

<script type="text/javascript" src="deity.slider.js"></script>
<script>
    window.addEventListener("load", function load(){
    window.removeEventListener("load", load, false);
    deitySlider.init({
        slider: '#slider_one',
        transitionTime: 1000,
        pauseTime: 3000,
        directionNav: true,
        pauseOnHover: true,
        captions: true,
    }); },false);
</script>
</body>
</html>
\$\endgroup\$
7
\$\begingroup\$

Your code needed quite a bit of refactoring. You had unnecessary nested functions, globals all over the place, inconsistent selectors, etc.

I took your code and refactored it so that you now only have 1 global, 'DietySlider'. I also completely refactored the structure so that it separates out every function instead of nesting them. The new structure is much easier to maintain and further extend features.

DietySlider.js

'use strict';

    var DietySlider = (function() {
        var _doc = document;
        var slider;

        var options = {
            slider: "#DietySlider",
            pauseTime: 3000,
            transitionTime: 1000,
            directionNav: false,
            pauseOnHover: false,
            captions: false,
            prevText: 'Prev',
            nextText: 'Next',
        };

        var settings = {
            stage: -1, //Starting Image
            stages: null,
            interval: null,
            images: null,
        };

        return {

            init: function(userDefinedOptions) {
                options = userDefinedOptions;
                slider = _doc.querySelector(options.slider);
                settings.images = _doc.querySelectorAll(options.slider + " > img");
                settings.stages = settings.images.length;
                //run init functions
                DietySlider.initCaptionElement();
                DietySlider.initNavigation();
                DietySlider.initHoverEvent();
                DietySlider.cycle(); //Load first slide
                DietySlider.setTimerInterval();
            },

            initCaptionElement: function() {
                if ( 0 == options.captions)
                    return;

                var element = _doc.createElement("div");
                element.className = "deity-captions";
                element.innerHTML = '<div class="deity-title"></div>';
                slider.appendChild(element);
            },

            initNavigation: function() {
                if( 0 == options.directionNav )
                    return;

                var element = _doc.createElement("div");
                element.className = "deity-directionNav";
                element.innerHTML = '<a class="deity-prevNav">'+ options.prevText +'</a><a class="deity-nextNav">'+ options.nextText +'</a>';
                slider.appendChild(element);

                var prevNav = slider.querySelector('.deity-prevNav');
                prevNav.addEventListener('click', function() {
                    settings.stage > 0 ? settings.stage -= 2 : settings.stage = (settings.stages - 2);
                    DietySlider.cycle("play");
                    DietySlider.setTimerInterval();
                }, false);

                var nextNav = slider.querySelector('.deity-nextNav');
                nextNav.addEventListener('click', function() {
                    DietySlider.cycle();
                    DietySlider.setTimerInterval();
                }, false);
            },

            initHoverEvent: function() {
                if( 0 == options.pauseOnHover )
                    return;

                slider.addEventListener("mouseover", function() {
                    clearInterval(settings.interval);
                }, false);

                slider.addEventListener("mouseout", function() {
                    if (settings.interval > null) {
                        DietySlider.setTimerInterval();
                    }
                }, false);
            },

            cycle: function() {
                console.log("cycling stage: " + (settings.stage + 1));

                var cSettings = settings;
                var cOptions = options;

                cSettings.stage = ( cSettings.stage + 1 ) % cSettings.stages;

                for (var i = 0, length = cSettings.stages; i < length; i++) {
                    if ( i === cSettings.stage ) {
                        cSettings.images[i].classList.remove("hidden");
                    } else {
                        cSettings.images[i].classList.add("hidden");
                    }
                }

                if( cOptions.captions ) {
                    DietySlider.insertImageCaption(cSettings);
                }

            },

            insertImageCaption: function(cSettings) {
                var title_div = slider.querySelector('.deity-title');
                var title_text = cSettings.images[cSettings.stage].getAttribute('title');
                var description = cSettings.images[cSettings.stage].getAttribute('description')
                var description_text = '<div class="deity-description">' + description + '</div>';

                if( title_text != null && title_text != '' && title_text != undefined ) {
                    title_div.innerHTML = title_text + description_text;
                } else {
                    title_div.innerHTML = "";
                }
            },

            setTimerInterval: function() {
                if ( settings.interval > null ) {
                    clearInterval(settings.interval);
                };
                settings.interval = setInterval(DietySlider.cycle, options.pauseTime);
            },

        };
    })();

index.html

<!DOCTYPE HTML>
<html>
    <head>
    <title>Deity Slider Demo</title>
    <link rel="stylesheet" type="text/css" href="deity-style.css"/>
    </head>
    <body>
        <div id="container">
            <div id="slider_container">
                <div id="slider_one" class="deity-Slider">
                    <img src="slide_1.jpg" title="Experienced" description="Lorem ipsum dolor sit amet, consectetur adipiscing elit. Vestibulum libero lacus, feugiat sit amet auctor ut, porttitor quis justo. " />
                    <img src="slide_2.jpg" title="An Idea" description="Cras id metus luctus, tristique nibh eu, cursus nulla. Etiam a neque nec erat fringilla sagittis a vulputate diam. Fusce vulputate mauris condimentum tortor scelerisque, non congue nisl luctus. Sed ac nibh non sapien tristique consectetur sit amet vitae libero. Sed porttitor lobortis pretium. Sed viverra lorem dolor, sit"/>
                    <img src="slide_3.jpg" title="Deity Design" description=" Donec lorem mi, vehicula ut pharetra in, tempus venenatis ante. Donec in dui auctor justo vehicula congue." />
                    <img src="slide_4.jpg" title="Knowledgeable" description="Aliquam erat volutpat. Nullam blandit, mi ac viverra mollis, nibh purus auctor mauris, sed dapibus ante lectus quis nibh. Interdum et malesuada fames ac ante ipsum primis in faucibus. Nulla facilisi. "/>
                </div>
            </div>
        </div>
        <script src="DietySlider.js"></script>
        <script>
            // Document ready
            (function(){
                (function(){var a=setInterval(function(){"complete"===document.readyState?ready(a):!1},16.6666666667)})();
                var ready = function(a) {
                    clearInterval(a);
                    DietySlider.init({
                        slider: '#slider_one',
                        transitionTime: 1000,
                        pauseTime: 3000,
                        directionNav: true,
                        pauseOnHover: true,
                        captions: true
                    });
                }
            })();
        </script>
    </body>
</html>
\$\endgroup\$
1
\$\begingroup\$

In general, I think it is fine.

In a few places, you create an element, append it, then go and find it:

slider.appendChild(document.createElement("div")).className = "deity-directionNav";
slider.getElementsByClassName('deity-directionNav')[0].innerHTML += '<a class="deity-prevNav">'+ options.prevText +'</a><a class="deity-nextNav">'+ options.nextText +'</a>';

It seems like it would be better to create the element, assign it to a variable, then append and modify.

var sliderChild = document.createElement("div");
sliderChild.className = "deity-directionNav";
sliderChild.innerHTML = '<a class="deity-prevNav">'+ options.prevText +'</a><a class="deity-nextNav">'+ options.nextText +'</a>';
slider.appendChild(sliderChild);

This line bothers me: cSettings.stage = ++cSettings.stage % cSettings.stages;

I'd prefer cSettings.state = (1 + cSettings.stage) % cSettings.stages; or the two lines:

++cSettings.stage;
cSettings.stage %= cSettings.stages;

Just because I had to think about the side effect of ++ in your line (yes, I realize it doesn't do anything, but I had to think about it).

I'd prefer checking for undefined options, but that is just a tradeoff between the features and size of the code, so that is up to you.

I assume it works, of course. I didn't actually try it.

\$\endgroup\$

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.