Return to Answer

• Don't continually re-query the DOM. Cache your control and slide elements in a variable your you don't need to spend time re-querying for them.
• You don't need to maintain state around clickedSlider condition. Consider using clearInterval() instead as more "proper" way to manage interval functionality.
• Consider breaking apart your autoSlide() function into it's key parts - one function for managing the current slide index and another for actually performing the action of making slide at given index "active". This gives you more code re-use in giving the to give capability to jump directly to a slide at a given index as is needed in your control click handler.
• Do you really a data properties for your controls and slides? If there is a 1-to-1 relationship between the slides and the controls and their ordering in DOM is the same, you should be able to know the control at index X relates to the slide at index X.

$(document).ready(function(){ var $testimonialSlides = $('.testimonial-slider-slides a'); var $testimonialControls = $('.testimonial-slider-controls a'); var slideCount = $testimonialSlides.length; var slideIndex = 0; function activateSlide(idx) { $testimonialSlides .removeClass('active') .eq(idx).addClass('active'); $testimonialControls .removeClass('active') .eq(idx).addClass('active'); } function autoSlide(){ slideIndex++; if (slideIndex === slideCount) { slideIndex = 0; } activateSlide(slideIndex); } var autoslideInterval = window.setInterval(autoslide, 2000); $testimonialControls.click(function(e){ e.preventDefault(); window.clearInterval(autoslideInterval); slideIndex = $testimonialControls.index(this); activateSlide(slideIndex); }); }); 

Why have superfluous comments? If you code is well-written with meaningful function and variables names (I think you do a good job of this), then should not need such comments.

• Don't continually re-query the DOM. Cache your control and slide elements in a variable so you don't need to spend time re-querying for them.
• You don't need to maintain state around clickedSlider condition. Consider using clearInterval() instead as more "proper" way to manage interval functionality.
• Consider breaking apart your autoSlide() function into it's key parts - one function for managing the current slide index and another for actually performing the action of making slide at given index "active". This gives you more code re-use in giving the capability to jump directly to a slide at a given index as is needed in your control click handler.
• Do you really need data properties for your controls and slides? If there is a 1-to-1 relationship between the slides and the controls and their ordering in DOM is the same, you should be able to know the control at index X relates to the slide at index X.

$(document).ready(function(){ var $testimonialSlides = $('.testimonial-slider-slides a'); var $testimonialControls = $('.testimonial-slider-controls a'); var slideCount = $testimonialSlides.length; var slideIndex = 0; function activateSlide(idx) { $testimonialSlides .removeClass('active') .eq(idx).addClass('active'); $testimonialControls .removeClass('active') .eq(idx).addClass('active'); } function autoSlide(){ slideIndex++; if (slideIndex === slideCount) { slideIndex = 0; } activateSlide(slideIndex); } var autoSlideInterval = window.setInterval(autoSlide, 2000); $testimonialControls.click(function(e){ e.preventDefault(); window.clearInterval(autoSlideInterval); slideIndex = $testimonialControls.index(this); activateSlide(slideIndex); }); }); 

Why have superfluous comments? If your code is well-written with meaningful function and variable names (I think you do a good job of this), then you should not need such comments.

$(document).ready(function() { // perhaps not all of the configurations shown below need to be passed // if there are default settings $('.testimonial-slider-slides a').autoslider( { controls: '.testimonial-slider-controls a', activeSlideClass: 'active', interval: 2000, startIndex: 0 } ); // other application code that needs to execute on ready() // perhaps even another autoslider with it's own encapsulated state }); 

$(document).ready(function() { // Perhaps not all of the configurations shown below need to be passed // if there are default settings. // This assumes autoslider() code is already included in document $('.testimonial-slider-slides a').autoslider( { controls: '.testimonial-slider-controls a', activeSlideClass: 'active', interval: 2000, startIndex: 0 } ); // other application code that needs to execute on ready() // perhaps even another autoslider with it's own encapsulated state }); 

You may also consider building this as a proper jQuery plug-in or class (though not shown in my example above). This would allow you to better encapsulate state of the slider functionality, such that state would not need to be held in main $(document).ready() scope. This also potentially makes this simple functionality re-usable in the future - just pass in config for DOM selectors, class name(s) to apply (i.e. active), interval time, etc..

You may also consider building this as a proper jQuery plug-in or class (though not shown in my example above). This would allow you to better encapsulate state of the slider functionality, such that state would not need to be held in main $(document).ready() scope. This also potentially makes this simple functionality re-usable in the future - just pass in config for DOM selectors, class name(s) to apply (i.e. active), interval time, etc. I would rather see a usage pattern like:

$(document).ready(function() { // perhaps not all of the configurations shown below need to be passed // if there are default settings $('.testimonial-slider-slides a').autoslider( { controls: '.testimonial-slider-controls a', activeSlideClass: 'active', interval: 2000, startIndex: 0 } ); // other application code that needs to execute on ready() // perhaps even another autoslider with it's own encapsulated state });