4
\$\begingroup\$

The following code, written in synchronous Node.js and intended to be used as a shell script, nests a try/catch inside a conditional.

Because both the conditional and the try/catch serve a similar purpose - respectively, check a file exists, and check that reading and writing to it succeeds - it feels clumsy that the code uses two different mechanisms. The nesting may also be unnecessary.

What would be a more readable way to write this code?

#!/usr/bin/env node
var fs = require('fs');
var path = process.argv[2];
var data = "#!/usr/bin/env node\n\n";
if (fs.existsSync(path)) {
 try {
 data += fs.readFileSync(path);
 fs.writeFileSync(path, data);
 } catch (err) {
 console.log("Problem reading or writing " + path + " : " + err.message)
 process.exit(1);
 }
} else {
 console.log("Invalid path");
 process.exit(1);
}
asked Aug 14, 2014 at 4:44
\$\endgroup\$

1 Answer 1

2
\$\begingroup\$

In general, a good strategy is to return early as soon as you detect an error. Putting the error handler for fs.existSync(path) in an else block far away from where the check occurred makes the code harder to follow, and increases the amount of nesting required.

Also, the error message should inform the user of which path is invalid.

#!/usr/bin/env node
var fs = require('fs');
var path = process.argv[2];
var data = "#!/usr/bin/env node\n\n";
if (!fs.existsSync(path)) {
 console.log("Invalid path: " + path);
 process.exit(1);
}
try {
 data += fs.readFileSync(path);
 fs.writeFileSync(path, data);
} catch (err) {
 console.log("Problem reading or writing " + path + " : " + err.message)
 process.exit(1);
}
answered Aug 14, 2014 at 5:47
\$\endgroup\$

Your Answer

Draft saved
Draft discarded

Sign up or log in

Sign up using Google
Sign up using Email and Password

Post as a guest

Required, but never shown

Post as a guest

Required, but never shown

By clicking "Post Your Answer", you agree to our terms of service and acknowledge you have read our privacy policy.