javascript carousel

Question 1

I made this javascript code for a simple slider that can be controlled by arrows up/down.

I'm wondering what I could have done better..

document.addEventListener('keydown', changeSlide);
var wrap = document.querySelector('#wrap');
var slides = wrap.getElementsByTagName('div');
var slidesCount = wrap.children.length-1;
var i = 0;
slides[i].classList.add('active');
document.querySelector('#current').innerHTML += i+1;
document.querySelector('#total').innerHTML += slidesCount+1;
function changeSlide(e) {
 if (e.keyCode == 38) {
 if(i >= slidesCount) {
 i = 0;
 } else(
 i++
 )
 } else if (e.keyCode == 40) {
 if(i <= 0) {
 i = slidesCount;
 } else (
 i--
 )
 }
 for (let index = 0; index < slidesCount+1; index++) {
 slides[index].classList.remove('active'); 
 }
 slides[i].classList.add('active');
 document.querySelector('#current').innerHTML = (i+1);
}

Here is event html:

<body>
 <link rel = "stylesheet" href = "style.css"/>
 <div id = "wrap">
 <div>
 <h3>Title</h3>
 <img src="https://upload.wikimedia.org/wikipedia/commons/thumb/1/19/Ryan_ten_Doeschate_ODI_batting_graph.svg/1216px-Ryan_ten_Doeschate_ODI_batting_graph.svg.png" alt="">
 </div>
 <div>
 <h3>Title</h3>
 <img src="https://pmpaspeakingofprecision.files.wordpress.com/2013/06/ismmay2013.jpg" alt="">
 </div>
 <div >
 <h3>Title</h3>
 <img src="https://www.rba.gov.au/speeches/2016/images/sp-so-2016年07月12日-graph3.gif" alt="">
 </div>
 <div >
 <h3>Title</h3>
 <img src="https://www.rbnz.govt.nz/-/media/ReserveBank/Images/Key%20graphs/key-graph-mortgage-rates-since-1990.jpg?la=en&hash=B224DD2C76E0B0C2DDC85D3A7D3E9A60B7A73ACC" alt="">
 </div>
 <div >
 <h3>Title</h3>
 <img src="https://realtechwater.com/w_p/wp-content/uploads/2018/09/BOD-Graph-Website.jpg" alt="">
 </div>
 </div>
 <div id = counter>
 <span id="current"></span>
 <span>of</span>
 <span id="total"></span>
 </div>
 <script src="slider.js"></script>
</body>

And a little bit of css:

img {
 display: block;
 width: 100%;
}
#wrap > div {
 display: none;
}
#counter {
 position: absolute;
 bottom: 5px;
 right: 5px;
}
.active {
 display: block!important;
}

For those who like codepen, here is the link.

Question 2

The main part of code I would focus on is this part with high branch complexity:

if (e.keyCode == 38) {
 if(i >= slidesCount) {
 i = 0;
 } else(
 i++
 )
} else if (e.keyCode == 40) {
 if(i <= 0) {
 i = slidesCount;
 } else (
 i--
 )
}

When you realise a carousel has circular navigation, you could take advantage of modular arithmetic. Note that you could then just use the actual count, without the -1 'hack'.

const slidesCount = wrap.children.length;

Refactored using modular arithmetic:

if (e.keyCode == 38) {
 i = (i + 1) % slidesCount;
} else if (e.keyCode == 40) {
 i = (i - 1 + slidesCount) % slidesCount;
}

This could further be refactored using a bi-directional formula:

const phase = e.keyCode == 38 ? 1 : e.keyCode == 40 ? -1 : 0;
i = ((i + phase) % slidesCount + slidesCount) % slidesCount;

When initialising the carousel, you activate the first slide. This exact code is repeated in changeSlide.

slides[i].classList.add('active');
document.querySelector('#current').innerHTML += i+1;

Consider providing a method activate(index), which should also include the code for deactivating the other sliders.

Prefer the use of let and const over var. The scope and intention of these keywords are better than the ol' var.

var wrap = document.querySelector('#wrap');
var slides = wrap.getElementsByTagName('div');
var slidesCount = wrap.children.length-1;
var i = 0;

const wrap = document.querySelector('#wrap');
const slides = wrap.getElementsByTagName('div');
const slidesCount = wrap.children.length;
let index = 0;

Question 3

You use modular arithmic twice but did you perhaps mean arithmetic?

Question 4

Of course, I put that one in for you to find... why else ;-)

Question 5

I guess it's a side-effect of that kotlin app about music notes some times ago :-]

Question 6

nesting ternaries 😬

Question 7

@LukeRobertson some like it, some don't :) I find this one to be readable.

dfhwze dfhwze 14.1k3 gold badges40 silver badges101 bronze badges · Answer 1 · 2019-09-02 19:34:08Z

The main part of code I would focus on is this part with high branch complexity:

if (e.keyCode == 38) {
 if(i >= slidesCount) {
 i = 0;
 } else(
 i++
 )
} else if (e.keyCode == 40) {
 if(i <= 0) {
 i = slidesCount;
 } else (
 i--
 )
}

When you realise a carousel has circular navigation, you could take advantage of modular arithmetic. Note that you could then just use the actual count, without the -1 'hack'.

const slidesCount = wrap.children.length;

Refactored using modular arithmetic:

if (e.keyCode == 38) {
 i = (i + 1) % slidesCount;
} else if (e.keyCode == 40) {
 i = (i - 1 + slidesCount) % slidesCount;
}

This could further be refactored using a bi-directional formula:

const phase = e.keyCode == 38 ? 1 : e.keyCode == 40 ? -1 : 0;
i = ((i + phase) % slidesCount + slidesCount) % slidesCount;

When initialising the carousel, you activate the first slide. This exact code is repeated in changeSlide.

slides[i].classList.add('active');
document.querySelector('#current').innerHTML += i+1;

Consider providing a method activate(index), which should also include the code for deactivating the other sliders.

Prefer the use of let and const over var. The scope and intention of these keywords are better than the ol' var.

var wrap = document.querySelector('#wrap');
var slides = wrap.getElementsByTagName('div');
var slidesCount = wrap.children.length-1;
var i = 0;

const wrap = document.querySelector('#wrap');
const slides = wrap.getElementsByTagName('div');
const slidesCount = wrap.children.length;
let index = 0;

You use modular arithmic twice but did you perhaps mean arithmetic?
Of course, I put that one in for you to find... why else ;-)
I guess it's a side-effect of that kotlin app about music notes some times ago :-]
@LukeRobertson some like it, some don't :) I find this one to be readable.

Stack Exchange Network

javascript carousel

1 Answer 1

Your Answer

Sign up or log in

Post as a guest

Post as a guest

Hot Network Questions

javascript carousel

1 Answer 1

Your Answer

Sign up or log in

Post as a guest

Post as a guest

Related

Hot Network Questions