2

For complex functions declared within a loop, I can see why I wouldn't want to do this, but why would it be be considered bad javascript?

We can name the function and place it outside the loop of course, but upsets the flow for something that is simple ( no async ).

Eg, below is a simple inline function declaration within a loop ( JSHINT/LINT complains, why this is considered a no no ?

for (var i = 0, len=arr.length; i < len; ++i) {

     dosomething(arr[i], function(returnvalue) {
         console.log(returnvalue);
     });

};
Rob Sedgwick
  • 5,216
  • 4
  • 20
  • 36
  • 4
    This creates the function for each iteration. It's uselessly heavy. It's better to take good habits than having to optimize later but well, if it's client side and performances don't matter, this isn't so important... – Denys Séguret Feb 02 '14 at 12:02
  • so its a performance problem ? – Rob Sedgwick Feb 02 '14 at 12:02
  • possible duplicate of [Don't make functions within a loop](http://stackoverflow.com/questions/10320343/dont-make-functions-within-a-loop) – Givi Feb 02 '14 at 12:08
  • or [JSlint error 'Don't make functions within a loop.' leads to question about Javascript itself](http://stackoverflow.com/questions/3927054/jslint-error-dont-make-functions-within-a-loop-leads-to-question-about-javas) – Givi Feb 02 '14 at 12:09
  • If you use `forEach` you can simply get around this issue – elclanrs Feb 02 '14 at 12:10
  • Thanks Givi, Great second link. Thanks Elclanrs, looking into that ( and why ) as it's handly for small opps to keep in line with the logic at times. – Rob Sedgwick Feb 02 '14 at 12:17

2 Answers2

3

Creating a function at each iteration is uselessly heavy.

Most of the time, in client side JavaScript, performance doesn't matter and there's no problem but it's better to take and keep good habits than having later to optimize the code (as long as the readability isn't hindered).

Here's a proof that you create a new function at each iteration :

var old;
function compare(_, a){
  if (old) console.log('equal ?', old==a);
  else old = a;
}
for (var i=0; i<2; i++){
  compare(i, function(i) { return i*i }); 
}

It logs 'equal' ? false

testable jsbin

Denys Séguret
  • 372,613
  • 87
  • 782
  • 758
  • 1
    As far as I remember it doesn't create the function multiple times. anonymous functions are created only once and references the same variables in the scope. – Loïc Faure-Lacroix Feb 02 '14 at 12:25
  • @LoïcFaure-Lacroix You might be interested by my edit. – Denys Séguret Feb 02 '14 at 12:30
  • Even if it's created multiple time, the real problem is that it references the same variables, and your `i` isn't copied to the scope so a function that is executed later might have weird values. – Loïc Faure-Lacroix Feb 02 '14 at 12:33
  • @LoïcFaure-Lacroix That's totally unrelated to the question. I explained why it was a bad practice : it creates useless functions, contrary to what you thought. – Denys Séguret Feb 02 '14 at 12:34
  • @dystroy did you run my fiddle? – Loïc Faure-Lacroix Feb 02 '14 at 12:36
  • @LoïcFaure-Lacroix Yes. You seem to answer a totally different question. I see you edited your answer because it was wrong but you forgot to remember the other comment in this thread. – Denys Séguret Feb 02 '14 at 12:44
  • 2
    It's true that on each iteration new function object is created, however I'm sure that in all current javascript implementations the code part of the function is shared. So yes, it’s will burden garbage collector with anonymous function objects, but it’s not “heavier”. I did a small benchmark, the results aren’t surprising: http://jsperf.com/function-in-the-loop – phadej Feb 02 '14 at 12:56
  • Thanks Oleg and all. What I am getting here from all this great info, is it looks safe to ignore the jshint error for small anon functions within small array loops. ( as it's v handly sometimes to keep code flow inline) and jshint is 'doing the right thing'. A "warning" rather than an "error". – Rob Sedgwick Feb 02 '14 at 13:17
  • @OlegGrenrus yeah that's what I mean, functions object are created but the js engine will compile the function once and bind the code, lexical scope to the function object that is created on each loop. – Loïc Faure-Lacroix Feb 02 '14 at 13:24
  • @RobSedgwick as far as I remember the real reason for the warning was to prevent accidental errors related to the lexical scope. JSlint can be quite annoying to be honest, that said, having a new object on each loop can be faster than having a function referenced by name. I bet the code to create the new object is faster than the code to lookup the name. – Loïc Faure-Lacroix Feb 02 '14 at 13:32
  • @Loic, "prevent accidental errors related to the lexical scope" if that is the key reason why it was put into jslint I should mark your answer as correct. "I bet the code to create the new object is faster than the code to lookup the name". - Sounds right to me in certain cases and as Olags jspref shows. – Rob Sedgwick Feb 02 '14 at 13:46
  • Hoping that @dystroy can clarify meaning of "uselessly heavy." As performance seems debatable for some cases where expression functions are within a loop. And/So might be an incorrect answer as to "why" the jshint/error gives warning. – Rob Sedgwick Feb 02 '14 at 13:58
  • Don't trust jsperf for such a test ! Search a little for "jsperf credibility" and you'll see that you must work much deeper to make a credible jsperf... Also don't forget that what matters in performances is most often allocation, because allocation means garbaging. Allocating a new function *isn't* light and is by no mean comparable to name lookup in optimized real code (in modern engines, objects are most often compiled as classes, especially if they don't change too much and name lookup is very fast). – Denys Séguret Feb 02 '14 at 13:59
  • @RobSedgwick check my answer, I updated it with the official answer from jslint. – Loïc Faure-Lacroix Feb 02 '14 at 14:05
  • I don't trust it. There is debate as to speed - (if that is what you mean be "uselessly heavy") - I can't see that creating an anon function within a loop as being any heavier than declaring any type of new variable (yet) - to be able to 'mark this as the correct answer' – Rob Sedgwick Feb 02 '14 at 14:06
  • really appreciate the answers – Rob Sedgwick Feb 02 '14 at 14:07
3

Here's one reason why you wouldn't want that. The function references the same vars.

http://jsfiddle.net/RCzyF/

var a = [];

for(var i=0; i<10; i++) {
    a.push(function () {
      return i; 
    });
}

h = "";
for(var j=0; j<10; j++) {
 h += "" + a[j]();   
}

alert(h);

One could expect to see 0123456789 but it will append 10 10 times to h instead. It can make code really hard to understand when one function might change the content of other functions.

Here's a more complex example how things can get wrong.

var a = [];

for(var i=0; i<10; i++) {
    a.push(function () {
      return i++; 
    });
}

h = "";
for(var j=0; j<10; j++) {
 h += "" + a[j]();   
}

alert(h);

When the functions are created, they point to the same lexical scope. When the function are executed, they change the value inside the function and each function in the array still point to the same value. This can lead to really hard bug to debug when a variable gets modified but you didn't directly modify it.

Also here's the real answer coming from jslint itself: http://jslinterrors.com/dont-make-functions-within-a-loop/

Loïc Faure-Lacroix
  • 13,220
  • 6
  • 67
  • 99
  • Look at my answer. I proved that *"The anonymous function is created once"* is wrong – Denys Séguret Feb 02 '14 at 12:32
  • 1
    Thanks Loic, this is an added bonus to the question, showing the looped scope variables are not created newly. very cool, and one to watch for for sure! – Rob Sedgwick Feb 02 '14 at 13:04
  • Thanks Loic, I am marking this as correct , as in my example, I pass 'i' to the inline function and your answers shows the potential problems with that, and , from the new link ( thanks for that ), it explains as the scoping issue as the real reason for the warning. I see that in many cases @Dystroy is also correct , but having to keeping to the example in the question, this ( along with Oleg Grenrus explanation ) shows the clearest reason. Thanks everyone! Ace learnings. – Rob Sedgwick Feb 02 '14 at 14:15