Code Architecture - Refactoring a Script as a Walk-Through to the Beginnings of Code Libraries

This article is about creating a cohesive coding experience through code structure. It intends to organize code in such manner as to increase scalability while decrease maintenance. I will walk through refactoring a piece of working code by going beyond mimicking its behavior and extracting its working parts. The parts will be the begin of a foundation and practices which will be used for future functionality. My main concern won’t be in as much as how to solve the problem nor what exactly happens within each independent artifact rather, how it will be separated and structured.

 The sample code comes via sndevs.slack.comm channel #codereview by user @vk, where input for reworking an Inbound Rule was requested. A warm thanks to @vk for his generosity, allowing me to use his script. Cheers, mate!

 The Original (partial) Script as presented on sndevs.slack.com (Inbound Rule)

var InsQues = new GlideRecord('asmt_assessment_instance_question');
InsQues.addQuery('instance.number=' + AssInsNum);
InsQues.addQuery('metric', 'b4468b89dbce2f407391ab8b4b961935'); //New Image Scale

InsQues.query();

if (InsQues.next()) {
    InsQues.value = SurRespValue;
    InsQues.update();

}

var InsQuesComm = new GlideRecord('asmt_assessment_instance_question');
InsQuesComm.addQuery('instance.number=' + AssInsNum);
InsQuesComm.addQuery('metric', 'b22a5f46db2b63007391ab8b4b9619ce'); //Comments
InsQuesComm.query();

if (InsQuesComm.next()) {
    InsQuesComm.string_value = body_comments;
    InsQuesComm.update();

}

The Original Problem

How can two records from the same table update different fields without having to query multiple times? One for each record with different fields to set. The solution, perhaps not apparent at first, is relatively straight forward: use a data dictionary to describe the desire result, feed it to a function using an IN condition then, leverage the key/value pairs in the data dictionary to set the value in the correct GlideRecord. I’ll return to this later on. For now, that was the jest of the request.

Identifying refactoring possibilities

1. Hard-coded IDs. (What happens if there is a need for additional ids, or ids change, or are retired?)
InsQues.addQuery('metric','b4468b89dbce2f407391ab8b4b961935')

2. Duplication of code (GlideRecord)
Two GlideRecords to do the same thing: query to set a field value.

var InsQuesComm = new GlideRecord('asmt_assessment_instance_question'); InsQuesComm.addQuery('metric','b22a5f46db2b63007391ab8b4b9619ce'); //Comments

InsQues.addQuery('instance.number='+AssInsNum); InsQues.addQuery('metric','b4468b89dbce2f407391ab8b4b961935'); //New Image Scale
 
3. Isolation of the script into an Inbound Rule

By directly adding script into an Inbound Rule, duplication becomes a necessity. Whenever a process outside an inbound rule needs to leverage the very same code, it must be re-copied at best; at worst re-built. Hopefully, the author is the same person making the new addition and knows to place the script into a function to leverage, otherwise it will be duplicated; and if it’s a new developer? New process. If this practice is a system-wide perverseness... Supporting an entire code-base where there is uncertainty of behavior grows increasingly challenging as more processes are built.
 
4. Inconsistent variable naming convention

Thou captious, having a consistent naming convention makes code easier to read. The faster this can happen, the better off. Various strategies exist to facilitate readability.

Here are some that I follow.

• camelCase for variables, parameters, arguments, functions. (Research shows that programmers and non read camelCase nearly 25% faster than other cases. The reasoning is the evolution of reading has allowed the mind to interpret capital letter between words with less difficulty. Those without any programming experience read camelCase even faster than those used in other cases. I use it simply because I find it prettier.
• Capitalization for Objects (Car, Plane, Auditor)
• Plurals for arrays (cars, planes, auditors)
• Single quotes for string literals
• CAPS for CONSTANTS

Convention is on the interest of reducing interpretation times (readability). Whatever you pick, stick with it.

While speaking of conventions, I’ll touch upon function arity. It is vastly easier to debug, unit test, maintain, create functionality for a function that takes one and only one parameter than it is when arity increases. If for whatever reason the function can’t be unary, make the parameter an object. In due time the reason will glare back at you happy that you did. Along the same lines, all functions should return some thing. No undefined of what-so-ever. If there is nothing to return, return true.

function anUnaryFunction(sysId) {
  /// do something

  return something;

}

function anotherUnaryFunction(params) {

  var sysId = params.sysId;
  var name = params.name;

  //do something

  return something;

}

 So the very first steps mentioned for creating readable and structure code are:

1. Naming conventions (sound, intuitive names might not arise until after the code is complete)
2. Unary functions

Those two alone are a fine start when starting out to structure.

The Refactoring Strategy: look at code in logical groupings from which to extract a structure.

Right from the beginning the rigidity of the original code comes to light. The tight coupling, and inflexibility introduced by hard-coded ids should be avoided at all costs. The technical dept it causes far outweighs any convenience benefits. Convenience is not an excuse for ineffective coding practices. The question then is: what is a more appropriate place for such hard-coded ids or values?

A solution is some form of Data Dictionary. An object that is a series of key/value pairs identifying properties to be used by a given process. In JavaScript this means Object Literals and their structure is totally dependent on you, though a flat object tends to be okay for most cases.

Extracting hard-coded values into Data Dictionaries

Firstly, identify functionality to come up with intuitive names. (I don’t know if those I’ve selected are as I don’t know the full business process being addressed. So I based them on what I saw in the code). This means to use the table being queried as a basis for names (asmt_assessment_instance_question.)

 From the GlideRecord queries, all hard-coded values are extracted into a script include.

1. An asmt_assessment_instance_question Table Data Dictionary (Script Include (this example), Table, or sys_properties) GlideRecord options extracted from original code

var AssessmentInstanceQuestionOptions = (function buildAssessmentInstanceQuestionOptions() {
    var data = {
        TABLE: 'asmt_assessment_instance_question',

        encodedQueries: {
            imageAndNumberByInstanceNumber: 'instance.number = $ {number}^metricIN${metricSysIds}'

        }
    }

    return data;
})();

Why place a GlideRecord data structure inside a Data Dictionary?

• Atomic testing of the queries. (as in, put an AFT in front of all Table Data Dictionaries to ensure queries work as expected
• Decouple data from code (separation of concerns)
• Easier to identify issues with query
• One GlideRecord function can now query all GlideRecord Data Dictionaries (80/20 solution)
• Avoids code duplication by necessity.
• Uncluttered entry point
• reusability

Why is it an IIFE (Singleton)?

Normally Data Dictionaries should be singletons. They should also be placed in either a sys_properties file or a table to do away with dev cycles whenever possible. I’ve used a Script Include for demonstration purposes. Notice that the values are still hard-coded, regardless of where they are hosted, they have to be hard-coded somewhere; however, they have now gained flexibility by being decoupled from code and located in it’s own container. The sole purpose of this container is to create a Data Structure to host GlideRecord queries that support the AssessmentInstanceQuestions process AND limited to the asmt_assessment_instance_question table.

The IIFE introduces scalability. Meaning that if the location of the Data Structure currently hosted inside the IIFE is moved to a table or a sys_properties, the functionality to rebuild it happens inside the IIFE. To the rest of the code-base, it’s as if nothing every happened. As long as you ensure to return the existing format in addition to new items, there will never be downstream breaks. This means that there is only one place in entire code base to look for hard coded values: properties files.

The properties file or data dictionary can not be called just by anyone, it can only be called by factory functions. These files can not bleed to various processes, no one outside the process that own it can work with them. If they are needed somewhere, the use one and only one entry point which will return it, such as a factory function.

Further refactoring of a data dictionary.

Break down queries by process (Off-boarding, on-boarding, Global for the table such as all active, all closed, etc) so that a table, say Incident queries are broken down into chucks.

Put the data structure in a sys_properties or table and use a factory function to build it. Don’t let any other process outside of the factory function build the data structure. There must exist one and only one place in the entire code-base which builds the structure.

Strongly consider flat objects as Data Dictionaries

If you know the data dictionary will not be a stable one because of volatile business requirements causing the structure to change drastically, consider returning an Object such as:

var makeTableOptions = function makeTableOptions (dataDictionary) {

    return {
        getSomeProperty: function getSomeProperty() {
            return dataDictionary.someProperty;

        },

        getSomeOtherProperty: function getSomeOtherProperty() {
            return data.someOtherProperty;
            
        }
    }
}

2. A Data Dictionary to leverage for setting values within the GlideRecord (Script Include (this example), Table, or sys_properties) Hard-coded ids extracted from original code

var AssessmentInstanceQuestionMetricStructure = {

  "b4468b89dbce2f407391ab8b4b961935": {  //id of item 
    "fieldName": "value"  //field to set in item

  },

  "b22a5f46db2b63007391ab8b4b9619ce": { //id of item
   "fieldName": "string_value" //field to set in item

  }

};

Why remove embedded strings such as sys_ids and field names

• It’s simply a bad practice regardless of upfront “benefit”.
• Separation of concerns
• When ids are retired, requires code updates
• One stop shopping for everything process specific for AssessmentInstanceQuestionMetricStructure

 Further refactoring of a data dictionary

• Use JSON in a sys_property
• Use table to store a JSON string, or fields to host each value in the key/value pair, AND using a factory function to build an Object Literal from the table query

 3. Replacing GlideRecord code duplication with a utility Object (ScriptInclude) GlideRecord extracted from original code

var DB = {

    queryTable: function queryTable(options) {
        var records = new GlideRecord(options.table);
        records.addEncodedQuery(options.encodedQuery);

        records.query();

        return records;
    }

}

Way replace GlideRecord with a utility function

• Avoids code duplication. No more new GlideRecord(tableName) clutering everywhere
• separates concerns into DB functions and Business Processes
• covers the 80/20 case
• Unit test all Table Data Dictionaries with one function
• reduced code-base size
• reusabilty
• no more isolating GlideRecords in business rules, action scripts, inbound rules, or using in multiple script includes that do precisely the same thing
• if it works once, it will always work
• if there is ever the introduction of SuperDupperGlideRecordVersion2, changing your entire code-base to work it it happens in one place
• working with encodedQuery is far easier

Note: I’ve often been told that I shouldn’t use encodedQueries and instead use addQuery because I’ll need to sanitize parameter data all over the place. To me this doesn’t speak of a code problem but a design issue. Data sanitation should happen in one and only one place. If it’s happening in multiple places, design should be revisited.

Also notice that the GlideRecord is infant. Meaning that there are a slew of functionality to a GlideRecord that the utility class just doesn’t support. This is actually not a problem, rather an indication of the maturity of the our utility classes. As more queries are introduced, a pattern starts emerging and then and only then, is it time time start expanding functionality. Revisit the glide utility functions to handle such cases.

When structuring code, and new to a process, always collect loads of data before beginning to build code. There is just no way of knowing how something is going to change in the future.

4. Utility functions to set values in a record (ScriptInclude)

var SetValueUtil = {
    setInCurrent: function setInCurrent(options) {

        var record = options.record;
        var payload = options.payload;

        for (var field in payload) {
            if (record.isValidField(field)) {
                record[field] = payload[field]
            }
        }

        return true;
    }
};

var replaceValues = function replaceValues(encodedQuery) {

    return function applyTranspose(values) {
        for (var index = 0, length = values.length; index < length; index++) {
            encodedQuery = encodedQuery.replace('${' + values[index] + '}');

        }

        return encodedQuery;
    }

}

Why a utility class to set values (setInCurrent Not used in this example)

This is part of building a tool kit. Rather than contentiously having to GlideRecord.fieldName = or GlideRecord.setValue, or replace a value in a string, leverage a utility function. This allows use of a Data Dictionary to set values in a record rather than having to do it over and over. Or duplicating the very same RegEx in various places.

TransposeValuesToQueryUsingArray is a brute utility function to replace values. In practice there will be a ReplaceStrategy Object along with a RegExpression Object. The replace Strategy is a tool kit of functions that leverage RegExpression for string replacements.

Utility functions are a great friend, they become functions used throughout the entire code-base to avoid the need to duplicate functionality. How many RegEx to remove the very same patter are there in SNow? No utility class leads to added work.

5. Utility function as an entry point to bring together the business process indicated in the Inbound Rule. (Inbound Rule Body Become this in a script include)

var updateAssessmentByMetric = function updateAssessmentByMetric(params) {

//curried function
    var toApplyValuesQueryFn = replaceValues(AssessmentInstanceQuestionOptions.EncodedQuery.encodedQuery); 

//apply values to raw query
    var query = toApplyValuesQuery([params.number, Object.keys(AssessmentInstanceQuestionMetricStructure)]); 

    var assessments = DB.queryTable({
        table: AssessmentInstanceQuestionTable.TABLE,
        encodedQuery: query

    })();

    var matric;
    //this can be abstracted away as well to become: updateRecords( { records: assessments, values: assingValues(AssessmentInstanceQuestionMetricStructure)(params)}); no need to have the while here
    while (assessments.next()) {
        matric = AssessmentInstanceQuestionMetricStructure[assessments.getValue('metric')];

        if (matric) {
            assessments[matric.fieldName] = params[matric.fieldName];
            assessments.update();

        }
    }

    return true;
};

Why a utility function rather than placing it inside the Inbound Rule

• Avoid code duplication by necessity (chances are that if a sys_id is being used here, the record is important enough where related process will use it. Move the code into a place where it can be reused
• group all the functions (function composition) that are required to carry out the process from start to finish. (complex behavior to help the entry point)
• Leverage functionality from other libraries
• This is the part that says what am I doing

Notice that the initial problem of updating two records without re-querying was solved without directly looking to solve it. It solved itself by introducing structure to the code. With a fully mature library, the code above would have been 2 lines: db query, and update record utility function calls.

Now that the problem is solved, requirements for additional sysIds with own fields don't require functional changes other than adding an entry to the Data Object. The utility entry point then picks it up and we are good to go. This means no downstream changes of any kind (counters popular opinion that JavaScript requires downstream changes all the time)

What does the code do?

1. It uses the values passed into the function to build the encodedQuery
2. It queries the table to get records
3. it loops through them to set values

• (note here that this functionality can also be abstracted away. I do just that in my production code. Fill a data object with values, pass it to a utility function that fetches the db and sets fields). that’s beyond my efforts for this article.

6. Entry point in the Inbound Rule (Code inside the Inbound Rule is now)

var result = updateAssessmentByMetric({
    number: current.number,
    value: current.image,
    string_value: current.comment

});

Why limit entry points to one liners.

• Process entry points should all be one line as to make code uniform across the code base.
• Eases maintenance removing clutter from the entry point
• duplicating the process else requires just one call
• Aside the function call, no code duplication by necessity
• unit testing the entire process as a whole, or in pieces is possible
• at-a-glance reading informs what business process is being executed

Final Description:

Building a tool set greatly simplifies a code base, maximizing reuse of components to the point where future coding requires very little new code. This means less code surface in which to hide bugs, less time reading code, and less cost associated.

To simplify maintenance, dividing code into neat logical compartments helps a great deal. This means that code as you know it know changes to telling code what to do, rather than telling it how to do. The tools offered by ServiceNow are robust enough where the limit of the organization of your code is us as programmers; our skills. Luckly, JavaScript has some patterns to help us do that.

Though the initial problem was solved, the goal wasn't to solve it but to break refactoring into a structure to be leverage for future code. It was solved by design without having to directly tackle the issue. The use of function composition solved the issue, while simultaneously creating the beginnings of a tool library for future use.

Also note that the discrepancy of code lines now when refactoring the code is not from added noise rather, the beginning of building a foundation which to leverage by other functionality.

TL;DR

1. Data Dictionaries to describe the processes
2. Factory functions to build the data dictionaries
3. Generic utility functions to support processes (such as CRUD operations)
4. single entry point for all your code-base
5. Remove GlideRecord from the code-base
6. Don’t allow various processes from accessing unrelated Data Dictionaries
7. Move all of your code into Script Includes
8. Come up with names that indicate what is happening in the body of the function
9. Get away from the defunct Class creation pattern of Script Includes
10. Functions should be unary
11. Functions must always return a value
12. Bundle complex process into a series of functions to use in the utility class that will represent the entry point of the process (or make it a factory returning an .execute() to kickoff the process)

My apologies for the lengthy article. Hope that gives ides for starting to build function libraries.