jQuery image carousel

Question

I have made this custom image carousel, using HTML5, jQuery and CSS. My aim was to make it really lightweight but with (just) enough features: "bullets", auto-advance, responsiveness.

var $elm = $('.slider'),
 $slidesContainer = $elm.find('.slides-container'),
 slides = $slidesContainer.children('a'),
 slidesCount = slides.length,
 slideHeight = $(slides[0]).find('img').outerHeight(false),
 animationspeed = 1500,
 animationInterval = 7000;
var shuffle = function(slides) {
 var j, x, i;
 for (i = slides.length - 1; i > 0; i--) {
 j = Math.floor(Math.random() * (i + 1));
 x = slides[i];
 slides[i] = slides[j];
 slides[j] = x;
 }
 return slides;
}
shuffle(slides);
// Set (initial) z-index for each slide
var setZindex = function() {
 for (var i = 0; i < slidesCount; i++) {
 $(slides[i]).css('z-index', slidesCount - i);
 }
};
setZindex();
var setActiveSlide = function() {
 $(slides).removeClass('active');
 $(slides[activeIdx]).addClass('active');
};
var advanceFunc = function() {
 if ($('.slider-nav li.activeSlide').index() + 1 != $('.slider-nav li').length) {
 $('.slider-nav li.activeSlide').next().find('a').trigger('click');
 } else {
 $('.slider-nav li:first').find('a').trigger('click');
 }
}
var autoAdvance = setInterval(advanceFunc, animationInterval);
//Set slide height
$(slides).css('height', slideHeight);
// Append bullets
if (slidesCount > 1) {
 /* Prepend the slider navigation to the slider
 if there are at least 2 slides */
 $elm.prepend('<ul class="slider-nav"></ul>');
 // make a bullet for each slide
 for (var i = 0; i < slidesCount; i++) {
 var bullets = '<li><a href="#">' + i + '</a></li>';
 if (i == 0) {
 // active bullet
 var bullets = '<li class="activeSlide"><a href="#">' + i + '</a></li>';
 // active slide
 $(slides[0]).addClass('active');
 }
 $('.slider-nav').append(bullets);
 }
};
var Queue = function() {
 var lastPromise = null;
 this.add = function(callable) {
 var methodDeferred = $.Deferred();
 var queueDeferred = this.setup();
 // execute next queue method
 queueDeferred.done(function() {
 // call actual method and wrap output in deferred
 callable().then(methodDeferred.resolve)
 });
 lastPromise = methodDeferred.promise();
 };
 this.setup = function() {
 var queueDeferred = $.Deferred();
 // when the previous method returns, resolve this one
 $.when(lastPromise).always(function() {
 queueDeferred.resolve();
 });
 return queueDeferred.promise();
 }
};
var queue = new Queue();
var slideUpDown = function(previousIdx, activeIdx) {
 queue.add(function() {
 return new Promise(function(resolve, reject) {
 // set top property for all the slides
 $(slides).not(slides[previousIdx]).css('top', slideHeight);
 // then animate to the next slide
 $(slides[activeIdx]).animate({
 'top': 0
 }, animationspeed);
 $(slides[previousIdx]).animate({
 'top': "-100%"
 }, animationspeed, 'swing', resolve);
 })
 })
};
var previousIdx = '0' // First slide
$('.slider-nav a').on('click', function(event) {
 event.preventDefault();
 activeIdx = $(this).text();
 // Disable clicling on an active item
 if ($(slides[activeIdx]).hasClass("active")) {
 return false;
 }
 $('.slider-nav a').closest('li').removeClass('activeSlide');
 $(this).closest('li').addClass('activeSlide');
 // Reset autoadvance if user clicks bullet
 if (event.originalEvent !== undefined) {
 clearInterval(autoAdvance);
 autoAdvance = setInterval(advanceFunc, animationInterval);
 }
 setActiveSlide();
 slideUpDown(previousIdx, activeIdx);
 previousIdx = activeIdx
});

body * {
 box-sizing: border-box;
}
.container {
 max-width: 1200px;
 margin: 0 auto;
}
.slider {
 width: 100%;
 height: 300px;
 position: relative;
 overflow: hidden;
}
.slider .slider-nav {
 text-align: center;
 position: absolute;
 padding: 0;
 margin: 0;
 left: 10px;
 right: 10px;
 bottom: 2px;
 z-index: 10;
}
.slider .slider-nav li {
 display: inline-block;
 width: 20px;
 height: 4px;
 margin: 0 1px;
 text-indent: -9999px;
 overflow: hidden;
 background-color: rgba(255, 255, 255, .5);
}
.slider .slider-nav a {
 display: block;
 height: 4px;
 line-height: 4px;
}
.slider .slider-nav li.activeSlide {
 background: #fff;
}
.slider .slider-nav li.activeSlide a {
 display: none;
}
.slider .slider-container {
 width: 100%;
 text-align: center;
}
.slider .slides-container a {
 height: 300px;
 display: block;
 position: absolute;
 top: 0;
 left: 0;
 right: 0;
}
.slider .slides-container img {
 transform: translateX(-50%);
 margin-left: 50%;
}

<script src="https://ajax.googleapis.com/ajax/libs/jquery/3.1.1/jquery.min.js"></script>
<div class="container">
 <div class="slider">
 <div class="slides-container">
 <a href="#">
 <img src="https://i.sstatic.net/P1Di6.jpg">
 </a>
 <a href="#">
 <img src="https://i.sstatic.net/nR2uJ.jpg">
 </a>
 <a href="#">
 <img src="https://i.sstatic.net/Zynhv.jpg">
 </a>
 <a href="#">
 <img src="https://i.sstatic.net/A9BgN.jpg">
 </a>
 </div>
 </div>
</div>

It works fine but I am certain it can be optimized: the same result can be achieved with less code. I am not thinking of a complete rewrite, but of ditching redundant and useless code.

The carousel's current architecture requires the slides to have z-indexes which I wish I could get rid of. What would be a good alternative to that?

See a jsFiddle HERE .

Please help me make it better. Thanks!

• Sᴀᴍ Onᴇᴌᴀ: Is the queue with promises supposed to prevent the slides from moving too quickly or some other purpose?
• Razvan Zamfir: @SᴀᴍOnᴇᴌᴀ Yes, this is the purpose.
• Sᴀᴍ Onᴇᴌᴀ: Why do you believe z-indexes need to be set? The snippet in the first version (without those set) appears to function the same as the snippet in the current version...
• Sᴀᴍ Onᴇᴌᴀ: After getting an answer you are not allowed to change your code anymore. This is to ensure that answers do not get invalidated and have to hit a moving target. If you have changed your code you can either post it as an answer (if it would constitute a code review) or ask a new question with your changed code (linking back to this one as reference). Refer to this post for more information

Answer 1

General Feedback

The carousel appears to work acceptably. The code is a bit scattered. Consider the variable activeIdx. It appears to be set in the click handler and referenced in the function setActiveSlide() as a global variable - not as a parameter, but in slideUpDown() it is a parameter.

I'm not convinced the promise queue is absolutely necessary. Perhaps a simple debounced function would suffice.

And there are a lot of repeated DOM queries - remember those are not cheap! Especially in the function advanceFunc(). Instead of querying the DOM so many times, it would be better to store the list items (A.K.A. bullets) in a variable after they are added and re-use them in advanceFunc(). Modulus division can then be used to determine the next index.

Specific critique

bullet creation

The name of the variable bullets is a bit misleading for a single element. A singular name like bullet would be more appropriate.

And to create each bullet, the jQuery function could be used. So instead of manually constructing the HTML for the list items:

var bullets = '<li><a href="#">' + i + '</a></li>';
if (i == 0) {
 // active bullet
 var bullets = '<li class="activeSlide"><a href="#">' + i + '</a></li>';

use $('<li>') for the bullet and .html() to set the inner HTML:

var bullet = $('<li>').html('<a href="#">' + i + '</a>');

Then the class name can be added via .addClass()

if (i == 0) {
 $(slides[0]).addClass('active');
 bullet.addClass('activeSlide');

That way the inner HTML is only specified once.

Shuffle function return value unused

There is no need to return slides at the end of shuffle():

return slides;

This is because the return value is not assigned to anything (unless you intended for that to be the case):

shuffle(slides);

Rewrite

See the modified code below. It doesn't use the queue or promises at all, and as far as I can tell maintains the same functionality. I also made previousIdx and activeIdx variables outside the functions instead of parameters. Because of this, I wrapped the whole thing in an IIFE to avoid adding those variables to the global scope.

$(function() {
 var $elm = $('.slider'),
 $slidesContainer = $elm.find('.slides-container'),
 slides = $slidesContainer.children('a'),
 slidesCount = slides.length,
 slideHeight = $(slides[0]).find('img').outerHeight(false),
 animationspeed = 1500,
 animationInterval = 7000;
 var activeIdx = 0;
 var previousIdx = 0; // First slide
 var shuffle = function(slides) {
 var j, x, i;
 for (i = slides.length - 1; i > 0; i--) {
 j = Math.floor(Math.random() * (i + 1));
 x = slides[i];
 slides[i] = slides[j];
 slides[j] = x;
 }
 return slides;
 }
 shuffle(slides);
 // Set (initial) z-index for each slide
 var setZindex = function() {
 for (var i = 0; i < slidesCount; i++) {
 $(slides[i]).css('z-index', slidesCount - i);
 }
 };
 setZindex();
 var setActiveSlide = function() {
 $(slides).removeClass('active');
 $(slides[activeIdx]).addClass('active');
 $(bullets).removeClass('activeSlide');
 $(bullets[activeIdx]).addClass('activeSlide');
 };
 function showSlideAtActiveIndex(resetInterval) {
 setActiveSlide();
 slideUpDown(); //previousIdx, activeIdx);
 previousIdx = activeIdx;
 }
 var advanceFunc = function() {
 activeIdx = ++activeIdx % slidesCount;
 showSlideAtActiveIndex();
 }
 var autoAdvance = setInterval(advanceFunc, animationInterval);
 //Set slide height
 $(slides).css('height', slideHeight);
 // Append bullets
 if (slidesCount > 1) {
 /* Prepend the slider navigation to the slider
 if there are at least 2 slides */
 $elm.prepend('<ul class="slider-nav"></ul>');
 // make a bullet for each slide
 for (var i = 0; i < slidesCount; i++) {
 var bullet = $('<li>').html('<a href="#">' + i + '</a>');
 if (i == 0) {
 bullet.addClass('activeSlide');
 // active bullet
 // active slide
 $(slides[0]).addClass('active');
 }
 $('.slider-nav').append(bullet);
 }
 };
 var bullets = $('.slider-nav li');
 var slideUpDown = function() {
 $(slides).not(slides[previousIdx]).css('top', slideHeight);
 // then animate to the next slide
 $(slides[activeIdx]).animate({
 'top': 0
 }, animationspeed);
 $(slides[previousIdx]).animate({
 'top': "-100%"
 }, animationspeed, 'swing');
 };
 $('.slider-nav a').on('click', function(event) {
 var clickedIdx = $(this).text();
 if ($(slides[clickedIdx]).hasClass("active")) {
 return false;
 }
 activeIdx = clickedIdx;
 showSlideAtActiveIndex(); 
 clearInterval(autoAdvance);
 setTimeout(function() {
 autoAdvance = setInterval(advanceFunc, animationInterval);
 }, animationInterval);
 });
});

body * {
 box-sizing: border-box;
}
.container {
 max-width: 1200px;
 margin: 0 auto;
}
.slider {
 width: 100%;
 height: 300px;
 position: relative;
 overflow: hidden;
}
.slider .slider-nav {
 text-align: center;
 position: absolute;
 padding: 0;
 margin: 0;
 left: 10px;
 right: 10px;
 bottom: 2px;
 z-index: 10;
}
.slider .slider-nav li {
 display: inline-block;
 width: 20px;
 height: 4px;
 margin: 0 1px;
 text-indent: -9999px;
 overflow: hidden;
 background-color: rgba(255, 255, 255, .5);
}
.slider .slider-nav a {
 display: block;
 height: 4px;
 line-height: 4px;
}
.slider .slider-nav li.activeSlide {
 background: #fff;
}
.slider .slider-nav li.activeSlide a {
 display: none;
}
.slider .slider-container {
 width: 100%;
 text-align: center;
}
.slider .slides-container a {
 display: block;
 position: absolute;
 top: 0;
 left: 0;
 right: 0;
}
.slider .slides-container img {
 transform: translateX(-50%);
 margin-left: 50%;
}

<script src="https://ajax.googleapis.com/ajax/libs/jquery/3.1.1/jquery.min.js"></script>
<div class="container">
 <div class="slider">
 <div class="slides-container">
 <a href="#">
 <img src="https://picsum.photos/1200/300/?gravity=east" alt="">
 </a>
 <a href="#">
 <img src="https://picsum.photos/1200/300/?gravity=south" alt="">
 </a>
 <a href="#">
 <img src="https://picsum.photos/1200/300/?gravity=west" alt="">
 </a>
 <a href="#">
 <img src="https://picsum.photos/1200/300/?gravity=north" alt="">
 </a>
 </div>
 </div>
</div>

• Razvan Zamfir: Feel free to improve my version. This is the reason it is posted here. Thank you!
• Razvan Zamfir: In fact, click the second bullet of the carousel on your site and you will see that the second slide replaces the first instantly, with no transition.
• Razvan Zamfir: The shuffle(slides); line "randomizes" the slides.
• Sᴀᴍ Onᴇᴌᴀ: Okay I added a modified snippet. I understand the point of shuffle() but am stating that there is no need for return slides; at the end
• Razvan Zamfir: It seems to look and behave differently: the slides do not advance one after the other (does this happen in the snippet window only?). Also, the z-indexes are still there.