Simple jQuery Slider Plugin

Question

I'm writing a very simple fading slider plugin for my own use. I would like to know what I could do to improve upon what I've already done.

I think I've done well, but I am certain there are things that I can do to improve.

(function($){
    $.fn.slider = function(options){
        var settings = $.extend({
            selector:this,
            speed:3000
        }, options);

        var $children = settings.selector.children();//Check the children from the element calling this function
        var $currentChildIndex = 0;//Set the current Index to 0 since we've not started yet.
        var $current = $($children[$currentChildIndex]);//Get the current child (Redundant?)

        setInterval(function(){
            $current = $($children[$currentChildIndex]);//Assign current from list of children using currentChildIndex

            $current.addClass('active');//Assign active to current slider

            $children.each(function(key,obj){
                if(!$current.is($(obj))){//Remove all active elements that aren't the current slider
                    $(obj).removeClass('active');
                }
            });

            if($currentChildIndex==($children.length-1)){//Check if Index has reached the end, if so reset to start
                $currentChildIndex=0;
            }else{
                $currentChildIndex++;
            }
        },settings.speed);//Wait every X seconds before changing slide
    }
}(jQuery));

Answer 1

• I think you can remove all of the comments you have here, as I think you are already using mostly meaningful variable names (a few minor comments on this below) and the code is clear in intent. The comments here mostly seem like clutter. This is not to mention that comments at the end of a line of code are generally hard to read. Comments should typically be before the line/section of code to which they apply on their own line(s).
• Typically jQuery plug-ins are applied like $([selector]).slider(). Such that inside the plugin code this refers to a jQuery collection. You are not treating this as a collection but rather as an individual element. Think of this as a collection. Also, I don't think selector setting is relevent for this reason. Typically element selection has already been applied by the point you are in the plug-in code. Keeping this philosophy of potentially working with a collection of elements, would also allow you to invoke multiple sliders with one pass.

For example:

// every element with slider class will be turned into a slider
$('.slider').slider();

• Typically jQuery plug-ins are designed to maintain chaining so a return statement should be applied to allow the collection that the plug-in is acting against (or some filtered subset of it depending on the logic of the plug-in) to be returned for further method chaining.
• Should $children be named $slides to indicate the selected children represent the slides to be displayed?
• Regarding var $currentChildIndex, why not just currentChildIndex? The styling approach of prepending $ to variables is typically to help identify jQuery collections. This is not a jQuery collection. This variable name is probably also unnecessarily long. Perhaps just currentIndex which still conveys clear meaning.
• I don't think you need $current at all. You can directly reference members of the $children collection directly using $children.eq(currentIndex)
• No need to iterate $children to remove class from inactive children. You can simplify this.

For example:

$children.removeClass('active');
$children.eq(currentIndex).addClass('active');

• You can simplify your if-else (to avoid unnecessary else code paths).

For example:

currentChildIndex++;
if(currentChildIndex === $children.length) {
    currentChildIndex = 0;
}

• I would recommend using strict comparisons (===, !==) as your default means to compare rather than loose comparisons. Loose comparisons tend to introduce bugs around truthy/falsey behavior (though in this case there should not be concern as you are clearly only comparing integer values). IMO, this is just a good habit to get into with one's coding. Only use loose comparisons where there is a specific reason to do so.

Putting it all together you might have something like:

(function($){
    $.fn.slider = function(options){
        var settings = $.extend({
            speed:3000
        }, options);

        return this.each(function() {
            var $slides = $(this).children();
            var currentIndex = 0;

            setInterval(function(){
                $slides.removeClass('active');
                $slides.eq(currentIndex).addClass('active');
                currentIndex++;
                if (currentIndex === $slides.length) {
                    currentIndex = 0;
               }
            },settings.speed);
        });
    }
}(jQuery));