2
\$\begingroup\$

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.

\$\endgroup\$
3
\$\begingroup\$

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;
\$\endgroup\$
  • \$\begingroup\$ You use modular arithmic twice but did you perhaps mean arithmetic? \$\endgroup\$ – t3chb0t Sep 3 '19 at 18:34
  • \$\begingroup\$ Of course, I put that one in for you to find... why else ;-) \$\endgroup\$ – dfhwze Sep 3 '19 at 18:37
  • \$\begingroup\$ I guess it's a side-effect of that kotlin app about music notes some times ago :-] \$\endgroup\$ – t3chb0t Sep 3 '19 at 18:37
  • \$\begingroup\$ nesting ternaries 😬 \$\endgroup\$ – Luke Robertson Sep 4 '19 at 0:04
  • \$\begingroup\$ @LukeRobertson some like it, some don't :) I find this one to be readable. \$\endgroup\$ – dfhwze Sep 4 '19 at 9:12

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.