The JS Function-in-Loop Bug

There is a nasty little thing in JavaScript that goes wrong all the time: putting functions into loops and accessing outer closure-variables from there. Code like this:

        for (var i = 0; i < document.body.children.length; i++) {
          var element = document.body.children[i];
          element.addEventListener("click", function() {
            clicked(element);
          });
        }

This code loops all direct children of document.body and adds a click-listener to them. The anonymous listener-function calls a function clicked() and passes the clicked element as parameter to it. Doing this, the anonymous function accesses the outer closure variable element.

But this code does not work. I call it the Function-in-Loop Bug. Don't nest functions into loops, or make sure they do not access outer closure variables.

Test Page

<!DOCTYPE HTML>
<html>

<head>
  <meta charset="UTF-8">
  <meta name="viewport" content="width=device-width, initial-scale=1"/>
  <title>How to install a listener in a loop</title>
</head>
  
<body>

  <p>PARAGRAPH</p>
  <span>SPAN</span><br>
  <div>DIV</div>

  <script type="text/javascript">
  var clickListener = (function() {
      var currentElement;
      var self = {};

      self.clicked = function(element) {
        if (currentElement)
          currentElement.style["background-color"] = ""; // reset selection color
          
        currentElement = element;
        
        element.style["background-color"] = "LightBlue"; // set new selection color
      };
      
      return self;
  })();
  </script>

  <script type="text/javascript">
  (function() {
      // initialization goes here ....
  })();
  </script>

</body>
</html>

Three direct children of document.body exist in this page. The first SCRIPT-tag contains a small singleton module that exposes a clicked(element) function, to be used to change the background-color of an element. We would have to call

Then there is an empty second SCRIPT-tag, containing an IIFE that will be implemented now to install click-listeners to all body-children.

Wrong

      var init = function(parent) {
        for (var i = 0; i < parent.children.length; i++) {
          var element = parent.children[i];
          
          if (element.tagName !== "BR" && element.tagName !== "SCRIPT")
            element.addEventListener("click", function() {
              clickListener.clicked(element);
            });
        }
      };

      init(document.body);

This code sets up an init() function which loops the body-children by index and installs a click-callback to them. The init() is called finally to really do that work. Within the loop, we avoid to install the listener also to SCRIPT or BR tags, because this makes no sense. Then there is a nested function which calls clickListener.clicked(element) with the currently looped element.

When you load this page into a web-browser, and you debug the JS code, you will see that the listener actually is called each time you click, but the parameter passed to the clicked() function is wrong, it is always the last SCRIPT tag of the document. Setting a color onto a SCRIPT element has no effect :-!

The element within the anonymous function is evaluated only when the function is actually executed, when the user clicks an element. And then the element variable will hold the last looped element as value (which is not what we would expect). The element variable survived the execution of init() because it is in the closure of the anonymous listener function.

Right

      var installClickListener = function(element) {
        element.addEventListener("click", function() {
          clickListener.clicked(element);
        });
      };
      
      var init = function(parent) {
        for (var i = 0; i < parent.children.length; i++) {
          var element = parent.children[i];
          if (element.tagName !== "BR" && element.tagName !== "SCRIPT")
            installClickListener(element);
        }
      };

      init(document.body);

The problem is to "freeze" the loop-variable element for the call to clickListener.clicked(element). That can be achieved by inserting an installClickListener() function between the loop and the listener-call. The questionable element variable is passed to the installClickListener() function, and as side-effect its value is "freezed", because JS copies the values of parameters when it calls functions with parameters. (Mind that it copies just the pointer, not the content!)

When you try this out now, you will see that any clicked element turns blue, while the previously selected element loses its blue. Which is what we wanted to achieve.

Resume

In Java, all closure-variables have to be final, that means they are real constants and not variables. That way they never could hold a wrong value at a later time when they are accessed by some event-callback. But the rubber-language JavaScript is much nearer to C than to Java, so don't expect technological advance from it.