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

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.