JavaScript/jQuery Image Slider

Question

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).

Answer 1

Firstly, I don't see any "real" issues with this - you mostly have done an awesome job! I have a few recommendations though that might help this be a little more performant, readable, and refined:

1. Do you really need all of these to be instance/public variables? Later on when you instantiate an ImageSlider, are you going to need an external module to know about these properties? Most of them could probably be private with var
2. lastImage and currImage are improperly/confusingly named - really they should be lastIndex and currIndex, because they are storing the index of an image, not a reference to the image itself.
3. It is a common convention to store a reference to the current scope at the top of a constrctor function.
4. The prefix "_" (underscore) is usually used to mark a function as private (using var _animate, for example). The airbnb style guide shows a usage similar to yours here, but, it is a very bad idea and I can't imagine why they would spread it. If a variable is attached to this, i.e. this.anything/this['anything'], it is, in JavaScript, not private. Putting this _ prefix is incredibly unintuitive and many people have told them so. At first glance, I thought maybe you were using it to indicate that the property was a method (i.e., this.elem and this.images are properties, but this._animate and this._init are functions), but I see you have other methods that do not use this prefix. If you are going to use this prefix to denote that it is a function, definitely be consistent with it.
5. You can remove the typeof check by reversing your logic a bit.
6. Did you mean to use "this" here, or "self"? Be sure to be consistent.
7. Use jQuery chaining to reduce scope resolution - you were calling this.bottom twice but you only needed to resolve that variable once, since jQuery will return the element for chainability. You can break it onto two lines if you prefer as well.
8. Pass animate by reference to setInterval instead of creating an anonymous scope to wrap the item.
9. minusTwo and plusOne are, at least for me, difficult to understand. I have read the function a few times and still do not quite understand why these variables have this name.
10. Cache this.bottom and this.top in order to reduce scope resolution in this function, since you are using them both quite a bit.

Here is the code, slightly modified and rewritten with specific numeric callouts:

function ImageSlider(wrapper, images){

    // 3
    var self = this;

    // 1
    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');


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

    // 4
    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 
     */
    // 4, 5
    this._animate = function(next){
       if(!next){
         self.prevImage();
        } else {
          self.nextImage();
        }
    }

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

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

            if (self.currIndex > self.lastIndex) {
                self.currIndex = 0;
            }

            // 7
            self.bottom.removeClass('active').attr('src', self.images[self.currIndex]);
        });
    }

    this.prevImage = function(){

        // 9
        var minusTwo = self.currIndex - 2;

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

        // 10
        var bottom = self.bottom,
            top    = self.top;

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

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


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

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

    // 4
    this._init();
}

Answer 2

1. Using Function.bind() can mean you can refer to this consistently (include a polyfill if necessary).
2. You can move the methods of ImageSlider to its prototype. This means that if there are multiple instances of ImageSlider then they do not each duplicate the functions unnecessarily.
3. You can move fadeTime and sliderTime to the prototype; again this means they are stores once on the class and not once per instance.
4. You do not need to reset the interval's function each time an animation occurs - you can instead pass the next / previous function to setInterval.
5. I've simplified (IMO) the code for getting the index of the next and previous images and moved it into a function to make the code more DRY.
6. You don't need $(window).on('load', ... in the constructor - instead wrap the call where you instantiate the class in that.

Code:

function ImageSlider(wrapper, images){

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

    this.currImage = 1;

    this.elem.css('height', this.top.height());

    this.next.show();
    this.next.css('margin-top', (this.next.outerHeight()/2)*-1);

    this.prev.show();
    this.prev.css('margin-top', (this.prev.outerHeight()/2)*-1);

    var nxt = this.nextImage.bind( this );
    var prv = this.prevImage.bind( this );
    this.animate( nxt );
    this.next.click( this.animate.bind( this, nxt ) );
    this.prev.click( this.animate.bind( this, prv ) );
}

ImageSlider.prototype.fadeTime    = 400;
ImageSlider.prototype.sliderDelay = 4000 + ImageSlider.prototype.fadeTime;

ImageSlider.prototype.animate = function( animationFn ){
    if ( this.interval )
    {
        clearInterval(this.interval);
        animationFn();
    }
    this.interval = setInterval(
        animationFn,
        this.sliderDelay
    );
}

ImageSlider.prototype.getNextIndex = function( increment ){
    var len = this.images.length;
    return ( len + this.currImage + increment ) % len;
}

ImageSlider.prototype.nextImage = function(){
    this.top.fadeOut(this.fadeTime, function(){
        this.bottom.addClass('active');
        this.top.attr('src', this.images[this.currImage]).show();

        this.currImage = this.getNextIndex(1);

        this.bottom.removeClass('active');

        this.bottom.attr('src', this.images[this.currImage]);
    }.bind( this ));
    console.log( "next", this.currImage );
}

ImageSlider.prototype.prevImage = function(){
    var minusTwo = this.getNextIndex(-2);
    this.bottom.attr('src', this.images[minusTwo]);
    this.top.fadeOut(this.fadeTime, function(){
        this.bottom.addClass('active');
        this.top.attr('src', this.images[ minusTwo ]).show();
        this.currImage = this.getNextIndex(-1);
        this.bottom.attr('src', this.images[this.currImage]);
        this.bottom.removeClass('active');
    }.bind( this ));
    console.log( "prev", this.currImage );
}

var LayerData = {
    featureImages:  ["https://dev.example.com/files/features/2/WNgrRdqZ_full.jpg", "https://dev.example.com/files/features/2/xoPS0udB_full.jpg", "https://dev.example.com/files/features/2/2jFVEbBr_full.jpg"]
};

var slider;
//$(window).on('load', function(){
slider = new ImageSlider($('#sliderWrap'), LayerData.featureImages);
console.log('Running');
//});