Custom jQuery Slider

Question

Can someone please help to rewrite / tidy up this?

// News Article Slideshow
var periodToChangeSlide = 5000;
var pp_slideshow = undefined;
var currentPage = 0;

$('#news-feature-img-wrap li').css('display', 'list-item').slice(1).css('display', 'none');
$('#news-items li:first').addClass('active');

$("#news-feature-wrap #news-items li").click(function () {
    $(this).parent().addClass('active');
    $(this).parent().siblings().removeClass('active');

    var index = $(this).parent().index();
    var toShow = $("#news-feature-wrap #news-feature-img-wrap li").eq(index);
    toShow.show();
    toShow.siblings().hide();
    currentPage = index;
    $.stopSlideshow();
});

$.startSlideshow = function () {
    if (typeof pp_slideshow == 'undefined') {
        pp_slideshow = setInterval($.startSlideshow, periodToChangeSlide);
    } else {
        $.changePage();
    }
}

$.stopSlideshow = function () {
    clearInterval(pp_slideshow);
    pp_slideshow = undefined;
}
$.changePage = function () {
    var numSlides = $('#news-feature-wrap #news-feature-img-wrap li').length;
    currentPage = (currentPage + 1) % numSlides;
    var menu = $('#news-feature-wrap #news-items li').eq(currentPage);
    menu.addClass('active');
    menu.siblings().removeClass('active');

    var toShow = $("#news-feature-wrap #news-feature-img-wrap li").eq(currentPage);
    toShow.show();
    toShow.siblings().hide();
}

$.startSlideshow();

Answer 1

I've got a few suggestions for you:

Wrap your entire thing in an IIFE, you don't need any external access to any of your vars.

;(function(){
   ...your code goes here
}());

Now your period to change slide isn't in the global name space.

You don't need to attach your startSlideSnow and stopSlideShow to the jQuery namespace (e.g. $.startSlideShow) you are just polluting jQuery's namespace which isn't goot.

Use setTimeout() instead of setInterval. The setTimeout function will only run after its callback is done, setInterval runs at the interval no matter what, so you can run into some strange behavior.

Try to not repeat selectors, and use chaining:

$(this).parent().addClass('active');
$(this).parent().siblings().removeClass('active');

could become:

$(this).parent().addClass('active').siblings().removeClass('active');

You could optimize your selectors as well, remember jQuery goes right to left, so in a lot of cases you're getting all li items on the page and filtering those. It might be a marginal speed boost doing $( 'selector' ).find( 'li' );.

There are a few more things you could do, like pluginize it, but these are the biggest things I see right away.

Answer 2

Without HTML we could not do too much, but here is what I noticed right away.

I would definately use more of the chaining that is provided with jQuery.

$("#news-feature-wrap #news-items li").click(function () {
    $(this).parent().addClass('active').siblings().removeClass('active');

    var index = $(this).parent().index();
    $("#news-feature-wrap #news-feature-img-wrap li").eq(index).show().siblings().hide();
    currentPage = index;
    $.stopSlideshow();
});

This is more so taking advantage of the chaining in jQuery. It will not change performance and is strictly aesthetic.