patternjavascriptMinor

Simple jQuery photo slider

Submitted by: @import:stackexchange-codereview·Mar 10, 2026·

Viewed 0 times

simplejquerysliderphoto

Problem

I'm fairly new to front end web development, but finally felt confident enough to create my own jQuery slider. I wish to receive constructive criticism about my code, how I can optimize any line of code, or ideas on how I can make this a bit more interesting.

`$(document).ready(function(){
/*
=====================================
THE SLIDER SCRIPT STARTS HERE
=====================================
*/
var i = 0;
var images = ["Slide/bandera_OK%202.png","Slide/CARAVANA_carta.png","Slide/KANGOO.png","Slide/mochila.png","Slide/RADICAL_OH_03_alta.png","Slide/SILVER_final.png"];

setInterval(function(){
var topDiv = $('[data-slide='+'top'+']');
var topImg = $('[data-slide='+'top'+'] > img');
var bottomDiv = $('[data-slide='+'bottom'+']');
var bottomImg = $('[data-slide='+'bottom'+'] > img');

i++;

if(i == images.length){
i = 0;
bottomDiv.animate({left:'100%'},1500);
topImg.attr('src',images[i]);
setTimeout(function(){
bottomDiv.css('z-index','0');
topDiv.css('z-index','1');
},2000);

setTimeout(function(){
bottomDiv.css('left','0px');
bottomImg.attr('src',images[i+1]);
},2500);

}
else if(i % 2 != 0){
topDiv.animate({left:'100%'},1500);
bottomImg.attr('src',images[i]);
setTimeout(function(){
topDiv.css('z-index','0');
bottomDiv.css('z-index','1');
},2000);

setTimeout(function(){
topDiv.css('left','0px');
topImg.attr('src',images[i+1]);
},2500);
}
else if(i % 2 == 0){
bottomDiv.animate({left:'100%'},1500);
topImg.attr('src',images[i]);
setTimeout(function(){

Solution

The first big things I got on your code, are:

-
you could define a function instead of passing an anonymous one to setInterval

-
you could move the dom serarch on the main scope to avoid the repetition on each iteration.

-
you could optimize the setTimeout function too, defining one on main scope and refer to a variable like selectedImage, instead of using the array.

Defining function, instead of using anonymous on the fly, help to avoid repeating yourself.

The "on the fly anonymous" are very good to cut off the development time, of for very short code or for examples.

But in production, it's better to came back and refactoring it.

Here how I would refactoring:

$(document).ready(function(){
        /*
        =====================================
            THE SLIDER SCRIPT STARTS HERE 
        =====================================
        */
        var i = 0; 
        var images = ["Slide/bandera_OK%202.png","Slide/CARAVANA_carta.png","Slide/KANGOO.png","Slide/mochila.png","Slide/RADICAL_OH_03_alta.png","Slide/SILVER_final.png"];
        var topDiv = $('[data-slide='+'top'+']');
        var topImg = $('[data-slide='+'top'+'] > img');
        var bottomDiv = $('[data-slide='+'bottom'+']');
        var bottomImg = $('[data-slide='+'bottom'+'] > img');
        var selectedImage = null;

        function mySlider() {

            i++;

            if(i == images.length){
                i = 0;
                bottomDiv.animate({left:'100%'},1500);
                topImg.attr('src',images[i]);
                setTimeout(raiseTopDiv,2000);

                seletectImage = images[i+1];

                setTimeout(showSelectedImage,2500);

            }
            else if(i % 2 != 0){
                topDiv.animate({left:'100%'},1500);
                bottomImg.attr('src',images[i]);
                setTimeout(raiseBottomDiv,2000);

                seletectImage = images[i+1];

                setTimeout(showSelectedImage,2500);
            }
            else if(i % 2 == 0){
                bottomDiv.animate({left:'100%'},1500);
                topImg.attr('src',images[i]);

                setTimeout(raiseTopDiv,2000);

                seletectImage = images[i+1];

                setTimeout(showSelectedImage,2500)

            }

        }

        function raiseTopDiv() {
            bottomDiv.css('z-index','0');
            topDiv.css('z-index','1');
        }

        function raiseBottomDiv() {
            topDiv.css('z-index','0');
            bottomDiv.css('z-index','1');
        }

        function showSelectedImage() {
            bottomDiv.css('left','0px');
            bottomImg.attr('src',seletectImage);
        }

        setInterval(mySlider,3000);

    });

Code Snippets

$(document).ready(function(){
        /*
        =====================================
            THE SLIDER SCRIPT STARTS HERE 
        =====================================
        */
        var i = 0; 
        var images = ["Slide/bandera_OK%202.png","Slide/CARAVANA_carta.png","Slide/KANGOO.png","Slide/mochila.png","Slide/RADICAL_OH_03_alta.png","Slide/SILVER_final.png"];
        var topDiv = $('[data-slide='+'top'+']');
        var topImg = $('[data-slide='+'top'+'] > img');
        var bottomDiv = $('[data-slide='+'bottom'+']');
        var bottomImg = $('[data-slide='+'bottom'+'] > img');
        var selectedImage = null;

        function mySlider() {

            i++;

            if(i == images.length){
                i = 0;
                bottomDiv.animate({left:'100%'},1500);
                topImg.attr('src',images[i]);
                setTimeout(raiseTopDiv,2000);

                seletectImage = images[i+1];

                setTimeout(showSelectedImage,2500);


            }
            else if(i % 2 != 0){
                topDiv.animate({left:'100%'},1500);
                bottomImg.attr('src',images[i]);
                setTimeout(raiseBottomDiv,2000);

                seletectImage = images[i+1];

                setTimeout(showSelectedImage,2500);
            }
            else if(i % 2 == 0){
                bottomDiv.animate({left:'100%'},1500);
                topImg.attr('src',images[i]);

                setTimeout(raiseTopDiv,2000);

                seletectImage = images[i+1];

                setTimeout(showSelectedImage,2500)

            }

        }

        function raiseTopDiv() {
            bottomDiv.css('z-index','0');
            topDiv.css('z-index','1');
        }

        function raiseBottomDiv() {
            topDiv.css('z-index','0');
            bottomDiv.css('z-index','1');
        }

        function showSelectedImage() {
            bottomDiv.css('left','0px');
            bottomImg.attr('src',seletectImage);
        }

        setInterval(mySlider,3000);

    });

Context

StackExchange Code Review Q#134135, answer score: 2

Revisions (0)

No revisions yet.