4
\$\begingroup\$

This is a follow-on to my previous question: Enforcing set environment variables

While learning more about JavaScript, node, and the bluemix environment, I have been using the loading of process environment variables as a starting point for understanding how the systems work.

This part of the system checks to see if a 'default' value exists for an environment variable. If there is a default, and the variable is not set, then the variable is set to the default's value. Default values are stored in a file in a pre-defined directory (the same folder as the script file).

After taking in the review suggestions, and some other suggestions from colleagues, I have changed the code mechanism to use a map/filter/process system on the files. This makes the code quite a bit neater.

Note: this code is a file/module in a node.js application deployed in a Bluemix host.

Again I am looking for any further insights in to how the code can be improved, and if any edge cases exist. Performance is not critical, but as I am just learning JavaScript I would appreciate any insights in to any bad practices that should be avoided so that issues are avoided later on.

In particular, I am concerned that the code is not as asynchronous as it could be, though it is a requirement that all the variables are set before the code terminates (nothing asynchronous can be incomplete).

/*jshint node:true*/

/*
 * Load various pre-determined environment variables
 * (files in this folder with .env extension).
 * Only if they have not previously been set in the environment.
 * 
 * This makes the setting of Bluemix style variables quite easy.
 */

var fs = require('fs');
var path = require('path');

var env = /\.env$/;

function isEnv(fileName) {
    return fileName.match(env);
}

function processEnv(fileName) {
    var key = fileName.replace(env, "");
    if (process.env.hasOwnProperty(key)) {
        return;
    }
    var filePath = path.join(__dirname, fileName);
    var data = fs.readFileSync(filePath, 'utf8');
    process.env[key] = data;
}

fs.readdirSync(__dirname)
    .filter(isEnv)
    .forEach(processEnv);
\$\endgroup\$
2
  • \$\begingroup\$ Instead of re-inventing the wheel, might I suggest dotenv, the npm package? It effectively takes everything in the .env file in the cwd and sets it to environment variables. Posting this in a comment because you might want to reinvent the wheel. npmjs.com/package/dotenv \$\endgroup\$
    – Dan
    Commented Jul 24, 2015 at 8:34
  • \$\begingroup\$ @DanPantry - I was not intending to reinvent the wheel. dotenv looks like it should be what I am using, but it does not have the same functionality. I will probably rename my file extensions to something other than .env to avoid confustion... but, for example, I need to set VCAP_SERVICES to large strings like this example \$\endgroup\$
    – rolfl
    Commented Jul 24, 2015 at 11:23

1 Answer 1

3
\$\begingroup\$

This code is very clean and is definitely a clear improvement from your last version.

Your code is easy to understand and follow, now that's it's more broken up (aside from one big block of code).


I recommend storing your regexular expression for locating the extension .env like this:

var env = new RegExp("\.env$/", "");

The reason is for this: RegExp.test.

The test method of a RegExp object does the following:

  • Returns true if the regexular expression matches the input string.

  • Returns false if the regexular expression does not match the input string.

Now, your isEnv function does this:

return env.test(fileName);

Why is this better? Well...

  • The test method of RegExp only has to worry about the regexular expression matching at all.

  • The match method of a string has to worry about the regexular expression matching AND it has to worry about creating and returning an array of all the matches.

That being said, the test method is obviously faster than using match.

\$\endgroup\$

Your Answer

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

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