trjhtr

Using javascript functions and the old way of doing things I wrote a piece of code to copy the first file in a directory to a file Test.

console.log("Copy First File");
var fs = require('fs');
fs.readdir(".", function(err, files) 
 if (err) console.log(err);process.exit(1);

 fs.exists(files[0], function(exits) 
 fs.readFile(files[0], function(err, data) 
 if (err) console.log(err);process.exit(1);

 fs.writeFile('Test', data, function(err) 
 if (err) console.log(err);process.exit(1);

 console.log("Data Copied");
 );

 )
 );
);

After reading about Promises (and several failed attempts). I managed to convert the above to use Promise and .then() chaining.

function getAListOfFiles() 
 return new Promise(function(resolve, reject) 
 fs.readdir(".", function(err, files) 
 if (err) 
 reject(err);
 
 else 
 resolve(files);
 
 );
 );

function checkFirstFileExists(files) 
 return new Promise(function(resolve, reject) 
 fs.exists(files[0], function(exists) 
 if (!exists) 
 reject("No File");
 
 else 
 resolve(files[0]);
 
 );
 );

function readFileData(file) 
 return new Promise(function(resolve, reject) 
 fs.readFile(file, function(err, data) 
 if (err) 
 reject(err);
 
 else 
 resolve(data);
 
 );
 );

function writeFileData(data) 
 return new Promise(function(resolve, reject) 
 fs.writeFile('Test', data, function(err) 
 if (err) 
 reject(err);
 
 else 
 resolve("A-OK");
 
 );
 );


console.log("Copy First File");
var fs = require('fs');

new Promise(function(resolve, reject) 
 resolve(1);
)
.then(getAListOfFiles)
.then(checkFirstFileExists)
.then(readFileData)
.then(writeFileData)
.then(function(comment) 
 console.log("Data Copied " + comment);
)
.catch(function(err) 
 console.log("Error: " + err);
);

I want to know is my use of Promise and chaining idiomatic? What can I do better? It seems much longer than the original and thus slightly harder to read.

Particularly interested if:

new Promise(function(resolve, reject) 
 resolve(1);
)
.then(getAListOfFiles)
.then(...

Is more idiomatic than

getAListOfFiles()
.then(...

For the most part, your use of promises is idiomatic. Good work! Most people new to promises fail to chain them and end up with a similar pyramid to your original code.

1. fs.exists is deprecated, you should either use fs.stat, fs.access, or just catch the exception when trying to read it. Additionally, the use of fs.exists introduces a race condition where the file could be deleted between the exists call and the read call.




2. If the first file turns out to be very large, you may run into memory issues since you first read the file into memory and then write it out to another file. Thankfully, you can use fs.copyFile to resolve this, and also reduce the amount of code you have to write. If you are stuck with a version of Node older than 8.5.0, you can use fs.createReadStream and fs.createWriteStream to copy the file without the memory issues.




3. As elclanrs mentioned in the comments, you can use util.promisify to automatically wrap some functions in a promise based api. It was added in 8.0.0, but is fairly simple to implement.



4. 
   

   You should not construct a promise to just return a value.

   
   
   

   new Promise(function(resolve, reject) 
    resolve(1);
   )
   

   
   
   

   is equivalent to

   
   
   

   Promise.resolve(1)
   

   
   
   

   In your case, since you don't actually use the return value, it is better to just begin the promise chain with getAListOfFiles(), and even if you did use the return value, you should just start with getAListOfFiles(1).

   
   

Here's how I would write this script with promises:

const promisify = require('util');
const fs = require('fs');

const readdir = promisify(fs.readdir);
const copy = promisify(fs.copyFile);

readdir('.')
.then(files => copy(files[0], 'Test'))
.then(() => console.log('Data copied!'))
.catch(console.error);

Bonus: It's a really good idea to understand how promises work before using async / await, but once you do, async functions can help make the logic look more synchronous. In this case, there isn't really a benefit, but for more complex situations, it can help immensely.

const promisify = require('util');
const fs = require('fs');

const readdir = promisify(fs.readdir);
const copy = promisify(fs.copyFile);

async function main() 
 try 
 const files = await readdir('.');
 await copy(files[0], 'Test');
 console.log('Data copied!');
 catch (err) 
 console.error(err);
 


main();