63

I was just linting some JavaScript code using JSHint. In the code I have two for-loops both used like this:

for (var i = 0; i < somevalue; i++) { ... }

So both for-loops use the var i for iteration.

Now JSHint shows me an error for the second for-loop: "'i' is already defined". I can't say that this isn't true (because it obviously is) but I always thought this wouldn't matter as the var i is only used in that specific place.

Is it bad practice to use for-loops this way? Should I use a different variable for each for-loop in my code like

//for-loop 1
for (var i = 0; ...; i++) { ... }

//for-loop 2
for (var j = 0; ...; j++) { ... }

Or is this on e of the errors I can ignore (because it doesn't break my code, it still does what it is supposed to do)?

JSLint btw. stops validating at the first for loop because I don't define var i at the top of the function (that's why I switched to JSHint in the first place). So according to the example in this question: Should I use JSLint or JSHint JavaScript validation? – I should use for-loops like this anyway to confirm JSLint:

...
var i;
...
//for-loop 1
for (i = 0; ...; i++) { ... }
...
//for-loop 2
for (i = 0; ...; i++) { ... }

This also looks good to me, because this way I should avoid both errors in JSLint and JSHint. But what I am uncertain about is if I should use a different variable for each for-loop like this:

...
var i, j;
...
//for-loop 1
for (i = 0; ...; i++) { ... }
//for-loop 2
for (j = 0; ...; j++) { ... }

So is there a best practice for this or could I just go with any of the code above, meaning I choose "my" best practice?

starball
  • 20,030
  • 7
  • 43
  • 238
TimG
  • 985
  • 3
  • 14
  • 24
  • The only time you need a different counter variable is when nesting loops (unless the nested loop is within its own function scope and doesn't use the counter from the previous scope). – bryc Jul 10 '15 at 20:03

6 Answers6

60

Since variable declarations are hoisted to the top of the scope in which they appear the interpreter will effectively interpret both versions in the same way. For that reason, JSHint and JSLint suggest moving the declarations out of the loop initialiser.

The following code...

for (var i = 0; i < 10; i++) {}
for (var i = 5; i < 15; i++) {}

... is effectively interpreted as this:

var i;
for (i = 0; i < 10; i++) {}
for (i = 5; i < 15; i++) {}

Notice that there is really only one declaration of i, and multiple assignments to it - you can't really "redeclare" a variable in the same scope.

To actually answer your question...

is there a best practice for this or could I just go with any of the code above?

There are varying opinions on how best to handle this. Personally, I agree with JSLint and think the code is clearer when you declare all variables together at the top of each scope. Since that's how the code will be interpreted, why not write code that looks as it behaves?

But, as you've observed, the code will work regardless of the approach taken so it's a style/convention choice, and you can use whichever form you feel most comfortable with.

starball
  • 20,030
  • 7
  • 43
  • 238
James Allardice
  • 164,175
  • 21
  • 332
  • 312
  • 4
    I think I like this approach best because I agree whith you when you say "why not write code that looks as it behaves?". It is very clear and readable. I think I will adept this style. – TimG Apr 03 '13 at 09:39
  • 2
    I would rather declare my loop iterator inside my loop header because it makes it very obvious what the variable is being used for, since loop iterators are historically simple non-descriptive names such as `i` and `j`. How should you avoid the issue of using the same variable as the iterator for multiple loops.? **Don't use the same variable as the iterator for multiple loops.** Either declare more than one iterator variable or don't do multiple loops in the same block. I personally find it most readable if you break each loop out into its own helper function, then you can use `i` in both. – WebWanderer Oct 17 '17 at 20:33
8

It has been mentioned only in the comment by @TSCrowder: If your environment supports it (Firefox, Node.js), in ES6 you can use let declaration

//for-loop 1
for (let i = 0; ...; i++) { ... }

//for-loop 2
for (let i = 0; ...; i++) { ... }

which limits the scope to within the for-loop. Bonus: JSHint stops complaining.

Community
  • 1
  • 1
serv-inc
  • 35,772
  • 9
  • 166
  • 188
6

Variables in javascript are function scoped (not block scoped).

When you define var i in a loop, it remains there in loop and also in the function having that loop.

See below,

function myfun() {
    //for-loop 1
    for (var i = 0; ...; i++) { ... }

    // i is already defined, its scope is visible outside of the loop1.
    // so you should do something like this in second loop.

    for (i = 0; ...; j++) { ... }

    // But doing such will be inappropriate, as you will need to remember
    // if `i` has been defined already or not. If not, the `i` would be global variable.
}
Jashwant
  • 28,410
  • 16
  • 70
  • 105
  • Yeah, this is too easy to get confused about (like why ist it declared in the first loop and not in the second). – TimG Apr 03 '13 at 09:41
  • 2
    But in this case, if the first loop is deleted, the second loop ends up leaking `i` to the global scope. However, if you are using a linter, this bug shouldn't ever get committed. – Andrew De Andrade Sep 25 '13 at 23:32
  • Thank you, to me, this illustrated the problem very well. – Doug Molineux Aug 26 '14 at 19:29
5

The reason JSHint shows the error is because in JS variable scope is function and variable declarations are hoisted to the top of the function.

In Firefox you can use let keyword to define block scope, but is not currently supported by other browsers.

The let keyword is included ECMAScript 6 specification.

Corneliu
  • 2,932
  • 1
  • 19
  • 22
4

I know this question has been answered, but if you want super for loops, write them like this:

var names = ['alex','john','paul','nemo'],
    name = '',
    idx = 0,
    len = names.length;

for(;idx<len;++idx)
{
    name = names[idx];
    // do processing...
}

A couple of things going on here...

  1. The array length is being stored in len. This stops JS evaluating names.length every iteration

  2. The idx increment is a PRE-INCREMENT (e.g. ++idx NOT idx++). Pre-increments are natively faster than Post-increments.

  3. The storing of a reference to name. This is optional but recommended if you'll be using the name variable a lot. Every call to names[idx] requires finding the index in the array. Whether this search be a linear search, tree search or hash table, the find is still happening. So store a reference in another variable to reduce lookups.

Finally, this is just my personal preference, and I have no proof or any performance benefits. However I always like initialising variables to the type they're going to be e.g. name = '',.

serv-inc
  • 35,772
  • 9
  • 166
  • 188
AlexMorley-Finch
  • 6,785
  • 15
  • 68
  • 103
  • 3
    By removing the initialiser from the loop itself you now have to reset `idx = 0` after each loop if you want to use the same identifier (which is what the question is about). – James Allardice Dec 16 '13 at 21:29
1

The best practice is to reduce the scope of variables, so the best way to declare iteration variable for the loops is

//for-loop 1
for (var i = 0; ...; i++) { ... }

//for-loop 2
for (var j = 0; ...; j++) { ... }

I know the scope of the variables declared with var but I am taking about code readability here.

Nikolay Kostov
  • 16,433
  • 23
  • 85
  • 123
  • 13
    Except that that isn't true in JavaScript. `var` **always** has function scope (or global scope, if outside of all functions). It does not have block scope. In ES6, we'll have `let` (once it gets broadly implemented), at which point this is a viable answer. But it makes no sense in today's world, with `var`, and is misleading. – T.J. Crowder Apr 03 '13 at 09:12
  • Yes, you are right, but it is not a good practice to declare all variables at the start of the code (like in C) – Nikolay Kostov Apr 03 '13 at 09:14
  • 2
    @ Nikolay: Yes, it is, if that's where the declaration actually occurs regardless of where you place it. Doing anything different is actively misleading. Actively misleading the people reading your code cannot be called "best practice." Best practice is to keep your functions short enough that declaring vars at the top **is** defining them close to where they're used. – T.J. Crowder Apr 03 '13 at 09:15
  • 1
    I am talking about code readability. It is better to see the variable just before you use it :) – Nikolay Kostov Apr 03 '13 at 09:17
  • 2
    @ Nikolay: *"Thank you for your minuses, but I am taking about code readability"* So am I. And so, I suspect, are several of the people saying this isn't useful. Re "just before you use it", did you read my comment above? Keep functions short, so the vars at the top **are** near where they're used. – T.J. Crowder Apr 03 '13 at 09:18
  • 7
    `Best practice is to keep your functions short enough that declaring vars at the top is defining them close to where they're used`. This should be way to code in any language. – Jashwant Apr 03 '13 at 09:18
  • 5
    I disagree with downvoters. "Mental" scoping is much more important than concrete scoping rules in a particular language. By putting "var" in `for(var i...` I'm telling the reader that `i` is going to be used in this block and nowhere else. Yes, technically it doesn't limit the scope of `i`, but makes the intention clear. – georg Apr 03 '13 at 09:26
  • 1
    @thg435 Or, it can mislead people that `i` is only visible in the `for` loop block. – Corneliu Apr 03 '13 at 09:29
  • @Corneliu: I'm assuming that whoever reads my program knows javascript (otherwise it wouldn't make much sense). – georg Apr 03 '13 at 09:31
  • @thg435: Grouping the declarations together is important in JavaScript because multiple declarations are not an error, as they are in several other languages. Littering them throughout the code is asking for a maintenance error, when someone is using (say) `index` for something in the function as a whole, and someone else dives into the middle and does `for (var index = ...`, blowing up `index`'s original meaning. Which is why I advocate: 1. Short functions, and 2. All `var` declarations at the top of the function, where you can readily see what's being used. Oh, and not misleading people. ;-) – T.J. Crowder Apr 03 '13 at 11:24
  • @T.J.Crowder: In a short function all var's usages are immediately obvious and such errors are never going to happen, which makes the requirement (2) redundant. So basically you're only advocating "short functions" and nothing more. That's fine with me, but I guess it's a bit different topic. My problem with "all vars on top" is that some of them (e.g. loop counters) cannot be initialized in a meaningful way, and declaration without initialization violates RAII and is bad style in most cases. – georg Apr 03 '13 at 11:53
  • Within the context of **JSLint** (a tag on this question), that's bad code. **It doesn't reduce scope** b/c of hoisting and, as written, **causes two JSLint errors** for not acknowledging that same hoisting. Without that tag, you've got a subjective argument, but with, you're "Crockfordian Wrong". – ruffin Apr 03 '13 at 22:09