CODEWITHSUNDEEP
javascript
Tyler
Tyler

Reputation: 322

Maintain original value during passing of a variable

Okay so what I'm trying to do is when a button is clicked function triggers that creates the following timeout:

setTimeout("if (document.getElementById(lightnum).style.backgroundColor=='green'){document.getElementById(lightnum).dataset.dead=1;document.getElementById(lightnum).style.backgroundColor='red';}", 3000);

The problem I'm having is that because the variable lightnum is reused instantly it makes it so when this timeout triggers it references the current value of lightnum not the value of lightnum when the settimeout was created. The functionality I'm looking for here is when the setTimeout triggers after 3 seconds it references the value of lightnum when it was originally created.

http://jsfiddle.net/657q2/1/

Upvotes: 0

Views: 65

Answers (3)

Brett Zamir
Brett Zamir

Reputation: 14345

"Jon" and "Ted Hopp" have proper answers, but I might just add why functions are better here:

  1. As Barmar stated, a string evaluation will be in global scope.
  2. IDEs will not syntax highlight within strings, so it makes your code less readable to yourself and others.
  3. It is slower to evaluate.
  4. Most seriously, if a portion of your string is from untrusted input, it could cause a security problem (e.g., if part of the string is coming from your database of user comments, a malicious user could add code there to grab the user's cookies).

Upvotes: 1

Jon
Jon

Reputation: 437386

First of all, that code should be in a proper function instead of a string.

Regarding your problem, it's fixed like this:

var callback = (function(target) {
    return function() {
        if (target.style.backgroundColor == 'green') {
            target.dataset.dead = 1;
            target.style.backgroundColor = 'red';
        }
    };
})(document.getElementById(lightnum));

setTimeout(callback, 3000);

The problem with your original code is that lightnum in your original callback evaluates to whatever its value is when the callback is invoked, as you have already seen. What you want instead is to somehow "freeze" it to its initial value.

One attempt to do that would be to make a local copy inside the function that sets the timeout (var copy = lightnum; etc). However, this will still not work because this time when the callback is invoked it will operate on the last value that copy had the last time this function was called (possibly, but not necessarily, the same behavior as your original code).

What you really want to do is put the current value of lightnum somewhere that is only accessible by the code of the callback; in JS, the only way to do that is pass it to a function as an argument. This necessitates the funky "function that returns function" syntax above: the inner function is the actual desired callback and the outer function is a "firewall" that prevents any outside meddling with the variable in question.

Upvotes: 4

Ted Hopp
Ted Hopp

Reputation: 234807

Use a closure instead of a string:

setTimeout(
    (function(ln) {
        return function() {
            if (document.getElementById(ln).style.backgroundColor=='green') {
                document.getElementById(ln).dataset.dead=1;
                document.getElementById(ln).style.backgroundColor='red';
            }
        };
    }(lightnum)),
    3000
);

Upvotes: 1

Related Questions