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.

Improve this question
\$\endgroup\$

1 Answer 1

Reset to default
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;
Improve this answer
\$\endgroup\$
5
  • \$\begingroup\$ You use modular arithmic twice but did you perhaps mean arithmetic? \$\endgroup\$
    – t3chb0t
    Sep 3, 2019 at 18:34
  • \$\begingroup\$ Of course, I put that one in for you to find... why else ;-) \$\endgroup\$
    – dfhwze
    Sep 3, 2019 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, 2019 at 18:37
  • \$\begingroup\$ nesting ternaries 😬 \$\endgroup\$
    – Luke Robertson
    Sep 4, 2019 at 0:04
  • \$\begingroup\$ @LukeRobertson some like it, some don't :) I find this one to be readable. \$\endgroup\$
    – dfhwze
    Sep 4, 2019 at 9:12

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge that you have read and understand our privacy policy and code of conduct.

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