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'd like some tips on optimizing the following code.

I'm a bit concerned that I'm 'polluting the global namespace' by not wrapping it in a function. The commented out var rotator { init { came from a JavaScript tutorial but I'm not sure it's the jQuery way? Should I wrap it in $(function()... instead?

$(document).ready(function() {

//Promos rotation
var $accordionList = $('.accordion').find('li');

// var rotator = {
//      init: function() {
        var numberOfItems = $accordionList.length;
        var currentItem = 0;

        // Set first item to active
        $accordionList.eq(currentItem).addClass('active').find('.content').slideToggle(800, function() {});
        var infiniateLoop = setInterval(function() {
            if(currentItem == numberOfItems - 1){
                currentItem = 0;
            }
            else {
                currentItem++;
            }

            // Remove active class, if is has it, and close content
            $accordionList.parent().find('li.active').removeClass('active')
                .find('.content').slideToggle(800, function() {
            });

            // Add active class and open content
            $accordionList.eq(currentItem).addClass('active').find('.content').slideToggle(800, function() {
            });

        }, 4000 );

        //$('.accordion li').on('click', function () {
        $accordionList.on('click', function () {
            // Stop rotation
            clearInterval(infiniateLoop);

            var $accordionHead = $(this);

            // Remove active class, if is has it, and close content
            $accordionHead.parent().find('li.active').removeClass('active')
                .find('.content').slideToggle(800, function() {
            });

            // Add active class and open content
            $accordionHead.addClass('active').find('.content').slideToggle(800, function() {
            });
        });
//      }
// };

// rotator.init();

});
share|improve this question

migrated from stackoverflow.com Dec 5 '12 at 13:42

This question came from our site for professional and enthusiast programmers.

1  
"I'm not sure it's the JQuery way?" jQuery way? $(function(){}) === $(document).ready(function(){}) –  Raminson Dec 5 '12 at 10:54
    
you are wrapping it in a function... $(document.ready(function... –  ahren Dec 5 '12 at 10:56
    
Your code as it is does not polute any global namespace, as it's all contained within a function being called by $(document).ready. The addition or removal of the rotator object does not change this fact. Either way it's wholey contained within a function –  Jamiec Dec 5 '12 at 10:56
2  
Since you wrapped your code in $(document).ready() and you are using the var keyword you are surely not polluting the global namespace –  devnull69 Dec 5 '12 at 10:56
    
yes but shouldn't I wrap it in another function so it doesn't conflict with other fucntion (yet to be written) that are also in the document.ready ? –  Chris Dec 5 '12 at 10:57
add comment

1 Answer 1

To your question ( confirmed by the comments ), you are not polluting the namespace because you are

  1. Wrapping already inside $(document).ready(function() {
  2. Using var for every variable.

Furthermore, from what I can see that is left over from rotator, it was probably a better approach than what you have now.

From a once over:

  • Spelling is important: infiniateLoop -> infiniteLoop
  • slideToggle(800, function() {}); can simply be written as slideToggle(800);
  • You could use a ternary for increasing currenItem : currentItem = currentItem == numberOfItems - 1 ? 0 : ++currentItem;
  • Please remove commented out code, also please re-indent after taking out rotator
  • It is considered good practice to have one big var at the start instead of multiple vars:
var $accordionList = $('.accordion').find('li'),
    numberOfItems = $accordionList.length,
    currentItem = 0;

    // Set first item to active
  $accordionList.eq(currentItem).addClass('active').find('.content').slideToggle(800);
  • Jshint could not find a single flaw
share|improve this answer
add comment

Your Answer

 
discard

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