4
\$\begingroup\$

I'm using node-ftp to download some files from an FTP server; due to nodeback difficulty and nested calls I'm promisified some of the calls, hoping that this will help get me out of pyramid code hell, but I think I'm still mis-applying something.

Before I get to my code pasted below, conceptually the pseudo-code of what I'm trying to do is:

foreach directory in a list:
    list files in directory
    filter to only files of interest
    download those files, save them to /tmp/whatever.

The objective is to end with a list of files downloaded and local paths.

I have code that works, but it's god-awful ugly and is not living up to the "promise of promises". How can I re-arrange this to be more like the pseudo code above, and aggregate the results? Part of the trouble is that the "list files in directory" is a promise, and each download returns a promise of a data stream. So unless I do this smartly, it's a 3-level nested array of promises that all have to be resolved, with the very bottom level promise results the intended return value.

//Main handler: when the connection is ready, start doing stuff.
c.on('ready', function() {
    config.clientCheckPaths
    .map(function (lookInPath) {    
        console.log('Examining ' + lookInPath);

        // Go get directory contents.
        listAsync(lookInPath)
        .then(function (files) {
            console.log('Files: ' + files.map(function (i) { return i.name; }));

            var wantedFiles = files.filter(function (item) {
                return config.isFileWanted(item.name);
            });

            // Filter it down to only the files we want.
            var arrayOfDownloadPromises = wantedFiles.map(function (x) {
                // For each file, go download it.
                return new Promise(function (resolve, reject) {
                    var fullPath = lookInPath + '/' + x.name;

                    console.log("Wanted file: " + lookInPath + '/' + x.name + ", " + Object.keys(x));

                    getAsync(fullPath)
                    .then(function (stream) {
                        // With the stream, name it something reasonable on the disk.
                        var localFileName = '/tmp/' + fullPath.replace(/[^A-Za-z0-9\.]/g, '_');
                        console.log("Writing stream to " + localFileName);
                        stream.once('close', function() {;});
                        stream.pipe(fs.createWriteStream(localFileName));

                        resolve(localFileName);
                    }, function(err) {
                        var msg = 'Getting file ' + fullPath + ' failed with ' + err;
                        console.error(msg);
                        reject(msg);
                    })
                });
            });

            // We then need to re-join all of the download promises and proceed 
            // when they're all finished.
            return Q.all(arrayOfDownloadPromises);
        }, function(err) {
            console.error('Listing directory ' + lookInPath + ' failed with ' + err);
        });
    })
    .map(function (promise) {
        console.log('Final: ' + promise);
    });
});

This code performs the right actions now, but doesn't aggregate the results correctly, and is ugly. How to refactor this when I have multi-level nested promises?

\$\endgroup\$
  • 1
    \$\begingroup\$ Much of your pyramid is due to your personal choice of coding style, half of the rest has nothing to do with promises at all, you'll get some indent when using .map. Which begs the question ... your first .map will result in an array of undefined being logged as the "Final Promise" , because nothing is being returned in function(lookInPath) - line 3 \$\endgroup\$ – Jaromanda X Dec 10 '15 at 2:44
  • \$\begingroup\$ Is there a way to improve this coding style to flatten it? As this is written, it works (except for the final promise) but it's pretty ugly. \$\endgroup\$ – FrobberOfBits Dec 10 '15 at 2:47
  • \$\begingroup\$ Questions about working code go on codereview.SE. \$\endgroup\$ – djechlin Dec 10 '15 at 3:13
  • \$\begingroup\$ You've got two nested loops. Of course they will form a pyramid. \$\endgroup\$ – Bergi Dec 10 '15 at 4:06
  • \$\begingroup\$ Avoid the Promise constructor antipattern! \$\endgroup\$ – Bergi Dec 10 '15 at 4:06
3
\$\begingroup\$

Here's a quick attempt to clean it up:

c.on('ready', function() {
  return Promise.all(config.clientCheckPaths.map(function (lookInPath) {    
    console.log('Examining ' + lookInPath);
    // Go get directory contents.
    return listAsync(lookInPath).then(function(files){
      return files.map(function(i){
        i.lookInPath = lookInPath;
        return i;
      });
    })
  }))
  .then(flatten)
  .then(function (files) {
    console.log('Files: ' + files.map(function (i) { return i.name; }));
    // Filter it down to only the files we want.
    return Promise.all(
      files.filter(function (item) {
        return config.isFileWanted(item.name);
      }).map(downloadWantedFiles)
    );
  });
});

function downloadWantedFiles(x) {
  // For each file, go download it.
  var fullPath = x.lookInPath + '/' + x.name;
  console.log("Wanted file: " + x.lookInPath + '/' + x.name + ", " + Object.keys(x));
  return getAsync(fullPath)
    .then(writeLocalFile(fullPath))
    .catch(couldNotDownload(fullPath))
  });
}

function writeLocalFile(fullPath) {
  return function (stream) {
    // With the stream, name it something reasonable on the disk.
    var localFileName = '/tmp/' + fullPath.replace(/[^A-Za-z0-9\.]/g, '_');
    console.log("Writing stream to " + localFileName);
    var write = stream.pipe(fs.createWriteStream(localFileName));
    // Only resolve once the local file has been written to disk
    return new Promise(function(resolve, reject){
      write.on('end', resolve.bind(null, localFileName));
      write.on('error', reject);
    });
  }
}

function couldNotDownload(fullPath) {
  return function (err) {
    var msg = 'Getting file ' + fullPath + ' failed with ' + err;
    console.error(msg);
    return Promise.reject(err);
  }
}

function flatten(arr) {
  return arr.reduce(function (memo, val) {
    var args = Array.isArray(val)
      ? flatten(val)
      : [val];
    memo.push.apply(memo, args);
    return memo;
  }, []);
}
\$\endgroup\$
  • \$\begingroup\$ You can flatten these lines? return listAsync(lookInPath).then(function(files){ return files.map(function(i){ \$\endgroup\$ – djechlin Dec 10 '15 at 3:10
  • \$\begingroup\$ @djechlin yeah, it's an array of arrays. An array of directories where each item is an array of files within that directory. \$\endgroup\$ – idbehold Dec 10 '15 at 3:14
  • 1
    \$\begingroup\$ Notice that flattening and calling Promise.all in a row (instead of nested) does alter the concurrency behaviour of the code. \$\endgroup\$ – Bergi Dec 10 '15 at 4:08
  • 1
    \$\begingroup\$ +1 for the refactoring of downloadWantedFiles \$\endgroup\$ – Bergi Dec 10 '15 at 4:09
0
\$\begingroup\$
  1. It's important to always check whether you actually need to use .map. As you probably know, the function returns a new array. Unless you'll be using the result of .map as an array, you can typically forEach.

  2. As others have stated, most of your pyramid structure is due to code style. Some of the things you do in each step of the pyramid should really be their own functions.

  3. Once your functions have been refactored to an acceptably DRY/SOLID state, you can usually write a top level promise that reads like a story. This is great because it comes out human readable and it may act as an index throughout your file.

  4. If you get in the habbit of using .apply/.bind it gets easier to refactor promises into a readable chain.

  5. To truly refactor this beyond my coming suggestion, you should consider making Typed Errors and only rejecting with them. This allows you to handle thrown errors outside of the main path of your promise chain. It would move all of your rigid error reporting away from the rest of the functions.

Below is my quick suggestion, it's not too far from other suggestions, but having a readable promise chain at the top makes the code a bit more readable/maintainable.

//Main handler: when the connection is ready, start doing stuff.
c.on('ready', function () {
  downloadWantedFiles();
});

// --- Note that this is a very human readable index into the function's structure
function downloadWantedFiles() {
  return listAllFiles(config.clientCheckPaths)
    .then(filterForWanted)
    .then(downloadFilesAsync)
    .then(reportSuccess)
  ;
}

// ---

function listAllFiles(checkPaths) {
  // Go get directory contents.
  return Promise.all(checkPaths.map(function (checkPath) {
    console.log('Examining ', checkPath);

    return listAsync(checkPath)
      .catch(function (err) {
        console.lerr('Listing directory ' + checkPath + ' failed with ' + err);
      })
    ;
  }))
  .then(function (fileArrays) {
    // Flatten file arrays
    return [].concat.apply([], fileArrays);
  });
}

function filterForWanted(files) {
  return files.filter(function (file) {
    return config.isFileWanted(item.name);
  });
}

function downloadFilesAsync(files) {
  return Promise.all(files.map(downloadFile));
}

function downloadFile(filePath) {
  return getAsync(filePath)
    // Should be bound to context of module/class/etc, I don't see one in your code so I bind null
    .then(saveFile.bind(null, filePath))
    .catch(handleDownloadFailure.bind(null, filePath))
  ;
}

function reportSuccess(files) {
  files.forEach(function (file) {
    console.log('Final: ' + file);
  })
}

function saveFile(downPath, stream) {
  var localFileName = ['/tmp/', downPath.replace(/[^A-Za-z0-9\.]/g, '_')].join();
  console.log("Writing stream to " + localFileName);

  stream.once('close', function () {;});
  stream.pipe(fs.createWriteStream(localFileName));

  return Promise.resolve(localFileName);
}

function handleDownloadFailure(filePath, err) {
  var msg = 'Getting file ' + filePath + ' failed with' + err;
  console.error(msg);
  return Promise.reject(msg);
}
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy