Carousel in Vanilla JavaScript The 2019 Stack Overflow Developer Survey Results Are InImage...
Can withdrawing asylum be illegal?
Falsification in Math vs Science
Cooking pasta in a water boiler
Can we generate random numbers using irrational numbers like π and e?
What do hard-Brexiteers want with respect to the Irish border?
If I score a critical hit on an 18 or higher, what are my chances of getting a critical hit if I roll 3d20?
Why can I use a list index as an indexing variable in a for loop?
How come people say “Would of”?
Does adding complexity mean a more secure cipher?
Why isn't the circumferential light around the M87 black hole's event horizon symmetric?
Did any laptop computers have a built-in 5 1/4 inch floppy drive?
Will it cause any balance problems to have PCs level up and gain the benefits of a long rest mid-fight?
How can I add encounters in the Lost Mine of Phandelver campaign without giving PCs too much XP?
Are spiders unable to hurt humans, especially very small spiders?
What could be the right powersource for 15 seconds lifespan disposable giant chainsaw?
The difference between dialogue marks
Pokemon Turn Based battle (Python)
How much of the clove should I use when using big garlic heads?
Ubuntu Server install with full GUI
Is an up-to-date browser secure on an out-of-date OS?
Likelihood that a superbug or lethal virus could come from a landfill
Why are there uneven bright areas in this photo of black hole?
What is the motivation for a law requiring 2 parties to consent for recording a conversation
A word that means fill it to the required quantity
Carousel in Vanilla JavaScript
The 2019 Stack Overflow Developer Survey Results Are InImage Carousel in JavaScriptInteractive sliderVanilla Javascript live filterVanilla Carousel: code above configurationVanilla JavaScript ToDo List implementationJavaScript/jQuery carousel/sliderVanilla JavaScript to-do listPopup classes hierarchy designJavascript image carouselVanilla JavaScript Calculator
.everyoneloves__top-leaderboard:empty,.everyoneloves__mid-leaderboard:empty,.everyoneloves__bot-mid-leaderboard:empty{ margin-bottom:0;
}
$begingroup$
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
javascript
edited Jan 9 '18 at 21:34
iwrestledabearonce
2,032613
asked Jan 9 '18 at 20:02
Jordan BaronJordan Baron
1264
$endgroup$
bumped to the homepage by Community♦ 10 mins ago
This question has answers that may be good or bad; the system has marked it active so that they can be reviewed.
add a comment |
$begingroup$
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
javascript
edited Jan 9 '18 at 21:34
iwrestledabearonce
2,032613
asked Jan 9 '18 at 20:02
Jordan BaronJordan Baron
1264
$endgroup$
bumped to the homepage by Community♦ 10 mins ago
This question has answers that may be good or bad; the system has marked it active so that they can be reviewed.
1
$begingroup$
@Iwrestledabearonce check - sorry, I didn't try the left button. Way to be! "Sometimes You Eat the Bear, and Sometimes the Bear Eats You."
$endgroup$
– Sᴀᴍ Onᴇᴌᴀ
Jan 9 '18 at 22:02
$begingroup$
You are using the rather modernclassList, but not e.g. theletkeyword. What environments/browsers would you like to support?
$endgroup$
– Jeroen
Jan 16 '18 at 23:17
add a comment |
$begingroup$
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
javascript
edited Jan 9 '18 at 21:34
iwrestledabearonce
2,032613
asked Jan 9 '18 at 20:02
Jordan BaronJordan Baron
1264
$endgroup$
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
javascript
edited Jan 9 '18 at 21:34
iwrestledabearonce
2,032613
asked Jan 9 '18 at 20:02
Jordan BaronJordan Baron
1264
edited Jan 9 '18 at 21:34
iwrestledabearonce
2,032613
asked Jan 9 '18 at 20:02
Jordan BaronJordan Baron
1264
edited Jan 9 '18 at 21:34
iwrestledabearonce
2,032613
edited Jan 9 '18 at 21:34
iwrestledabearonce
2,032613
edited Jan 9 '18 at 21:34
iwrestledabearonce
2,032613
2,032613
asked Jan 9 '18 at 20:02
Jordan BaronJordan Baron
1264
asked Jan 9 '18 at 20:02
Jordan BaronJordan Baron
1264
asked Jan 9 '18 at 20:02
Jordan BaronJordan Baron
1264
1264
bumped to the homepage by Community♦ 10 mins 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♦ 10 mins ago
This question has answers that may be good or bad; the system has marked it active so that they can be reviewed.
1
$begingroup$
@Iwrestledabearonce check - sorry, I didn't try the left button. Way to be! "Sometimes You Eat the Bear, and Sometimes the Bear Eats You."
$endgroup$
– Sᴀᴍ Onᴇᴌᴀ
Jan 9 '18 at 22:02
$begingroup$
You are using the rather modernclassList, but not e.g. theletkeyword. What environments/browsers would you like to support?
$endgroup$
– Jeroen
Jan 16 '18 at 23:17
add a comment |
1
$begingroup$
@Iwrestledabearonce check - sorry, I didn't try the left button. Way to be! "Sometimes You Eat the Bear, and Sometimes the Bear Eats You."
$endgroup$
– Sᴀᴍ Onᴇᴌᴀ
Jan 9 '18 at 22:02
$begingroup$
You are using the rather modernclassList, but not e.g. theletkeyword. What environments/browsers would you like to support?
$endgroup$
– Jeroen
Jan 16 '18 at 23:17
1
1
$begingroup$
@Iwrestledabearonce check - sorry, I didn't try the left button. Way to be! "Sometimes You Eat the Bear, and Sometimes the Bear Eats You."
$endgroup$
– Sᴀᴍ Onᴇᴌᴀ
Jan 9 '18 at 22:02
$begingroup$
@Iwrestledabearonce check - sorry, I didn't try the left button. Way to be! "Sometimes You Eat the Bear, and Sometimes the Bear Eats You."
$endgroup$
– Sᴀᴍ Onᴇᴌᴀ
Jan 9 '18 at 22:02
$begingroup$
You are using the rather modern
classList, but not e.g. the let keyword. What environments/browsers would you like to support?$endgroup$
– Jeroen
Jan 16 '18 at 23:17
$begingroup$
You are using the rather modern
classList, but not e.g. the let keyword. What environments/browsers would you like to support?$endgroup$
– Jeroen
Jan 16 '18 at 23:17
add a comment |
1 Answer
1
active
oldest
votes
$begingroup$
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
}
answered Jan 16 '18 at 21:47
radarbobradarbob
5,4701127
$endgroup$
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',
autoActivateHeartbeat: false,
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
});
}
});
Sign up or log in
StackExchange.ready(function () {
StackExchange.helpers.onClickDraftSave('#login-link');
});
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
Post as a guest
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
Required, but never shown
1 Answer
1
active
oldest
votes
1 Answer
1
active
oldest
votes
active
oldest
votes
active
oldest
votes
$begingroup$
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
}
answered Jan 16 '18 at 21:47
radarbobradarbob
5,4701127
$endgroup$
add a comment |
$begingroup$
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
}
answered Jan 16 '18 at 21:47
radarbobradarbob
5,4701127
$endgroup$
add a comment |
$begingroup$
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
}
answered Jan 16 '18 at 21:47
radarbobradarbob
5,4701127
$endgroup$
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
}
answered Jan 16 '18 at 21:47
radarbobradarbob
5,4701127
edited Jan 16 '18 at 21:57
answered Jan 16 '18 at 21:47
radarbobradarbob
5,4701127
answered Jan 16 '18 at 21:47
radarbobradarbob
5,4701127
answered Jan 16 '18 at 21:47
radarbobradarbob
5,4701127
5,4701127
add a comment |
add a comment |
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.
Sign up or log in
StackExchange.ready(function () {
StackExchange.helpers.onClickDraftSave('#login-link');
});
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
Post as a guest
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
Required, but never shown
Sign up or log in
StackExchange.ready(function () {
StackExchange.helpers.onClickDraftSave('#login-link');
});
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
Post as a guest
Required, but never shown
Sign up or log in
StackExchange.ready(function () {
StackExchange.helpers.onClickDraftSave('#login-link');
});
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
Post as a guest
Required, but never shown
Sign up or log in
StackExchange.ready(function () {
StackExchange.helpers.onClickDraftSave('#login-link');
});
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
Post as a guest
Required, but never shown
Required, but never shown
Required, but never shown
Required, but never shown
Required, but never shown
Required, but never shown
Required, but never shown
Required, but never shown
Required, but never shown
1
$begingroup$
@Iwrestledabearonce check - sorry, I didn't try the left button. Way to be! "Sometimes You Eat the Bear, and Sometimes the Bear Eats You."
$endgroup$
– Sᴀᴍ Onᴇᴌᴀ
Jan 9 '18 at 22:02
$begingroup$
You are using the rather modern
classList, but not e.g. theletkeyword. What environments/browsers would you like to support?$endgroup$
– Jeroen
Jan 16 '18 at 23:17