Ttykuu

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>

JSFiddle

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