4
\$\begingroup\$

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(...
\$\endgroup\$
  • 2
    \$\begingroup\$ You're basically doing what util.promisify already does for you. \$\endgroup\$ – elclanrs Mar 23 '18 at 3:43
3
\$\begingroup\$

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();
\$\endgroup\$
  • \$\begingroup\$ I am unclear what this means: const { promisify } = require('util'); Is it like: const util = require("util"); const promisify = util.promisify; \$\endgroup\$ – Martin York Mar 23 '18 at 18:55
  • \$\begingroup\$ @MartinYork yep, that's exactly what it does, it's called destructuring \$\endgroup\$ – Gerrit0 Mar 23 '18 at 18:55

Your Answer

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

Not the answer you're looking for? Browse other questions tagged or ask your own question.