0
\$\begingroup\$

I wrote the following code for creating a simple image carousel, that shows multiple images at the same time in the same order, but starting at a different position when the "next" or "prev" button is clicked.

This code works, but does it contradict programming principles and algorithms?

Is there a better solution?

var images = [
   "1.jpg", 
   "2.jpg", 
   "3.jpg"
];
let slider = document.getElementById("slider");
function next(){
  images.forEach((item , index)=>{
    if(slider.getAttribute("src") == item)
      if(index != images.length-1)
        nextIndex = index+1;
      else
       nextIndex = 0;
    });
   slider.setAttribute("src",images[nextIndex]);  
}
function prev(){
  images.forEach((item , index)=>{
    if(slider.getAttribute("src") == item)
      if(index != 0)
         prevIndex = index-1;
      else
        prevIndex = images.length-1;
    });
   slider.setAttribute("src",images[prevIndex]);  
}
<div>
  <button onClick = "prev()"> Prev </button>
  <img id="slider" src="1.jpg" 
    width="200px" height="100px"/>
  <button onClick = "next()"> Next </button>
</div>

\$\endgroup\$
2
  • 1
    \$\begingroup\$ The name "Slider" is not entirely correct for this, as it does not provide animation. \$\endgroup\$
    – TimSparrow
    Feb 8, 2018 at 12:09
  • \$\begingroup\$ Consider preloading images, as a user will see them loading when they press buttons, which is not good, especially on slow connections \$\endgroup\$
    – TimSparrow
    Feb 8, 2018 at 12:10

1 Answer 1

3
\$\begingroup\$

There are several problems with your code right now.

  1. Don't create global variables. nextIndex and prevIndex are both globals that are implicitly created when you first assign to them.

  2. Don't abuse not using braces on if statements. It is fine to skip them for a very simple statement (assuming it is allowed in your style guide) but nesting multiple ifs without braces is a recipe for disaster.

    // Much safer 
    if (slider.getAttribute("src") == item) {
      if(index != images.length-1)
        nextIndex = index+1;
      else
       nextIndex = 0;
    }
    
  3. Don't use setAttribute / getAttribute if you are accessing a standard property on an element. slider.src is shorter and easier to read than slider.getAttribute('src').

  4. Consider making your code more modular. What happens if you now want two sliders on the page? Ideally, it should just be another line of code to add the functionality (or maybe even no more lines of code!)

  5. Consider preloading images. TimSparrow mentioned this in the comments - with your current code the next image won't be loaded until the user clicks on the next button. This can result in a bad experience on slow connections.


There are a few more things I could say, but I believe it would be more helpful to just provide an alternative implementation.

This implementation has a couple advantages:

  1. You can just add another image-slider div wherever you want another set of images.
  2. By using an internal hidden div for storing the images, you automatically get preloaded images.

function createSlider(container) {
  const images = container.querySelectorAll('.images > img');

  const displayImage = container.querySelector('[data-display]')

  // State
  let imageIndex;
  
  function setImage(index) {
    imageIndex = index;
    displayImage.src = images[index].src;
  }

  // Show the first image initially
  setImage(0);

  // Button listener
  container.addEventListener('click', event => {
    if (event.target.hasAttribute('data-prev')) {
      setImage(imageIndex === 0 ? images.length - 1 : imageIndex - 1)
    } else if (event.target.hasAttribute('data-next')) {
      setImage(imageIndex === images.length - 1 ? 0 : imageIndex + 1)
    }
  });
}

document.querySelectorAll('.image-slider').forEach(createSlider);
.image-slider .images {
  display: none;
}
<div class="image-slider">
  <div class="images">
    <img src="https://placeimg.com/200/100/animals">
    <img src="https://placeimg.com/200/100/arch">
    <img src="https://placeimg.com/200/100/nature">
  </div>
  <button data-prev>Prev</button>
  <img data-display width="200px" height="100px">
  <button data-next>Next</button>
</div>

\$\endgroup\$
2
  • \$\begingroup\$ thanks , it's really very good . but i don't know source of {container} that placed as a argument in create slider function ; it's same document object? how? forgive me i am a beginner :( \$\endgroup\$ Feb 9, 2018 at 9:12
  • \$\begingroup\$ The container element is the div with the class image-slider in the html, take a look at what document.querySelectorAll('.image-slider') prints when you execute it in the dev console. \$\endgroup\$
    – Gerrit0
    Feb 9, 2018 at 16:17

Your Answer

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

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