global array returns undefined

Question

I have a slideshow with button like 1,2,3,4. Also I have define an Array with 4 images url. When click the button, it call the specific corresponding url.

<div id="slideshow">
<div id="source"><img src="images/blue.jpg">
<ul id="controller">
<li class="button">1</li>
<li class="button">2</li>
<li class="button">3</li>
<li class="button">4</li>
</ul>
</div>
</div>

Javascript:

var button=document.getElementsByClassName("button")
var image=document.getElementsByTagName("img")[0]
var sources=new Array("images/blue.jpg","images/red.jpg","images/yellow.jpg","images/green.jpg");

for (var i=0;i<sources.length;i++){
    button[i].onclick=function(){
            image.src=sources[i]
            }
}

But when I click the button, the sources[i] alway return undefined.

In your case `i` variable can't be passed to event handler (onclick). You have to store its values within appropriate DOM elements — hindmost, Apr 05 '14 at 06:54
possible duplicate of [Javascript closure inside loops - simple practical example](http://stackoverflow.com/questions/750486/javascript-closure-inside-loops-simple-practical-example) — Felix Kling, Apr 05 '14 at 10:17
and http://stackoverflow.com/questions/4091765/assign-click-handlers-in-for-loop — Felix Kling, Apr 05 '14 at 10:18

score 0 · Answer 1 · answered Apr 05 '14 at 07:02

0

for (var i=0;i<sources.length;i++){
    button[i].onclick= function(i){
       return function(){
            image.src=sources[i]
            }
     }(i);
}

try this http://jsbin.com/puyipiro/1/

P.S. error cause in onclick handler i equal 4 , so must save current value on each iteration with creation handler

answered Apr 05 '14 at 07:02

Vasiliy vvscode Vanchuk

7,007
2
21
44

also I want to use this way – Frank Dai Apr 05 '14 at 07:10
We pass value on each iteration into the function, so it save value in closure. – Vasiliy vvscode Vanchuk Apr 05 '14 at 07:10
var button=document.getElementsByClassName("button") var image=document.getElementsByTagName("img")[0] var sources=new Array("images/blue.jpg","images/red.jpg","images/yellow.jpg","images/green.jpg"); function changepic(x) { imarge.src=sources[x] } for (var i=0;i – Frank Dai Apr 05 '14 at 07:11
All i recommend you look to another answers - i am also think, that it is better save image number as attribute – Vasiliy vvscode Vanchuk Apr 05 '14 at 07:15

score 0 · Answer 2 · answered Apr 05 '14 at 07:04

You have run into one of the most confusing parts of JavaScript, the closure capture problem. To get your code to work re-write it as,

var button=document.getElementsByClassName("button")
var image=document.getElementsByTagName("img")[0]
var sources=new Array("images/blue.jpg","images/red.jpg","images/yellow.jpg","images/green.jpg");

for (var i=0;i<sources.length;i++){
   (function (i) {
     button[i].onclick=function(){
       image.src=sources[i]
     }
   })(i);
}

This uses a immediately executing function to introduce a new scope so each onclick has its own copy of i that has the value of i at the time the function was created instead of sharing the same i which will have the value of sources.length.

If you targeting a browser that supports ES 6 you can use let instead of var which will then allow you to write this as,

var button=document.getElementsByClassName("button")
var image=document.getElementsByTagName("img")[0]
var sources=new Array("images/blue.jpg","images/red.jpg","images/yellow.jpg","images/green.jpg");

for (var i=0;i<sources.length;i++){
  let j = i;
  button[i].onclick=function() {
    image.src=sources[j]
  }
}

The let statement creates a fresh scope for j for each iteration in the loop so the j captured by the onclick functions are different and store the value of i had when the function was created as the immediately executing function above does.

Unfortunately, ES 6 is not yet broadly available so some variant of my original suggestion is what you need.

score -1 · Answer 3 · answered Apr 05 '14 at 07:10

-1

You may store i values within appropriate DOM elements (buttons), e.g. as attributes, then retrieving them in onclick handler. Somehow like this:

for (var i=0;i<sources.length;i++){
    button[i].setAttribute('data-btn_i', i);
    button[i].onclick=function(){
        image.src=sources[this.getAttribute('data-btn_i')];
    }
}

answered Apr 05 '14 at 07:10

hindmost

7,125
3
27
39

That is an expensive non-composable solution. For example, if you cut-and-pasted this code it would break as both copies would use the same attribute. You would need to remember to change the attribute name. Creating a closure is much cleaner, faster and is composable. – chuckj Apr 05 '14 at 07:38
@chuckj expensive - maybe, non-composable - unlikely. These 2 statements are intended to be always next each to other. So I don't see how it could break anything – hindmost Apr 05 '14 at 07:49
I understand the intent. However, it doesn't change the fact that you are using, in effect, global variables. – chuckj Apr 05 '14 at 09:48
global variables? I think you have to read about data attributes (https://developer.mozilla.org/en-US/docs/Web/Guide/HTML/Using_data_attributes) before mentioning global variables – hindmost Apr 05 '14 at 11:23
Please. I understand what an attribute is. I didn't say you used a global variable, I said you are using a technique that is equivalent to using a global variable. – chuckj Apr 05 '14 at 21:20
A correct code fragment is not composable when it can be rendered incorrect when composed with another correct code fragment. To render yours incorrect I just need the correct code fragment `button[0].setAttribute('data-btn_i', sources.length + 1)'` Non-composability is a general property of code that uses global variables or globally accessible values, such as a DOM attribute. – chuckj Apr 05 '14 at 21:36
This is the first time I see anyone compare HTML attribute with global variable. You might as well classify any "id" attribute (of DOM element) as global variable. In this case any JS snippet like this `document.getElementById("someform")` should be considered as deprecated. So you didn't convince me anyway. – hindmost Apr 06 '14 at 08:06
I am not saying using a global is bad or needs to be deprecated, just that it is not composable. I don't need to convince you of a fact; it just is. As for `document.getElementById("someform")`, yes I would consider this accessing a globally accessible value. `document` is global therefore everything reachable through it is also global. My point is you shouldn't use a global, or anything like it, if a better approach is available to you. – chuckj Apr 07 '14 at 02:00
My point is you don't understand essence of HTML attribute. If we could compare it with variable, it is more related to local variable than global. The scope of an attribute is only the HTML element it is belong to. We free to use the same named attributes in different HTML elements since they never can have collision. – hindmost Apr 07 '14 at 16:30
I am not sure where you got the idea I don't know what attributes are or how they work. Let me assure you I do. I apparently will never make myself clear enough to be understood so I will gracefully bow out of this conversation. – chuckj Apr 08 '14 at 05:39

global array returns undefined

3 Answers3