1
\$\begingroup\$

I've been doing some reading and tutorials. As such I've contrived a node based project to apply what I've learned. I thought this was a good example as it had some conditional async calls and error handling.

Any and all criticism of not following current JS best practices welcome.

/**
 * mkLocalCfg function, 
 * makes a local config directory structure and empty config file.
 * in the form "~/.config/nobjs/nobjs_config.json"
 * 
 * @param {mkLocalCfgCallback} cb - The callback that handles the response.
 * 
 */
function mkLocalCfg(cb) {
    //validate root folder exists, if not make it.
    if (!admin.hasHomeConfigDir()) {
        fs.mkdir(os.homedir() + "/.config", "775", function(err) {
            //stop here if you can't make it.
            if (err) {cb(err); return;}
        });
    }
    fs.mkdir(admin.getConfigLocation(false), "775", function(err) {
        //can't make config folder end it here
        if (err) {cb(err); return;}
        //write config file, return null or err on error
        fs.writeFile(admin.getConfigLocation(true), JSON.stringify({blogs: {}}), function(err) {
            if (err) {cb(err); return;}
            cb(null);
        });
    });
}
\$\endgroup\$
0
1
\$\begingroup\$

There's one main issue I currently see with your code and that is that if the admin does not have a home configuration directory (admin.hasHomeConfigDir()) then it will be created asynchronously. The issue with this it that your second call won't wait for this, leading to a race condition where you're trying to make (and write to) a path that might not exist yet.

One solution for this is to have a function inside your function that handles the "second half" of your creation (i.e. the config location creation and file writing).

function createConfiguration(callback) {

    // Make the configuration directory and write to the configuration.
    var writeConfiguration = function() {
        fs.mkdir(admin.getConfigLocation(false), "775", function(err) {
            if (err) {
                callback(err);
                return;
            }

            // Write the configuration, either forwarding an error to the
            // callback if an error occurs or nothing at all on success.
            fs.writeFile(admin.getConfigLocation(true), JSON.stringify({blogs: {}}), function(err) {
                if (err) {
                    callback(err);
                    return;
                }

                callback(null);
            });
        });
    };

    // If the admin home configuration directory exists, don't attempt to
    // create it.
    if (admin.hasHomeConfigDir()) {
        writeConfiguration();
    } else {
        // Otherwise create the directory and write the configuration.
        fs.mkdir(os.homedir() + "/.config", "775", function(err) {
            if (err) {
                callback(err);
                return;
            }

            writeConfiguration();
        });
    }
}

I tidied up your code a bit, mostly renaming things (e.g. renaming the function to have a simpler name, renaming arguments to be more readable) and rewording the comments.

Another issue I have noticed with your code is that if the directory that admin.getConfigLocation(false) returns already exists, an error will be thrown. You should probably check whether or not the configuration location exists before creating it.

Overall it's rather good and well written code, although I don't really have much experience in the field of "JS standards" so to speak.

\$\endgroup\$
3
  • \$\begingroup\$ Good stuff thank you. I guess the if(err) { callback (err); rerturn; } on one line stylisctically is a poor choice? I wouldn't normally do that, but in JS it seems like it would repeat everywhere and i wanted to get it out of the way. Also, correct use of return; statement to bail on error? \$\endgroup\$ May 25 '16 at 21:22
  • \$\begingroup\$ Sadly it's something that is used everywhere, but it seems most people still lay it out on separate lines just for readability. It's not usually recommended to lay out an if statement like that. return is fine to use when there is an error, as you're passing the error to the callback and you don't want to run any of the code after that. \$\endgroup\$ May 25 '16 at 21:24
  • 1
    \$\begingroup\$ i was reading this some more when i had more time, great point about the race condition and not waiting. Really valuable lesson here, its something to keep in mind going forward and exactly what i was looking for. async is different mind think. thanks. \$\endgroup\$ May 26 '16 at 3:54

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.