Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

I've just wrote this class for an image slider (it cross-fades rather than sliding, so a class rename may be beneficial). It's my first proper attempt at JavaScript OOP. Can anyone see any real issues with this?

// Wrapper is the container element, in this case $('#sliderWrap').
// images is an array of the images.

function ImageSlider(wrapper, images){

    // Elements.
    this.elem = wrapper;
    this.images = images;
    this.top = this.elem.find('#topImage');
    this.bottom = this.elem.find('#bottomImage');
    this.next = this.elem.find('#nextBtn');
    this.prev = this.elem.find('#prevBtn');


    this.lastImage = images.length - 1;
    this.currImage = 1; // This is 1 as the first iteration is rendered in the HTML.
    this.interval;
    this.fadeTime = 400;
    this.sliderDelay = 4000 + this.fadeTime;

    var self = this;

    this._init = function(){

        $(window).on('load', function(){
            self.elem.css('height', self.top.height());
            self.next.show();
            self.prev.show();
            self._animate();
            self._startInterval();
            self.next.click(function(){
                clearInterval(self.interval);
                self._animate(true);
                self._startInterval();
            });

            self.prev.click(function(){
                clearInterval(self.interval);
                self._animate(false);
                self._startInterval();
            });
        });
    }

    /**
     * @param next boolean. True next, false prev 
     */

    this._animate = function(next){
        if(typeof next == 'undefined') {
            next = true;
        }

        if (next) {
            self.nextImage();
        } else {
            self.prevImage();
        }
    }

    this.nextImage = function(){
        self.currImage++;


        this.top.fadeOut(self.fadeTime, function(){
            self.bottom.addClass('active');
            self.top.attr('src', self.images[self.currImage - 1]).show();

            if (self.currImage > self.lastImage) {
                self.currImage = 0;
            }

            self.bottom.removeClass('active');

            self.bottom.attr('src', self.images[self.currImage]);
        });
    }

    this.prevImage = function(){

        var minusTwo = self.currImage - 2;

        switch (minusTwo){
            case -1:
                minusTwo = self.lastImage;
                break;
            case -2:
                minusTwo = self.lastImage - 1;
                break;  
        }

        self.bottom.attr('src', self.images[minusTwo]);
        self.top.fadeOut(self.fadeTime, function(){
            self.bottom.addClass('active');

            var plusOne = minusTwo + 1;
            if (plusOne > self.lastImage){
                plusOne = 0;
            }


            self.top.attr('src', self.images[minusTwo]).show();
            self.bottom.attr('src', self.images[plusOne]);
            self.bottom.removeClass('active');
            self.currImage--;
            if(self.currImage < 0){
                self.currImage = self.lastImage;
            }
        });
    }

    this._startInterval = function(){
        self.interval = setInterval(function(){
            self._animate();
        }, self.sliderDelay);
    }

    this._init();
}

The HTML markup to go with this is.

<div id="sliderWrap">
    <div id="nextBtn"><i class="fa fa-chevron-right"></i></div>
    <div id="prevBtn"><i class="fa fa-chevron-left"></i></div>
    <img id="topImage" src="{$featureImages[0]}" />
    <img id="bottomImage" src="{$featureImages[1]}" />
</div>

I think the only really relevant CSS is the active class, which basically puts the bottomImage on top of the topImage (z-index wise).

share|improve this question
    
My immediate reaction is that instead of that switch statement, you should be using modulo %, or doing an if (< 0) += images.length. –  Schism Aug 22 at 14:10
    
I've not seen that if statement before, I'll try it now. Do I need to put minusTwo between the ( and <? –  Tom Hart Aug 22 at 14:20
    
I shortened it; the actual statement would be `var minusTwo = self.currImage - 2; if (minusTwo < 0) { minusTwo += images.length; } –  Schism Aug 22 at 14:21
    
Ahh okay, cheers :). –  Tom Hart Aug 22 at 14:22
    
I think this code could very much benefit of the module pattern, were you not tired of typing this. and self. ? If you provide a jsbin/jsfiddle/plnkr then I might do an extensive review. –  konijn Aug 25 at 15:23

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Browse other questions tagged or ask your own question.