It now seems like the OP edited their question to change the code to something COMPLETELY DIFFERENT than they originally had. The answer below applies to what their code originally was, though my first code example also solves their issue.
Your script seems to work fine for a number of us (see referenced jsFiddle links in the comments to your question).
As to your other question: document.getElementById("light") fetches the DOM element that contains your light representation. You can either fetch that DOM element once and store it in a persistent variable such that you can just use that variable each time or you can simply fetch it each time you need it.
The general style guideline is that you don't want to persistently store DOM objects in long lasting variables unless you have a really good reason for doing so because in dynamic pages where objects are created and destroyed or in large complicated projects, storing a DOM reference in a persistent script variable can sometimes lead to either a memory leak or an inconsistent state. If you fetch the DOM object upon demand, you never get an inconsistent state or a leak.
But, there are more efficient ways to code what you've done that still request the DOM object upon demand such as this:
<script>
var on = false;
function toggleLight() {
var bulb = document.getElementById("light");
if (on) {
bulb.style.color = "black";
} else {
bulb.style.color = "yellow";
}
on = !on;
}
</script>
This style would fetch the DOM object only once ever, but it would then be storing that DOM object in a global variable which you generally want to avoid unless you have a very good reason for doing so:
<script>
var on = false;
var bulb = document.getElementById("light");
function toggleLight() {
if (on) {
bulb.style.color = "black";
} else {
bulb.style.color = "yellow";
}
on = !on;
}
</script>
The second script has a slight performance difference than the first (more startup execution time, less execution time at the time of the click), but since this is a user-triggered event, it would not be possible to notice the difference in execution time at the time of the click, therefore, the cleaner design of the first option is recommended (no storing of DOM objects in persistent variables).
FYI, notice that I've also shortened some of the rest of your code.
if (on) in this case works just as well as if (on == true), but is more concise and is a more generally used style.- There is no reason to use
else if (on == false) when a simple else will work just as well for your specific case.
- I've replace the two separate assignments to on with a single assignment that simply reverses its state
on = !on;.
- I've move the fetching of the DOM element outside the
if/else statement since it is needed in both. Per the principles of DRY, you want to combine common code into a single place as often as practical.
