For-loop incrementing to limit before being passed to function

Question

I have some code to copy files contained in an array for each source and destination directory in a dirs array. Each iteration through the loop, it calls the function that copies. It looks like this:

var filesInEachDir ["file1", "file2", "file3"];
var dirs = [
  {"source": "sourceDirectory1", "dest":"destinationDirectory1"},
  {"source": "sourceDirectory2", "dest":"destinationDirectory2"},
  {"source": "sourceDirectory3" "dest":"destinationDirectory3"},
];

for (var i = 0; i < dirs.length; i++){
  fs.mkdir(dirs[i], function(err){
    if(err){
      console.log(err);
    }else{
      copyFiles(dirs[i], filesInEachDir);
    }
   });
}

function copyFiles(dirs, files){
  for (var c = 0; c < files.length; c++){
    fs.copy(files[c], dirs.source, dirs.dest, {replace: false}, function(err){
      if (err){
        console.log(err);
      }else{
        console.log('file copied');
      }
    });
  }
}

For some reason, only the files in the last element of dirs are copied. If I add another element to dirs, its files are copied, and not any other. So it looks like i is incrementing fully before calling the copyFiles function. Why is this happening? How can I give copyFiles each incrementing value of i?

What is `fs.mkdir`? Is the function parameter it is passed called asynchronously? That would cause this problem. — IMTheNachoMan, Jul 22 '15 at 22:22
@Paulpro the `forEach` method suggested in the post you marked as a duplicate hasn't solved my issue. I don't think this is the same issue. I think it has to day with `mkdir`'s asynchronous nature. Are you certain this is the same thing? — 1252748, Jul 22 '15 at 22:34
It's the same thing, and the usual cause is asynchronous callbacks. A forEach loop should fix it: `dirs.forEach( function ( dir ) { fs.mkdir( dir, function ( ) { ... else{ copyFiles( dir, filesInEachDir ); } }); });` — Paul, Jul 22 '15 at 22:38
@Paulpro will you re-open my question? The solution in the question you've marked this as a duplicate of doesn't work in my situation. — 1252748, Jul 23 '15 at 00:55
@thomas I reopened it, though I suggest rereading your question and making sure that what you post is syntactically correct and fully reproduces your error. Your call to `fs.mkdir` doesn't seem to be closed, there is no `})`. — Paul, Jul 23 '15 at 03:29
@Paulpro thanks for the pointer. I did make a small reproduction of the entire thing just to make it simpler for the post. When I'm back on a real computer tomorrow, I'll check the syntax errors are just a result of that and not my actual code. Thanks very much for re-opening :) — 1252748, Jul 23 '15 at 03:32
@vp_arth I don't think I understand what you mean. Can you elaborate. Thanks — 1252748, Jul 23 '15 at 15:56

Tomalak · Answer 1 · 2015-07-23T16:02:58.507

You have the classic problem caused by using a for loop over an asynchronous function: The loop finishes long before any of the functions it calls does, so when the first function actually starts to do something, the loop index it refers to has already been overwritten.

Use .forEach instead to avoid overwriting your loop indexes.

var filesInEachDir = ["file1", "file2", "file3"];
var dirs = [
  {"source": "sourceDirectory1", "dest":"destinationDirectory1"},
  {"source": "sourceDirectory2", "dest":"destinationDirectory2"},
  {"source": "sourceDirectory3", "dest":"destinationDirectory3"}
];

dirs.forEach(function (dir) {
  fs.mkdir(dir, function(err) {
    if (err) {
      console.log(err);
    } else {
      copyFiles(dir, filesInEachDir);
    }
  });
});

function copyFiles(dir, files) {
  files.forEach(function (file) {
    fs.copy(file, dir.source, dir.dest, {replace: false}, function(err) {
      if (err) {
        console.log(err);
      } else {
        console.log('file copied');
      }
    });
  });
}

It comes down to scoping your variables properly. In your code, i is in a higher scope, when looking from within your mkdir callback. The contents of i is controlled externally, by the for loop. You want a local array index instead.

You can use an IIFE to close over i and create a callback that has its own copy of it:

for (var i = 0; i < dirs.length; i++){
  fs.mkdir(dir, (function (i) {
    return function(err) {
      if (err) {
        console.log(err);
      } else {
        copyFiles(dirs[i], filesInEachDir);
      }
    };
  })(i));
}

or better yet

for (var i = 0; i < dirs.length; i++){
  fs.mkdir(dir, (function (dir) {
    return function(err) {
      if (err) {
        console.log(err);
      } else {
        copyFiles(dir, filesInEachDir);
      }
    };
  })(dirs[i]));
}

...and that is technically equivalent to using .forEach, it's just not as easy on the eye.

Hi, thanks for walking me through this. Are you missing some paranthesis before the returned function. JS hint is telling me that it's `expecting and assignment or function call near the bottom of both your second and third examples. — 1252748, Jul 23 '15 at 15:37
Can you explain what is happening with `(dirs[i])` being given as another parameter in another set of parenthesis to `fs.mkdir`? — 1252748, Jul 23 '15 at 15:52
Also, making both of my loops into `forEach` hasn't corrected the problem. Are the other two methods truly identical? — 1252748, Jul 23 '15 at 15:55
That's not "another set of parentheses", its the parameter to the function call. Contemplate the setup for a bit and you will see it. :) — Tomalak, Jul 23 '15 at 15:55
Let us [continue this discussion in chat](http://chat.stackoverflow.com/rooms/84096/discussion-between-thomas-and-tomalak). — 1252748, Jul 23 '15 at 16:03

For-loop incrementing to limit before being passed to function

1 Answers1