Loading ...
Sorry, an error occurred while loading the content.

Be careful when making functions in a loop...

Expand Messages
  • tonyorlando
    In 3000 lines of JS code, I m completely clean according to JSLint, except for five messages like this: Be careful when making functions within a loop.
    Message 1 of 8 , Jul 25, 2008
    • 0 Attachment
      In 3000 lines of JS code, I'm completely clean according to JSLint,
      except for five messages like this:

      "Be careful when making functions within a loop. Consider putting the
      function in a closure."

      These are marked as "errors," but since the message is "be careful,"
      they seem more like warnings.

      I'm not sure what to do about these.

      It would be nice if for every type of error JSLint reports, there
      would be "before" and "after" snippets of code so I'd get an idea of
      what I'm supposed to change. If there was a discussion of why I should
      make the change, that'd be appreciated as well.

      What's the idea behind this error?
    • Douglas Crockford
      ... It is generally a bad idea to define a function in a loop. If the function refers to a variable that changes in the loop, then it will bind to the final
      Message 2 of 8 , Jul 25, 2008
      • 0 Attachment
        --- In jslint_com@yahoogroups.com, "tonyorlando" <rhettanderson@...>
        wrote:
        >
        > In 3000 lines of JS code, I'm completely clean according to JSLint,
        > except for five messages like this:
        >
        > "Be careful when making functions within a loop. Consider putting the
        > function in a closure."
        >
        > These are marked as "errors," but since the message is "be careful,"
        > they seem more like warnings.
        >
        > I'm not sure what to do about these.

        It is generally a bad idea to define a function in a loop. If the
        function refers to a variable that changes in the loop, then it will
        bind to the final state, not the current state, of the variable, which
        usually causes errors. The remedy is to move the function declaration
        out of the loop (which is also an optimization) or to wrap the
        function declaration inside another function that will bind the loop
        values.

        Since you didn't include an example from your loops, I can't tell you
        which you should do.
      • Ben Collver
        Hi Doug, I wrote an example script and it appears to bind to the current value of the variable i . The script is at the following URL.
        Message 3 of 8 , Aug 13 10:31 AM
        • 0 Attachment
          Hi Doug,

          I wrote an example script and it appears to bind to the current value
          of the variable "i".  The script is at the following URL.
          http://nopaste.info/551fef77cf.html 

          Example output:
          0,a
          0,b
          0,c
          1,a
          1,b
          1,c
          2,a
          2,b
          2,c
          3,a
          3,b
          3,c

          If you please would you show an example of the problem?

          Thank you,

          Ben
        • Douglas Crockford
          ... Create a page with three buttons, a, b, and c. When you click on a button, it alerts its number. Modify your loop to get each element by id and attach an
          Message 4 of 8 , Aug 13 11:31 AM
          • 0 Attachment
            --- In jslint_com@yahoogroups.com, "Ben Collver" <tylx@...> wrote:
            > I wrote an example script and it appears to bind to the current value
            > of the variable "i". The script is at the following URL.

            > If you please would you show an example of the problem?

            Create a page with three buttons, a, b, and c. When you click on a
            button, it alerts its number. Modify your loop to get each element by
            id and attach an onclick handler that alerts the index. If you do it
            incorrectly, they will all alert the same wrong number.
          • Ben Collver
            I created a page with three buttons, a, b, and c. When you click on a button, it alerts its number. I modified the loop to attach an onclick handler to each
            Message 5 of 8 , Aug 19 1:40 PM
            • 0 Attachment
              I created a page with three buttons, a, b, and c. When you click on a button, it alerts its number. I modified the loop to attach an onclick handler to each input element and alert the index. They all alert 4 as expected.

              I must have misunderstood the instructions. The page is at http://bencollver.googlepages.com/looptest.html

              Ben


              ________________________________________________________________________
              Posted by: "Douglas Crockford" douglas@... douglascrockford
              Date: Wed Aug 13, 2008 11:31 am ((PDT))

              Create a page with three buttons, a, b, and c. When you click on a
              button, it alerts its number. Modify your loop to get each element by
              id and attach an onclick handler that alerts the index. If you do it
              incorrectly, they will all alert the same wrong number.
            • William Chapman
              Ben, Another approach: function main() { var arr = [ a , b , c ]; var closedValue; for (var i = 0; i
              Message 6 of 8 , Aug 20 9:24 AM
              • 0 Attachment
                Ben,

                Another approach:

                function main() {
                var arr = ['a', 'b', 'c'];
                var closedValue;
                for (var i = 0; i < 3; i++) {
                closedValue = i;
                document.getElementById(arr[i]).onclick = function(closedValue) {
                alert(closedValue);
                });
                }
                }

                In your code, this line:
                alert(i)
                displays the value of i **when the button is clicked**, as opposed to,
                **when the function is assigned to onclick.**

                For me, the original point - that one needs to take care when defining
                functions in a loop - is valid. Which isn't the same as saying it's never a
                good idea.

                -- Bill

                On Tue, Aug 19, 2008 at 1:40 PM, Ben Collver <tylx@...> wrote:

                > I created a page with three buttons, a, b, and c. When you click on a
                > button, it alerts its number. I modified the loop to attach an onclick
                > handler to each input element and alert the index. They all alert 4 as
                > expected.
                >
                > I must have misunderstood the instructions. The page is at
                > http://bencollver.googlepages.com/looptest.html
                >
                > Ben
                >
                > __________________________________________________________
                > Posted by: "Douglas Crockford" douglas@...<douglas%40crockford.com>douglascrockford
                > Date: Wed Aug 13, 2008 11:31 am ((PDT))
                >
                >
                > Create a page with three buttons, a, b, and c. When you click on a
                > button, it alerts its number. Modify your loop to get each element by
                > id and attach an onclick handler that alerts the index. If you do it
                > incorrectly, they will all alert the same wrong number.
                >
                >
                >


                [Non-text portions of this message have been removed]
              • William Chapman
                Ben, Please forgive my previous email which is totally wrong! The following actually works (shamelessly copied from Crockford: The Good Stuff page 39):
                Message 7 of 8 , Aug 20 4:19 PM
                • 0 Attachment
                  Ben,

                  Please forgive my previous email which is totally wrong! The following
                  actually works (shamelessly copied from Crockford: "The Good Stuff" page
                  39):

                  function main() {
                  var arr = ['a', 'b', 'c'];
                  for (var i = 0; i < 3; i++) {
                  document.getElementById(arr[i]).onclick = function(i) {
                  return function (event) {
                  alert(i);
                  };
                  }(i);
                  }
                  }

                  Other comments:
                  (1) The onclick handlers in your HTML <input > elements are overwritten by
                  main() above, so are not needed..
                  (2) Your original forEach() function is not used in the above approach.

                  Sorry about the bum steer!

                  -- Bill

                  On Wed, Aug 20, 2008 at 9:24 AM, William Chapman <jeddahbill@...>wrote:

                  > Ben,
                  >
                  > Another approach:
                  >
                  > function main() {
                  > var arr = ['a', 'b', 'c'];
                  > var closedValue;
                  > for (var i = 0; i < 3; i++) {
                  > closedValue = i;
                  > document.getElementById(arr[i]).onclick = function(closedValue) {
                  > alert(closedValue);
                  > });
                  > }
                  > }
                  >
                  > In your code, this line:
                  > alert(i)
                  > displays the value of i **when the button is clicked**, as opposed to,
                  > **when the function is assigned to onclick.**
                  >
                  > For me, the original point - that one needs to take care when defining
                  > functions in a loop - is valid. Which isn't the same as saying it's never a
                  > good idea.
                  >
                  > -- Bill
                  >
                  >
                  > On Tue, Aug 19, 2008 at 1:40 PM, Ben Collver <tylx@...> wrote:
                  >
                  >> I created a page with three buttons, a, b, and c. When you click on a
                  >> button, it alerts its number. I modified the loop to attach an onclick
                  >> handler to each input element and alert the index. They all alert 4 as
                  >> expected.
                  >>
                  >> I must have misunderstood the instructions. The page is at
                  >> http://bencollver.googlepages.com/looptest.html
                  >>
                  >> Ben
                  >>
                  >> __________________________________________________________
                  >> Posted by: "Douglas Crockford" douglas@...<douglas%40crockford.com>douglascrockford
                  >> Date: Wed Aug 13, 2008 11:31 am ((PDT))
                  >>
                  >>
                  >> Create a page with three buttons, a, b, and c. When you click on a
                  >> button, it alerts its number. Modify your loop to get each element by
                  >> id and attach an onclick handler that alerts the index. If you do it
                  >> incorrectly, they will all alert the same wrong number.
                  >>
                  >>
                  >>
                  >
                  >


                  [Non-text portions of this message have been removed]
                • Ben Collver
                  Bill, Thank you for the response. I worked backward from it to get to an example of the problem. It behaves the same on several hosts: it outputs three 3 s.
                  Message 8 of 8 , Aug 25 10:28 PM
                  • 0 Attachment
                    Bill,

                    Thank you for the response. I worked backward from it to get to an example of the problem. It behaves the same on several hosts: it outputs three 3's. My original WSH example must have avoided the problem.

                    http://bencollver.googlepages.com/looptest3.html

                    Cheers,

                    Ben
                  Your message has been successfully submitted and would be delivered to recipients shortly.