Carousel in Vanilla JavaScript

Question

I finally got my carousel to work in JavaScript, and I want to know what you guys think about it and what I can do better.

var reviews = document.getElementsByClassName('review');
var leftArrow = document.getElementsByClassName('arrow')[0];
var rightArrow = document.getElementsByClassName('arrow')[1];

var currentReview;
var nextReview;

function carousel(direction) {
  for (var i = 0; i < reviews.length; i++) {
    if (reviews[i].classList.contains("show")) {
      currentReview = reviews[i];
      if (direction == 'forward') {
        if (i + 1 > reviews.length - 1) {
          nextReview = reviews[0];
        } else {
          nextReview = reviews[i + 1];
        }
      } else {
        if (i - 1 < 0) {
          nextReview = reviews[reviews.length - 1];
        } else {
          nextReview = reviews[i - 1];
        }
      }
    }
  }

  currentReview.classList.remove("show");
  nextReview.classList.add("show");
}

leftArrow.onclick = function() {
  carousel('backward');
}
rightArrow.onclick = function() {
  carousel('forward');
}

* {
  font-family: Arial;
}

.carousel {
  display: flex;
  align-items: center;
  justify-content: center;
}

.review {
  display: none;
  text-align: center;
}

.show {
  display: block;
}

.arrow {
  margin-left: 25vw;
  margin-right: 25vw;
}

.arrow-left {
  width: 0;
  height: 0;
  border-top: 10px solid transparent;
  border-bottom: 10px solid transparent;
  border-right: 10px solid black;
}

.arrow-right {
  width: 0;
  height: 0;
  border-top: 10px solid transparent;
  border-bottom: 10px solid transparent;
  border-left: 10px solid black;
}

<div class="carousel">
  <div class="arrow-left arrow"></div>
  <div class="reviews">
    <div class="review show">
      <h1 class="title">Title1</h1>
    </div>
    <div class="review">
      <h1 class="title">Title2</h1>
    </div>
    <div class="review">
      <h1 class="title">Title3</h1>
    </div>
  </div>
  <div class="arrow-right arrow"></div>

It can also be demonstrated in this JSFiddle

Answer 1

Arrow elements

There are two elements for the arrows, and presuming there is only one of each type (i.e. one left, one right) then it would be more appropriate to use an id attribute for distinguishing between those two.

Instead of

<div class="arrow-left arrow"></div>
<div class="arrow-right arrow"></div>

use the id attribute:

<div class="arrow" id="arrow-left"></div>
<div class="arrow" id="arrow-right"></div>

And instead of selecting elements by class name and taking the first one:

var leftArrow = document.getElementsByClassName('arrow')[0];
var rightArrow = document.getElementsByClassName('arrow')[1];

select them using document.getElementById():

var leftArrow = document.getElementById('arrow-left');
var rightArrow = document.getElementById('arrow-right');

Obviously this would require the CSS to be updated as well.

Direction parameter

There are only two possible values for direction, so it could be changed to a boolean like forward that defaults to true (default parameters is a feature of ecmascript-6 so be aware of the browser compatibility).

function carousel(forward = true) {
  if (forward) {
    //move forward 
  }
  else { 
    //move backward
  }

Simplify onclick event handlers

After making the first parameter for carousel a boolean that defaults to true (for the direction), the onclick handlers can be simplified to function references. false can be tied to the leftArrow handler by employing a partially applied function with Function.bind()

leftArrow.onclick = carousel.bind(null, false);
rightArrow.onclick = carousel;

If there was a need to have multiple event handlers on each element then addEventListener() could be used:

leftArrow.addEventListener('click', carousel.bind(null, false));
rightArrow.addEventListener('click', carousel);

Simplify logic for current and next

Instead of looping through the reviews, select the elements that have class name show (should only be one):

var shownReviews = document.getElementsByClassName('show');

That will be a live HTMLCollection, meaning that it updates as the DOM is updated, so there is no need to re-query it.

Then you can utilize shownReviews[0] - this can be used to set currentReview. Then when forward is true the value for nextReview can be set utilizing nextElementSibling or if that is null using parentElement.firstElementChild (and conversely using previousElementSibling and lastElementChild when forward is false).

function carousel(forward = true) {
  currentReview = shownReviews[0];
  if (forward) {
    nextReview = currentReview.nextElementSibling || currentReview.parentElement.firstElementChild;  
  }
  else {
    nextReview = currentReview.previousElementSibling || currentReview.parentNode.lastElementChild;   
  }
  currentReview.classList.remove("show");
  nextReview.classList.add("show");
}

Simplify CSS

The following styles can be moved out of each of the rulesets for the arrows and moved into the ruleset for .arrow:

width: 0;
height: 0;
border: 10px solid transparent; 

Then the rulesets for the arrows becomes a single style:

#arrow-left {
  border-right: 10px solid black;
}

#arrow-right {
  border-left: 10px solid black;
}

The margin styles for .arrow can be combined to a single style using two values:

margin: 0 25vw; /* top & bottom: 0, left & right: 25vw */

---

Rewrite

Below is simplified code using the advice above.

var shownReviews = document.getElementsByClassName('show');
var leftArrow = document.getElementById('arrow-left');
var rightArrow = document.getElementById('arrow-right');

var currentReview;
var nextReview;

function carousel(forward = true) {
  currentReview = shownReviews[0];
  if (forward) {
    nextReview = currentReview.nextElementSibling || currentReview.parentElement.firstElementChild;  
  }
  else {
    nextReview = currentReview.previousElementSibling || currentReview.parentNode.lastElementChild;   
  }
  currentReview.classList.remove("show");
  nextReview.classList.add("show");
}

leftArrow.onclick = carousel.bind(null, false);

rightArrow.onclick = carousel;

* {
  font-family: Arial;
}

.carousel {
  display: flex;
  align-items: center;
  justify-content: center;
}

.review {
  display: none;
  text-align: center;
}

.show {
  display: block;
}

.arrow {
  border: 10px solid transparent;
  width: 0;
  height: 0;
  margin: 0 25vw;
}

#arrow-left {
  border-right: 10px solid black;
}

#arrow-right {
  border-left: 10px solid black;
}

<div class="carousel">
  <div class="arrow" id="arrow-left"></div>
  <div class="reviews">
    <div class="review show">
      <h1 class="title">Title1</h1>
    </div>
    <div class="review">
      <h1 class="title">Title2</h1>
    </div>
    <div class="review">
      <h1 class="title">Title3</h1>
    </div>
  </div>
  <div class="arrow" id="arrow-right"></div>
</div>

Answer 2

Put space around control structures & label some closing braces, IMO if > 2 consecutive closing braces then start labeling - about every 3rd one.

for (var i = 0; i < reviews.length; i++) {

  if (reviews[i].classList.contains("show")) {
    currentReview = reviews[i];

    if (direction == 'forward') {

      if (i + 1 > reviews.length - 1) {
        nextReview = reviews[0];
      } else {
        nextReview = reviews[i + 1];
      }

    } else {

    if (i - 1 < 0) {
      nextReview = reviews[reviews.length - 1];
    } else {
      nextReview = reviews[i - 1];
    }

  } // if direction

}

---

Logic nesting is too much. When I read that final else Im saying "else what? Where am I?" Too many ifs is bad enough, with if/else code clarity is out the window and bug potential explodes.

for (var i = 0; i < reviews.length; i++) {

   switch(direction) {
     case 'forward':
       // your code here
       break;

     case 'backward' :
       // your code here
       break;

     default :
        alert(`direction "${direction}" is invalid`);
   } // switch
} 

switch goodness:

• encourages use of a default. Get in the habit of writing error trapping.
• Your code is "forward, or anything not forward" -> in contrast this is "forward", "backward", "anything else is a mistake".
• Explicitly coding for all conditions unambiguously tells the reader what's what.
• Extensible. Adding another condition is easy. In contract the nested if/else is very highly error prone. And you can imagine that switch complexity does not compound like if/else.
• All the above makes it an ideal place for your general dispatching.

---

Given separate event handlers code can be simpler because a parameter is not required and code is greatly simplified. The for loop is unnecessary. Note that currentReview, nextReview are now indexes, not the objects themselves - which actually means only one of these is needed. There may be some redundant code for showing & hiding but the simplicity is very compelling.

function forward() {
   nextReview = currentReview >= classList.length - 1? 0 : ++currentReview;
   // reviews[nextReview] ....
}

function backward() {
   nextReview = currentReview <= 0 ? classList.length - 1 : --currentReview;
   // you know what to do here
}

Answer 3

DRY Code

Your carousal is a circular construction. We can see that in your code you perform a modular incrementation/decrementation. There is a DRY way to write this code with the use of the % operator. Also, use let and const instead of var. The latter is scoped broader than you might expect.

Let's get rid of this redundant code:

      // ..
      if (direction == 'forward') {
        if (i + 1 > reviews.length - 1) {
          nextReview = reviews[0];
        } else {
          nextReview = reviews[i + 1];
        }
      } else {
        if (i - 1 < 0) {
          nextReview = reviews[reviews.length - 1];
        } else {
          nextReview = reviews[i - 1];
        }
      }
      // ..

Modulo Arithmic

Refactored using modulo arithmic. (i + offset + reviews.length) % reviews.length makes sure you remain within bounds, both when decrementing before the beginning and incrementing after the end. Also note that it is common to use i, j to indicate indices. If you don't like this, you could use respectively currentIndex and newIndex instead.

function carousel(direction) {
  const offset = direction == 'forward' ? 1 : -1;
  for (let i = 0; i < reviews.length; i++) {
    if (reviews[i].classList.contains("show")) {
      currentReview = reviews[i];
      const j = (i + offset + reviews.length) % reviews.length;
      nextReview = reviews[j];
    }
  }

  currentReview.classList.remove("show");
  nextReview.classList.add("show");
}

Method findIndex

Also note that you only change the class "show" from the last occurence of currentReview in the loop above. I am not sure whether this is as designed (because maybe only 1 instance can be shown?) or a bug..

If only and exactly 1 item can be shown, you could rewrite the function to take use of findIndex.

function carousel(direction) {
  const offset = direction == 'forward' ? 1 : -1;
  const i = reviews.findIndex(r => r.classList.contains("show"));
  const j = (i + offset + reviews.length) % reviews.length;
  reviews[i].classList.remove("show");
  reviews[j].classList.add("show");
}

Parameters

The last thing I would argue is the use of a readable string, rather than directly using the int value: const offset = direction == 'forward' ? 1 : -1;.

const backward = -1;
const forward = 1;

leftArrow.onclick = function() {
  carousel(backward);
}
rightArrow.onclick = function() {
  carousel(forward);
}