Javascript image carousel

Question

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>

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

Gerrit0 · Accepted Answer · 2018-02-08 20:58:32Z

There are several problems with your code right now.

Don't create global variables. nextIndex and prevIndex are both globals that are implicitly created when you first assign to them.
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;
}
```
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').
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!)
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:

You can just add another image-slider div wherever you want another set of images.
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>

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 :( — mohsen hasani
– mohsen hasani, Commented Feb 9, 2018 at 9:12
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. — Gerrit0
– Gerrit0, Commented Feb 9, 2018 at 16:17

Stack Exchange Network

Javascript image carousel

1 Answer 1

You must log in to answer this question.

Hot Network Questions

Javascript image carousel

1 Answer 1

You must log in to answer this question.

Related

Hot Network Questions