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.

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();
share|improve this question
2  
@palacsint done! –  Jordy Vialoux Jul 23 '12 at 2:13
    
It would be nice to have example html. –  Inkbug Jul 23 '12 at 5:37
    
@JordyVialoux: Thanks :) –  palacsint Jul 23 '12 at 8:26
    
@JordyVialoux You should use === by default, and not ==. –  ANeves Nov 20 '12 at 17:09

2 Answers 2

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.

share|improve this answer

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.

share|improve this answer

Your Answer

 
discard

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

Not the answer you're looking for? Browse other questions tagged or ask your own question.