Trouble with closure in Javascript loop

Question

The process: In the game I'm making, there's a for loop that's supposed to save a value in an array. That value changes with each iteration. The problem: when the loop is done running, every element of the array is identical, all showing the most recent value.
I know this issue is common, and I've made so many different tweaks and attempts at solving it over the past 2 days.

0) I tried separating things into separate functions as much as possible.
1) I tried defining my loop counters with "let" so they would have a local scope.
2) I tried wrapping my assignment in a self-executing function so it would happen immediately, preserving the value of currentlyOn before the next loop iteration changes it. My counter is the variable c.

(function(c2, currentlyOn2) {
  onAtSameTime[c2] = currentlyOn2;
  return 0;
})(c, currentlyOn);

3) I tried attempt #2 with the added feature of returning a function, which still didn't save the value of currentlyOn. This option isn't a good one for me anyway, because the whole point is that I'm doing some computations ahead of time so my game will have a quick animation loop.

onAtSameTime[c] = (function(currentlyOn2) {
  return function() {
    return currentlyOn2;
  };
})(currentlyOn);

I'm tired of beating my head against this wall. Can anyone explain what I'm doing wrong?

For more details, check out the jsfiddle I made. The problem area is at line 59, using a simple assignment:

onAtSameTime[c] = currentlyOn;

currentlyOn is merely a pointer to where the value for the array is stored. Every [c] index will contain the same pointer, and as a result will then also access the same value. Use localized arrays instead of currentlyOn to avoid this, or consider refactoring your approach to avoid one array when there should be many. — Travis J
– Travis J, Commented Jan 10, 2017 at 21:15
No, this has nothing to do with closures. There are no functions that are executed after your loops. You seem to have problems with the array iteration, array cloning, or your algorithm. — Bergi
– Bergi, Commented Jan 10, 2017 at 22:04
You need to post more code so we can see what you're doing wrong. — Barmar
– Barmar, Commented Jan 10, 2017 at 22:27
Post your code here, not just at jsfiddle. You can make it executable with Stack Snippets — Barmar
– Barmar, Commented Jan 10, 2017 at 22:29

Eric Gibby · Accepted Answer · 2017-01-10 21:27:57Z

1

onAtSameTime[c] = currentlyOn; sets onAtSameTime[c] equal to the reference of currentlyOn, since currentlyOn is an array, not a primitive value. That reference gets updated with each iteration. You could work around that by creating a copy of the array before adding it to the onAtSameTime array. Something like onAtSameTime[c] = [].concat(currentlyOn); would do the trick.

See this fork of your JSFiddle: https://jsfiddle.net/L2by787y/

answered Jan 10, 2017 at 21:27

Eric Gibby

2311 silver badge4 bronze badges

Sign up to request clarification or add additional context in comments.

1 Comment

inthse Over a year ago

Thanks for the solution and detailed explanation! Your answer works for me.

Nina Scholz · Accepted Answer · 2017-01-10 22:09:27Z

1

You could make a copy from currentlyOn for assigning to onAtSameTime[c]. This keeps the values, but does not keep the reference to the same array.

onAtSameTime[c] = currentlyOn.slice(); // use copy

"use strict";
function log(text) {
    document.getElementById("logbox").innerHTML += JSON.stringify(text) + "<br>";
    return 0;
}

function whichSwitchesAreOn() {
    var currentlyOn = [],
        flickedSet,
        flickedOne,
        turningOnCheck;

    for (var c = 0; c < switchesToggled.length; c++) {
        flickedSet = switchesToggled[c];
        for (var d = 0; d < flickedSet.length; d++) {
            flickedOne = flickedSet[d];
            turningOnCheck = currentlyOn.indexOf(flickedOne);
            if (turningOnCheck == -1) {
                currentlyOn.push(flickedOne);
            } else {
                currentlyOn.splice(turningOnCheck, 1);
            }
        }
        log("currentlyOn: " + currentlyOn);
        onAtSameTime[c] = currentlyOn.slice(); // use copy
    }
    return 0;
}

var switchesToggled = [[0], [1, 2], [0], [2], []],
    onAtSameTime = [];

whichSwitchesAreOn();
log(onAtSameTime);

<div id="logbox"></div>

edited Jan 10, 2017 at 22:09

answered Jan 10, 2017 at 21:43

Nina Scholz

388k26 gold badges367 silver badges417 bronze badges

2 Comments

Bergi Over a year ago

onAtSameTime[c] = currentlyOn.slice(); is simpler and one line shorter

inthse Over a year ago

Thanks so much for your time. I accepted Eric Gibby's answer because it had more detail to the explanation, but your answer also works.

Learning2Code · Accepted Answer · 2017-01-10 21:16:31Z

0

You say you have tried let?

Did you have let currentlyOn = [] inside of the for loop?

  for(var c = 0; c < switchesToggled.length; c++) {
      let currentlyOn = [];

answered Jan 10, 2017 at 21:16

Learning2Code

5219 silver badges21 bronze badges

2 Comments

Nina Scholz Over a year ago

it is working with just an assignment without let.

inthse Over a year ago

Unfortunately, each iteration of the loop must be able to remember the previous value of currentlyOn, so I can't define it within the loop.

Collectives™ on Stack Overflow

Trouble with closure in Javascript loop

3 Answers 3

1 Comment

2 Comments

2 Comments

Your Answer

Hot Network Questions

Collectives™ on Stack Overflow

3 Answers 3

1 Comment

2 Comments

2 Comments

Your Answer

Sign up or log in

Post as a guest

Related