Code Smells, Refactoring Long Functions – Bloaters: Day 2

Javier Arroyo · ‎02-25-2020

This article is Day 2 of a multi-day series about refactoring Code Smells. Code Smells are characteristics that can negatively impact code; ranging from hindered read-ability, clutter, unscale-able, to outdated programming practices.

Refactoring techniques used will be mostly, if not all, from the Imperative/OO perspective as ServiceNow OOTB code is all written as such. I will try to be brief and attempt to use language features specific to JavaScript that can differ from those used by classically trained coders. Observations that differ from classic code smell advice might be a result of JavaScript handling a smell differently.

Day 2 – Long Methods

The strategy to refactor long methods is a bit more complex than that used for Comments in Day 1. It can involve breaking down the method into functions, introducing a new Script Include to host long list of parameters, or making a method a Script Include of its own.

There isn’t a set line size to universally identify long methods. This is because long varies drastically from language to language. In JavaScript it has become rule of thumb to start considering refactoring a method when it gets to 10 lines, not allowing it to grow to more than 15 lines.

Why are long methods smelly?

Long methods allude to functions that do more than one thing
Functions that do multiple things are harder to reason about
Methods tend to grow over time, a long method will eventually grow longer
Scrolling up and down the screen to see items at top and bottom isn’t efficient
Can signal tight coupling
Are harder to maintain
Aren’t scale-able
Duplicate code by necessity
Harder to debug
Harder to unit test

Clues to help identify long methods

Comments inside the body of the method
Case statements are a practice left behind from procedural programming. In OO this can be replaced with polymorphism or objects
Conditional Statements
loops
Code groupings
variable lists
long parameter lists. In JavaScript this means more than 2 parameters. (this is a smell of its own)
more than 2 indentations

Example

Lets make believe that the OOTB function below is long and needs refactoring. 1. Because it's too long and 2. because it's cryptic.
My apologies for grabbing a smaller one, i'm shooting for brevity and maintaining my sanity.

var GlideUserSubscriptionAjax = {

    syncALicense: function () {

        var retJSON = {};
        var licenseSysID = this.getParameter('sysparm_license_sysID');

        if (JSUtil.nil(licenseSysID)) {
            retJSON.syncStatus = 'failure';
            return new JSON().encode(retJSON);
        }

        var licenseGR = new GlideRecord('license_details');
        licenseGR.get(licenseSysID);

        var syncHelper = new UserSetSubscriptionSyncHelper();
        var gr = syncHelper._getUserSetsForLicense(licenseSysID);

        if (gr != null && gr.getRowCount() == 0) {
            retJSON.syncStatus = "nosync";
            return new JSON().encode(retJSON);
        }

        retJSON = syncHelper.syncLicense(licenseGR);
        retJSON.syncStatus = 'success';

        return new JSON().encode(retJSON);
    }
};

The function above does various things, making it challening to follow what happens when and why. The reader has to analyze each line, having no continuity in the reading. If the code was any longer, changing it would be challenging.

Solution

Break the function into abstractions in order to gain meaning and create manageable smaller pieces, which also give the benefit of being portable; something they weren't inside that method.

The final method could be translated to:

function syncALicense() {
    var licenseSysID = this.getParameter('sysparm_license_sysID');

    //In OO Programming stop condtions are encouraged.
    if (missingLicenseSysID(licenseSysID)) {
        return encodeStatus(SyncStatus.failure);

    }

    var licenseDetails = getLicenseDetails(licenseSysId);
    var syncSubstription = tryToSyncUserSubscription(licenseDetails, licenseSysId);

    return encodePayload(syncSubstription);

}

or even using the more modern point free style

function syncALicense() {
    var licenseSysID = this.getParameter('sysparm_license_sysID');

    var syncPayload = missingLicenseSysID(licenseSysID) ?
                SyncStatus.failure :
                tryToSyncUserSubscription(getLicenseDetails(licenseSysId), licenseSysId);

    return encodePayload(syncPayload);
}

Now, the smaller functions to support the functionality which each allows to be tested and debug atomically, can be composed, shared, etc.

First, a Sync Status to group together the payloads for subscription. Perhaps this would be better name SubscriptionPayload. This item handles one and one thing only, doesn't need condition statements nor cases.

var SyncStatus = {
    'failure': {
        syncStatus: 'failure'
    },

    'nosync': {
        syncStatus: 'nosync'

    },

    'sucess': function (licenseDetails, syncHelper) {
        var payload = syncHelper.syncALicense(licenseDetails);
        payload.syncStatus = 'success';

        return payload;
    }

};

The function to actually carry out the subscription sync. Because of the related context of this function, it can be grouped together.
Notice that in Modern JavaScript, ternary operations are preferred over if/else. As is less temporary variables

function tryToSyncUserSubscription(licenseSysId, licenseDetails) {

    var syncHelper = new UserSetSubscriptionSyncHelper();
    var licenseSets = syncHelper._getUserSetsForLicense(licenseSysId);

    return hasRecords(licenseSets) ? SyncStatus.sucess(licenseDetails, syncHelper) : SyncStatus.nosync;

}

lastly, the helper functions to support the full functionality. Each one of them is nothing more but an idiomatic encapsulation of items that require more analysis than required for at-a-glance reading. They can also all be used independent of the main functionality, quickly proving that they can be shared in the rest of the codebase. A huge win for Don't Repeat Yourself.

function encodePayload(syncStatus) {
    return JSON.stringify(syncStatus);

}

function hasRecords(glideRecords) {
    return glideRecords && glideRecords.hasNext();

}

function getLicenseDetails(licenseSysId) {
    var license = new GlideRecord('license_details');
    license.get(licenseSysID);

    return license;
}

function missingLicenceSysID(licenseSysID) {
    return JSUtil.nil(licenseSysID);

}

Side by Side comparison of both codes.

Smelly "Long" Code Less Smelly Shorter Code

var GlideUserSubscriptionAjax = {

    syncALicense: function () {

        var retJSON = {};
        var licenseSysID = this.getParameter('sysparm_license_sysID');

        if (JSUtil.nil(licenseSysID)) {
            retJSON.syncStatus = 'failure';
            return new JSON().encode(retJSON);
        }

        var licenseGR = new GlideRecord('license_details');
        licenseGR.get(licenseSysID);

        var syncHelper = new UserSetSubscriptionSyncHelper();
        var gr = syncHelper._getUserSetsForLicense(licenseSysID);

        if (gr != null && gr.getRowCount() == 0) {
            retJSON.syncStatus = "nosync";
            return new JSON().encode(retJSON);
        }

        retJSON = syncHelper.syncLicense(licenseGR);
        retJSON.syncStatus = 'success';

        return new JSON().encode(retJSON);
    }
};

function syncALicense() {
    var licenseSysID = this.getParameter('sysparm_license_sysID');

    //In OO Programming stop condtions are ecouraged.
    if (missingLicenseSysID(licenseSysID)) {
        return encodeStatus(SyncStatus.failure);

    }

    var licenseDetails = getLicenseDetails(licenseSysId);
    var syncSubstription = tryToSyncUserSubscription(licenseDetails, licenseSysId);

    return encodePayload(syncSubstription);

}

OR

function syncALicense() {
    var licenseSysID = this.getParameter('sysparm_license_sysID');

    var syncPayload = missingLicenseSysID(licenseSysID) ?
                SyncStatus.failure :
                tryToSyncUserSubscription(getLicenseDetails(licenseSysId), licenseSysId);

    return encodePayload(syncPayload);
}

Unlike comments, long methods don’t need to exist. They are inconsiderate of future readers, forcing coders to reason about each segment and it as a whole. The less a reader has to reason about code the better; a reader should be reading without having to compute what’s happening. Long methods do just that, they require the reader to think much more than necessary. When you see long methods break them down.

The idea is to keep implementation details at the bottom abstraction layer and show the more readable code at the higher layers. Or as some call it, closest to the user.

Form this point other principles can be applied to further remove the smell from the refactored code. The beauty of small functions is that one can fully concentrate on the few lines, making it a load lot easier to debug and find bugs, and the names make them much more meaningful when reading.