trjhtr

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>

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>