Instantly share code, notes, and snippets.

Embed
What would you like to do?
Simple node.js code style tips to improve code quality

Whether you use 2 spaces or 4 spaces, there are a few simple things that can make your node.js code easier to read. We've been using them in all the hapi modules for over 4 years now to great results. This list is by no means complete but it highlights the most useful elements that will give you immediate value in reducing bugs.

Required modules

JavaScript makes it harder than most languages to know where variables are coming from. Variables assigned required modules are particularly important because they represent a singleton object shared with the entire application. There are also globals and module globals, along with function variables and arguments.

Traditionally, variables starting with an uppercase letter represent a class that must be instantiated using new. This was an important semantic in the early days of JavaScript but at this point, if you don't know Date requires new Date() you are probably very new. We have adopted Upper Camel Case variable names for all module global variables which are assigned required modules:

var Hapi = require('hapi');

Note that you cannot new Hapi(), only new Hapi.Server(). In this style, the exported object from the modules always exposes an object which contains the API. This means a single function module should still export an object with the single method as an object property:

exports.add = function (a, b) {

    return a + b;
};

This makes it trivial to identify which variables in your code represent required modules. The language itself does contain a few uppercase variables but those are well known and should not cause any confusion.

Module classes

When a module (any node.js file) contains its own prototype class, that class variable also starts with an uppercase which can cause confusion. However, we do not allow any module global variable except for one: internals (and of course whatever node.js provides).

var Hapi = require('hapi');

var internals = {};

internals.Server = function () {

    this.server = new Hapi.Server();
    this.server.connection();
};

internals.server = new internals.Server();

// This is not allowed:

var server = new internals.Server();

This means that within any function in your code, uppercase variables are either required modules APIs or JavaScript natives. Any internals prefixed variables are module globals which act as a singleton. The rest are either function variables or arguments which are easier to spot because of the smaller scope.

A note about singletons - I often see bugs caused by developers using a module global variable used by a prototype. For example:

var internals = {};

internals.storage = {};

exports.Cache = internals.Cache = function () {

};

internals.Cache.prototype.get = function (key) {

    return internals.storage[key];
};

internals.Cache.prototype.set = function (key, value) {

    return internals.storage[key] = value;
};

The problem is that multiple instances of the cache will all share the same memory:

var Cache = require('./cache');

var cache1 = new Cache();
var cache2 = new Cache();    // Uses the same memory cache1 uses

By requiring the use of internals as a prefix, this bug becomes obvious to spot. When I code review a pull request, I always search for all the instances of internals to make sure it only holds static configuration and other safe data to share between instances. This is much harder to spot without the prefix.

Callback new lines

Understanding your code flow is critical in asynchronous development. Consider this:

internals.read = function (db, key, callback) {
    db.get(key, function (err, value, meta) {
        if (err) {
            return callback(err);
        }

        if (meta.age > 10000) {
            return callback(null, null);
        }

        return callback(null, value);
    });
};

At a quick glance, it is not obvious that the db.get() method is asynchronous. It looks just like any other scope indentation. By adding a new line after every function declaration, we make the callbacks "pop" and much easier to spot:

internals.read = function (db, key, callback) {

    db.get(key, function (err, value, meta) {

        if (err) {
            return callback(err);
        }

        if (meta.age > 10000) {
            return callback(null, null);
        }

        return callback(null, value);
    });
};

Allowing your eyes to immediately identify those line breaks means you can quickly note where your code switched event loop ticks.

return callbacks

You might have noticed that every callback() call is prefixed with return. This is a defensive programming tool. It provides both a visual cue where in the code execution ends and returned up the stack, but also protection against adding code later after that point that can create timing issues, race conditions, or multiple callback calls.

It is sometimes helpful to add a return when it is not obvious a nested function contains a callback. Consider the example above, it can be made safer with an extra return either in front of the method:

internals.read = function (db, key, callback) {

    return db.get(key, function (err, value, meta) {

        if (err) {
            return callback(err);
        }

        if (meta.age > 10000) {
            return callback(null, null);
        }

        return callback(null, value);
    });
};

or immediately after (usually with a comment):

internals.read = function (db, key, callback) {

    db.get(key, function (err, value, meta) {

        if (err) {
            return callback(err);
        }

        if (meta.age > 10000) {
            return callback(null, null);
        }

        return callback(null, value);
    });

    return;    // db.get() invokes the callback
};

callback vs next

There are many cases where a callback doesn't have to be called on the next tick for performance or workflow reasons. However, that can create an unpredictable API where it is not clear in which tick the callback is invoked. To make it explicit, I use callback when it is guaranteed the callback is called on another tick, and next when it can be both. It is a simple naming convention that has made maintaining hapi much simpler over many months.

exports.a = function (a, b, callback) {

    return process.nextTick(function () {

        return callback(null, a + b);
    });
};

exports.b = function (a, b, next) {

    return next(null, a + b);
};
@mark-bradshaw

This comment has been minimized.

mark-bradshaw commented Feb 3, 2015

Thanks Eran. Since working with Hapi I've adopted the extra line after function calls, and it quickly became a favorite style addition.

@AdriVanHoudt

This comment has been minimized.

AdriVanHoudt commented Feb 3, 2015

Thanks! I like the explanation behind every style you follow. Question though. Don't you feel using that many returns feels like clutter quickly? Because in the example given you have 5 returns for a simple db get. To me it feels like only putting return where you actually return a callback.

@jmmitchell

This comment has been minimized.

jmmitchell commented Feb 3, 2015

Eran, it does not appear that I can make a pull request on a gist via github, so here goes with an out-of-band notice for a minor spelling correction that can be seen in my fork: https://gist.github.com/jmmitchell/c9d64d04b42639be149a

@spencerwi

This comment has been minimized.

spencerwi commented Feb 3, 2015

Minor critique:

We have adopted CamelCase variable names for all module global variables which are assigned required >modules:

var Hapi = require('hapi');

What you're looking for is Pascal Case or Upper Camel Case.

@hueniverse

This comment has been minimized.

Owner

hueniverse commented Feb 4, 2015

@spencerwi @jmmitchell thanks for the corrections.

@AdriVanHoudt I think it's easier to follow than a nested if...else statement.

@jaw187

This comment has been minimized.

jaw187 commented Feb 4, 2015

👏 👏 👏 👏

@joseph-norman

This comment has been minimized.

joseph-norman commented Feb 5, 2015

Great Stuff Eran.

@zackiles

This comment has been minimized.

zackiles commented Feb 7, 2015

You certainly nailed my two pet peeves when I switched from C# to JS. Camel cased external objects/classes, and readability of callback chains. I've actually seen several guides say specifically to use pascal cased for external/required modules which actually results in a several annoyances. I tend to use descriptive function parameters so I find annoyances like this

books = require('books')
books.getBooks().then(function(books){ ... })

Here I'm unable to cleanly use the 'books' name again, and probably have to use 'results' or something similar in the resolve instead. Pascal is one of those OOP constructs that clearly has a purpose and I have no idea why JS developers decided to abandon it. I feel anyone who has to grep a large codebase would never do this themselves, but I think 80%+ of npm modules are like this.

@echorohit

This comment has been minimized.

echorohit commented Jan 11, 2016

Thank you Eran.

@afgallo

This comment has been minimized.

afgallo commented Mar 6, 2016

Thanks Eran, that's pretty good. I've been reading a lot about Hapi.js and started using it in a new project (having a great experience so far!). I'm also new to node.js but have a strong background in C#. This might sound like a silly question but I was just wondering where the Class notation fits in. Take the example below:

class UserHandler {
  constructor() {
    ...
  }

  create(request, reply) {
    ...
    return reply('ok').code(201);
  }
}

module.exports = UserHandler;

This is not commonly seen (I haven't come across any examples like it) within the hapi.js community. Instead, what I actually see most of the time is something like the below:

// user-handler.js
module.exports = function create(request, reply) {
    ...
    return reply('ok').code(201);
}

Is there anything wrong with the Class notation approach? I do I realise I am exporting a whole class definition in approach 1 and just a function definition in approach 2, I just wanted to clarify the lack of usage on 1.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment