# Image Carousel in JavaScript

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 */
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";
}
return;
}
}
}


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 = 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
}

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.