Promises turning into pyramids

Question

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?

Answer 1

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;
  }, []);
}

Answer 2

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);
}