Image Carousel in JavaScript

Question

I created a very basic "image projector" that shows the current one and shows the next on clicking into the image.

And yeah, it works, but I don't think that's a very elegant way to do it. Any advice please?

Bear in mind I'm just starting my JavaScript classes and that's my third assignment, but the teacher is not really helpful when it comes to clean code, for him if it works, it works :/

Anyway, here's the full code: http://jsfiddle.net/AngelCJ/F5Hg7/

And the messy part:

/* loads the first image of the ul element using the n counter */
window.onload = function loadFirst() {
 var n=0;
 document.getElementById("list").children[n].style.display="block";
 /* onclick event, add +1 to the counter */
 document.getElementById("btn").onclick = function slider(){
 n++;
 /* and display the corresponding image */
 document.getElementById("list").children[n].style.display="block";
 /* while deleteing the previous one */
 document.getElementById("list").children[n-1].style.display="none";
 /* if we reach the final image */
 if(n==3) {
 /* shows the final image */
 document.getElementById("list").children[3].style.display="block";
 /* and after we click on it */
 document.getElementById("btn").onclick = function slider(){
 /* deletes it to leave the space to the first one */
 document.getElementById("list").children[3].style.display="none";
 /* returning to it loading the function to start again */
 loadFirst();
 }
 return;
 }
 }
}

Answer 1

In your example you're adding events inside events, this is not good idea because it creates unnecessary closures and can lead to memory leaks.

First I would cache the elements so you only query the DOM once, improving performance. Then you can create a function to hide all images, and a function to go to the next image, and reuse that function for both events:

// IIFE (Immediately invoked function expression)
// to not leak variables to global scope
(function(){
 // Cache elements
 var list = document.getElementById('list');
 var btn = document.getElementById('btn');
 var current = 0;
 // Hide all images
 function hide() {
 for (var i=0; i<list.children.length; i++) {
 list.children[i].style.display = 'none';
 }
 }
 // Go to next image
 function next() {
 hide();
 list.children[current++].style.display = 'block';
 // Did we reach the end?
 if (current >= list.children.length) {
 current = 0;
 }
 }
 // Reuse function
 window.onload = btn.onclick = next;
}());

Demo: http://jsfiddle.net/F5Hg7/2/

Answer 2

window.onload = function loadFirst() {
 var n = 0; //Initial counter 
 var images = document.getElementById("list").children; //get length of images carousel
 images[n].style.display = "block"; //set first image visible
 document.getElementById("btn").onclick = function slider() {
 //(n % images.length) will give previous child's index
 images[n % images.length].style.display = "none"; //set previous image invisible
 n++; //increase initial counter
 i = n % images.length; //(n%images.length) will give next child's index
 images[i].style.display = "block"; //set display visible of next image
 }
}

Fiddle

Or With IIFE

(function () {
 var images = document.getElementById("list").children; //get length of images carousel
 var n = images.length - 1; //Initial counter 
 var btn = document.getElementById("btn");
 function carousel() {
 images[n % images.length].style.display = "none"; //set previous image invisible
 n++;
 i = n % images.length;
 images[i].style.display = "block"; //set next image visible
 }
 window.onload = btn.onclick =carousel;
})();

fiddle

• amon: Could you edit your answer to explain what you've improved and why you did this? As it stands, this is a code dump, not a code review.
• Vicky Gonsalves: Apologies..I m new to this site..Kindly suggest if my answer is appropriate..Thanks

Answer 3

Don't use the onXXX properties to assign events. This only allows you to attach one listener to an event, so that any other scripts doing the same with overwrite each other. Use addEventLister instead.