0

First Question on this site so I hope I do this right! I have a javascript function that I want to display an image (image1.jpg) when the page is loaded, and then every 2 seconds change the image by going through the loop. However only the first image is showing so it seems the JS function is not being called. Can anyone tell me if I am doing something wrong here because it looks fine to me so can't understand why it won't work. Thanks

<html>
    <head>      
        <script type="text/javascript">
            function displayImages(){
                var images = ['image1.jpg', 'image2.jpg', 'image3.jpg'];
                var i = 1;

                if(i>images.length-1){
                    this.src=images[0];
                    i=1;
                }else{
                    this.src=images[i];
                    i++;
                }
                setTimeout("displayImages()", 2000);
            }
        </script>
    </head>

    <body onload="displayImages();">
        <img id="myButton" src="image1.jpg" />
    </body>
</html>
Improve this question
5
  • 1
    Did you try putting an alert() or console.log() in the function to see if it reaches the function? Commented Apr 24, 2014 at 9:11
  • You do not need the semicolon on 'onload', after the displayImages(). Commented Apr 24, 2014 at 9:12
  • Works for me jsfiddle.net/acA7W Commented Apr 24, 2014 at 9:13
  • I see that none of the answers is still accepted. There were issues with mine but now I got it working. Answer includes link to the jsfiddle. Take a look. Commented Apr 24, 2014 at 10:23
  • This question is similar to: What is the scope of variables in JavaScript?. If you believe it’s different, please edit the question, make it clear how it’s different and/or how the answers on that question are not helpful for your problem. Commented Oct 9, 2024 at 14:57

7 Answers 7

Reset to default
7

You need to move the line

var i = 1;

outside the displayImages -function or it will start from one each time!

EDIT: But using a global variable is not considered good practice, so you could use closures instead. Also as noted in other answers, you are referencing this which does not refer to the image object, so I corrected that as well as simplified the logic a bit:

<script type="text/javascript">
 function displayImages( i ){
   var images = ['image1.jpg', 'image2.jpg', 'image3.jpg'];
   var img = document.getElementById('myButton');
   img.src = images[i];
   i = (i+1) % images.length;
   setTimeout( function() { displayImages(i); }, 2000 );
}
</script>
<body onload="displayImages(0);">
Improve this answer
Sign up to request clarification or add additional context in comments.

3 Comments

It would be useful if you describe how to do this without making i global, ie using a closure.
+1 for using closures and anonymous function. However I would like to read your opinion about my solution.
Your solution @ElmoVanKielmo, might be hard to read for those not used to assigning extra properties to a function object. I have never written any code like that. It is probably a matter of personal taste though. Any potential difference in performance probably doesn't matter in such a simple case..
3

You need the value of i to be available at each call, it can be kept in a closure using something like:

var displayImages = (function() {
  var i = 0;
  var images = ['image1.jpg', 'image2.jpg', 'image3.jpg'];
  return function() {
    document.getElementById('myButton').src = images[i++ % images.length];
    setTimeout(displayImages, 2000);
  }
}());

Also, since this isn't set by the call, it will default to the global/window object, so you need to get a reference to the image. That too could be held in a closure.

Improve this answer

2 Comments

This will solve the problem. May be its better to reset the 'i' value after calculating a mod. Or the i keeps on incrementing.
Sure, but it will take long time to get to 9007199254740992. :-)
1

There are a couple of issues here that are stopping this from working.

First the var i = 1; needs to be moved outside the function to make the increment work. Also note that the first item in an array is 0, not 1.

Second you're using this to refer to change the image's src, but this is not a reference to the image. The best thing to do is use here is document.getElementById instead.

var i, button;

i = 0;
button = document.getElementById('myButton');

function displayImages() {
    var images = ['image1.jpg', 'image2.jpg', 'image3.jpg'];

    if (i > images.length - 1){
        button.src = images[0];
        i = 0;
    }

    else{
        button.src = images[i];
        i++;
    }

    setTimeout(displayImages, 2000);
}

There's still some room for improvement and optimisation, but this should work.

Improve this answer

Comments

1

You are reinitializing value of i every time, so change the following:

function displayImages(){
    var images = ['image1.jpg', 'image2.jpg', 'image3.jpg'];
    if(!displayImages.i || displayImages.i >= images.length) displayImages.i = 0;
    document.getElementById('myButton').src = images[displayImages.i];
    displayImages.i++;
    setTimeout(displayImages, 2000);
}

Functions are objects in JS and because of this:

  • you can pass them by reference and not as a string improving performance and readability
  • you can add fields and even methods to a function object like I did with displayImages.i

EDIT: I've realized that there was one more issue src was not being set for button.
Now I've fixed this and also made other improvements.

Here is the fiddle http://jsfiddle.net/aD4Kj/3/ Only image URLs changed to actually show something.

Improve this answer

1 Comment

Guys, I've updated my answer to really resolve the problem but I still argue that passing string has worse performance.
0 
  <script type="text/javascript">
           var i = 1;
            function displayImages(){
            .......
            ........

Just make "i" Global. so that whenever displayImages being called it will not redefined to 1.

Improve this answer

Comments

0 
<html>
<head>
    <script type="text/javascript">
        var i = 1;
        function displayImages() {
            var images = ['img1.jpg', 'img2.jpg', 'img3.jpg'];
            i++;
            if (i > images.length - 1) {           
                i = 0;
            }
            $('#myButton').attr('src', images[i]);
            setTimeout(displayImages, 2000);
        }
    </script></head>

<body onload="displayImages();">
    <img id="myButton" src="img1.jpg" height="150px" width="150px"/>
</body>
</html>

Make i global.

Improve this answer

Comments

0

here your are using displayImages() recursively and variable for index i. e i assign as local variable in function displayImages() , you need to assign it global variable i.e outside of the function also initialize it from i=0 as array index always start from 0,

your code become

        var i = 0; // assign i global
        function displayImages(){


       var images = ['image1.jpg', 'image2.jpg', 'image3.jpg'];



       if(i>images.length-1){

       document.getElementById('myButton').src=images[0]; //get img by id
       i=0;         // to get first image
       }

       else{

       document.getElementById('myButton').src=images[i]; //get img by id
       i++;
       }
       setTimeout("displayImages()", 2000);

    }
Improve this answer

Comments

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.