Carousel in Vanilla JavaScript

up vote
4
down vote

favorite

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

edited Jan 9 at 21:34

Occam's Razor

1,987513

asked Jan 9 at 20:02

Jordan Baron

1264

bumped to the homepage by Community♦ 16 hours ago

This question has answers that may be good or bad; the system has marked it active so that they can be reviewed.

1

@Iwrestledabearonce check - sorry, I didn't try the left button. Way to be! "Sometimes You Eat the Bear, and Sometimes the Bear Eats You."
– Sᴀᴍ Onᴇᴌᴀ
Jan 9 at 22:02

You are using the rather modern classList, but not e.g. the let keyword. What environments/browsers would you like to support?
– Jeroen
Jan 16 at 23:17

add a comment |

up vote
4
down vote

favorite

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

edited Jan 9 at 21:34

Occam's Razor

1,987513

asked Jan 9 at 20:02

Jordan Baron

1264

bumped to the homepage by Community♦ 16 hours ago

This question has answers that may be good or bad; the system has marked it active so that they can be reviewed.

1

@Iwrestledabearonce check - sorry, I didn't try the left button. Way to be! "Sometimes You Eat the Bear, and Sometimes the Bear Eats You."
– Sᴀᴍ Onᴇᴌᴀ
Jan 9 at 22:02

You are using the rather modern classList, but not e.g. the let keyword. What environments/browsers would you like to support?
– Jeroen
Jan 16 at 23:17

add a comment |

up vote
4
down vote

favorite

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

edited Jan 9 at 21:34

Occam's Razor

1,987513

asked Jan 9 at 20:02

Jordan Baron

1264

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

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>

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>

javascript

edited Jan 9 at 21:34

Occam's Razor

1,987513

asked Jan 9 at 20:02

Jordan Baron

1264

edited Jan 9 at 21:34

Occam's Razor

1,987513

asked Jan 9 at 20:02

Jordan Baron

1264

edited Jan 9 at 21:34

Occam's Razor

1,987513

edited Jan 9 at 21:34

Occam's Razor

1,987513

edited Jan 9 at 21:34

Occam's Razor

1,987513

asked Jan 9 at 20:02

Jordan Baron

1264

asked Jan 9 at 20:02

Jordan Baron

1264

asked Jan 9 at 20:02

Jordan Baron

1264

bumped to the homepage by Community♦ 16 hours ago

This question has answers that may be good or bad; the system has marked it active so that they can be reviewed.

bumped to the homepage by Community♦ 16 hours ago

This question has answers that may be good or bad; the system has marked it active so that they can be reviewed.

1

@Iwrestledabearonce check - sorry, I didn't try the left button. Way to be! "Sometimes You Eat the Bear, and Sometimes the Bear Eats You."
– Sᴀᴍ Onᴇᴌᴀ
Jan 9 at 22:02

You are using the rather modern classList, but not e.g. the let keyword. What environments/browsers would you like to support?
– Jeroen
Jan 16 at 23:17

add a comment |

1

@Iwrestledabearonce check - sorry, I didn't try the left button. Way to be! "Sometimes You Eat the Bear, and Sometimes the Bear Eats You."
– Sᴀᴍ Onᴇᴌᴀ
Jan 9 at 22:02

You are using the rather modern classList, but not e.g. the let keyword. What environments/browsers would you like to support?
– Jeroen
Jan 16 at 23:17

@Iwrestledabearonce check - sorry, I didn't try the left button. Way to be! "Sometimes You Eat the Bear, and Sometimes the Bear Eats You."
– Sᴀᴍ Onᴇᴌᴀ
Jan 9 at 22:02

You are using the rather modern classList, but not e.g. the let keyword. What environments/browsers would you like to support?
– Jeroen
Jan 16 at 23:17

add a comment |

1 Answer
1

active

oldest

votes

up vote
0
down vote

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

}

edited Jan 16 at 21:57

answered Jan 16 at 21:47

radarbob

5,2741025

add a comment |

Your Answer

StackExchange.ifUsing("editor", function () {
return StackExchange.using("mathjaxEditing", function () {
StackExchange.MarkdownEditor.creationCallbacks.add(function (editor, postfix) {
StackExchange.mathjaxEditing.prepareWmdForMathJax(editor, postfix, [["\$", "\$"]]);
});
});
}, "mathjax-editing");

StackExchange.ifUsing("editor", function () {
StackExchange.using("externalEditor", function () {
StackExchange.using("snippets", function () {
StackExchange.snippets.init();
});
});
}, "code-snippets");

StackExchange.ready(function() {
var channelOptions = {
tags: "".split(" "),
id: "196"
};
initTagRenderer("".split(" "), "".split(" "), channelOptions);

StackExchange.using("externalEditor", function() {
// Have to fire editor after snippets, if snippets enabled
if (StackExchange.settings.snippets.snippetsEnabled) {
StackExchange.using("snippets", function() {
createEditor();
});
}
else {
createEditor();
}
});

function createEditor() {
StackExchange.prepareEditor({
heartbeatType: 'answer',
convertImagesToLinks: false,
noModals: true,
showLowRepImageUploadWarning: true,
reputationToPostImages: null,
bindNavPrevention: true,
postfix: "",
imageUploader: {
brandingHtml: "Powered by u003ca class="icon-imgur-white" href="https://imgur.com/"u003eu003c/au003e",
contentPolicyHtml: "User contributions licensed under u003ca href="https://creativecommons.org/licenses/by-sa/3.0/"u003ecc by-sa 3.0 with attribution requiredu003c/au003e u003ca href="https://stackoverflow.com/legal/content-policy"u003e(content policy)u003c/au003e",
allowUrls: true
},
onDemand: true,
discardSelector: ".discard-answer"
,immediatelyShowMarkdownHelp:true
});

}
});

draft saved

draft discarded

Sign up or log in

StackExchange.ready(function () {
StackExchange.helpers.onClickDraftSave('#login-link');
});

Post as a guest

Name

Required, but never shown

StackExchange.ready(
function () {
StackExchange.openid.initPostLogin('.new-post-login', 'https%3a%2f%2fcodereview.stackexchange.com%2fquestions%2f184681%2fcarousel-in-vanilla-javascript%23new-answer', 'question_page');
}
);

Post as a guest

Name

Required, but never shown

1 Answer
1

active

oldest

votes

1 Answer
1

active

oldest

votes

up vote
0
down vote

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.

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

}

edited Jan 16 at 21:57

answered Jan 16 at 21:47

radarbob

5,2741025

add a comment |

up vote
0
down vote

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.

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

}

edited Jan 16 at 21:57

answered Jan 16 at 21:47

radarbob

5,2741025

add a comment |

up vote
0
down vote

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.

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

}

edited Jan 16 at 21:57

answered Jan 16 at 21:47

radarbob

5,2741025

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.

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

}

edited Jan 16 at 21:57

answered Jan 16 at 21:47

radarbob

5,2741025

edited Jan 16 at 21:57

answered Jan 16 at 21:47

radarbob

5,2741025

answered Jan 16 at 21:47

radarbob

5,2741025

answered Jan 16 at 21:47

radarbob

5,2741025

add a comment |

draft saved

draft discarded

Thanks for contributing an answer to Code Review Stack Exchange!

Please be sure to answer the question. Provide details and share your research!

But avoid …

Asking for help, clarification, or responding to other answers.

Making statements based on opinion; back them up with references or personal experience.

Use MathJax to format equations. MathJax reference.

To learn more, see our tips on writing great answers.

Some of your past answers have not been well-received, and you're in danger of being blocked from answering.

Please pay close attention to the following guidance:

Please be sure to answer the question. Provide details and share your research!

But avoid …

Asking for help, clarification, or responding to other answers.

Making statements based on opinion; back them up with references or personal experience.

To learn more, see our tips on writing great answers.

draft saved

draft discarded

Sign up or log in

StackExchange.ready(function () {
StackExchange.helpers.onClickDraftSave('#login-link');
});

Post as a guest

Name

Required, but never shown

Post as a guest

Name

Required, but never shown

Sign up or log in

StackExchange.ready(function () {
StackExchange.helpers.onClickDraftSave('#login-link');
});

Post as a guest

Name

Required, but never shown

Sign up or log in

StackExchange.ready(function () {
StackExchange.helpers.onClickDraftSave('#login-link');
});

Post as a guest

Name

Required, but never shown

Sign up or log in

StackExchange.ready(function () {
StackExchange.helpers.onClickDraftSave('#login-link');
});

Post as a guest

Name

Required, but never shown

Name

Required, but never shown

Name

Required, but never shown

This page is only for reference, If you need detailed information, please check here

搜尋此網誌

Krdytkyu