A small JavaScript Refactoring example

A small but real-world example of JavaScript code refactoring using the lodash utility belt (and common sense).
By Angelos Pikoulas Posted 21 December 2015

A small JavaScript Refactoring example

What is refactoring anyway ?

Code refactoring is defined in wikipedia as:

The process of restructuring existing computer code – changing the factoring – without changing its external behavior. Refactoring improves nonfunctional attributes of the software.

It all sounds great, but how exactly do we proceed with existing code that is in production, written a long time ago and for which few people understand why it was written the way it is ?

The idea is to look for repeating patterns of behavior in code written in an old fashion way (eg imperative) and replace it with more functional counterparts that replicate the same behavior, but in a more concise & declarative way. This is an iterative process, where while the mud is cleared off, more good (or bad) patterns reveal themselves and we can clearly see the true intention of the code.

Utilities are promoters

Not too long ago we decided to employ lodash (a descendant of underscore) as our utility belt, with the premise that it will gradually replace existing custom utilities and will also simplify the code around them. Lodash promotes a more compact and concise style of programming, since it's built on functional ideas from languages like erlang, haskell etc. The result is that many custom non-business code is replaced by its reusable functional style facilities and the business-related code becomes more clear and decoupled.

One subtle objective we had was to remove custom array utils like concatWithoutDuplicate, pushWithoutDuplicate, removeNullElements and various similar ones. But even more importantly, we would also have a go and refactor the code around those, often also deleting obsolete code fragments and functions/methods that are inefficiently built around them.

In the end, whole code fragments will be replaced with more declarative lodash two-liners. But most crucially, with all the mud cleaned off, we will be able to see at a glance what a part of the code :

  • is supposed to be doing.

  • if it's named properly.

  • whether its really where it belongs.

  • whether its really needed at all.

All these are crucial aspects of writing solid and clean code that others (and you) can understand and reason about.

Pushing is old (and imperative)

An Array.push operation often represents an old style of imperative programming. Its much easier to go from one data structure / collection to another using a functional, almost declarative, way of a _.map and perhaps then _.filter, or applying a _.remove or a _.uniq to the result, instead of a custom function where we are declaring temporary variables, looping, checking, pushing, returning etc.

So we started looking at the pattern around where pushWithoutDuplicate was used. It is commonly used with a similar pattern inside a utility function like createAnArrayOfUniquePropertiesFromObjectsInSomeCollection where:

  • a new array (the result) is created, along with the temp index vars like i, j etc

  • a for loop iterates through the collection

  • for each item (an object) in the collection we retrieve a property from it (eg appName)

  • we then perhaps make some checks and push or directly push to the array with out duplicates, using pushWithoutDuplicate

  • after the loop is done, we finally return that array result

Real example

As an example of this pattern, lets look at the original code of such a straightforward case :

var FORM_TYPES = require('../Capture/FORM_TYPES'),
    arrayUtils = require('../../Shared/Utils/Array');

 function getAllAvailableAppNames(appsForms) {
    var i,
        appsAvailable = [];

    for (i = 0; i < appsForms.length; i++) {
        arrayUtils.pushWithoutDuplicate(appsForms[i].appName, appsAvailable);
    }

    return appsAvailable;
}

function AppsFormManager(appsForms){
    var appsAvailable = [];

    this.formTypeId = appsForms.length <= 0 ? undefined : FORM_TYPES.APPS_FORM;
    appsAvailable = getAllAvailableAppNames(appsForms);

    this.getAppsAvailable = function () {
        return appsAvailable;
    };
}

module.exports = AppsFormManager;

At a first glance, in these ~25 lines of code (without jsdoc / comments), we see a class-like / constructor that attaches a property and a function to the created this instance (that should be part of the function's prototype, but that's not relevant now).

It also uses a private function getAllAvailableAppNames that follows the pattern described above. This is where we start: simplifying the body of this function, realizing that it could simply become :

_.uniq(_.map(appsForms, function(appsForm){ 
    return appsForm.appName
}));

or even better :

_.uniq(_.pluck(appsForms, 'appName'))`

Further more we can completely remove the function getAllAvailableAppNames(){} and stick this new shorter body to the exported getAppsAvailable() function right away, since we also noted that getAppsAvailable() is called just once so there is no point in caching the result to the appsAvailable variable either. The refactored code is already quite shorter:

var FORM_TYPES = require('../Capture/FORM_TYPES');

function AppsFormManager(appsForms){

    this.formTypeId = appsForms.length <= 0 ? undefined : FORM_TYPES.APPS_FORM;

    this.getAppsAvailable = function () {
        return _.uniq(_.pluck(appsForms, 'appName'));
    };
}

module.exports = AppsFormManager;

But we're not quite there yet. We proceed with the optimization by deducting from the code that getAppsAvailable always returns the same result (i.e appsForms never changes), so it would be prudent to replace the function with a simple assignment this.appsAvailable = _.uniq(_.pluck(appsForms, 'appName')); and the whole code now becomes even shorter:

var FORM_TYPES = require('../Capture/FORM_TYPES');

function AppsFormManager(appsForms){
    this.formTypeId = appsForms.length <= 0 ? undefined : FORM_TYPES.APPS_FORM;

    this.appsAvailable = _.uniq(_.pluck(appsForms, 'appName'));
}

module.exports = AppsFormManager;

At this point, the tests will fail cause getAppsAvailable is spied on, and hence we need to assess whether these test make sense at all or if we need to alter them as well.

Observe and you will see

The crucial observation of this exercise is that we can now clearly see that the whole existence of AppsFormanager is to create instances (or maybe just a single instance), where each instance just holds two values on creation that never change (formTypeId and appsAvailable). It has no methods, nor complicated logic: it just holds two keys/values.

It serves no other purpose of abstracting some reoccurring behavior, its not even practically a class in the javascript sense, its just an object constructor that isn't subclassed or reused anywhere.

So, do we need this constructor ? If you're coming from a Java or C# background, almost the only way to create objects is create a class that instantiates them. In JavaScript you don't need that: object creation is as simple as { aProperty: 'of an Object' }, i.e on the fly creation of an object, which is really a HashMap of key/value pairs. All objects inherit from the base Object class, but if the properties & values are all you care about, you don't need the boilerplate of a custom class just to create instances that hold some specific properties / values.

Loose excess baggage

One good reason to create a class (i.e an instance constructor) would be the need to create those in many different places, so a constructor is something you reuse (i.e new SomeClassConstructor()). But actually by looking at our specific code that uses AppsFormManager, we can see that there is only one single instance being created by this constructor, in just one place. So if we can find a good place to store formTypeId and appsAvailable, then we definitely don't need this class / constructor at all.

But even if we do decide to keep it (eg for future extension reasons), we'll have this in mind and at least we'll be happy we've actually reduced it to the minimum required code. And we have code that can can understand and reason about with one glance.

Conclusion

We've only scratched the surface with a simple example and using only one of the hundreds functional patterns (_.map & its brother _.pluck) of libraries like lodash. The promise of employing the functional sides of JavaScript along with an eye to recognise what the code actually does and then using the right _.xxx to reflect that, goes a long way to creating more maintainable, extensible and testable code that is easier to understand and reason about.

Useful links

Angelos Pikoulas
Angelos Pikoulas
Software Engineer