Code Smells, Refactoring Long Script Includes– Bloaters: Day 5

Javier Arroyo · ‎07-24-2020

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

Refactoring practices 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 may differ from those used by classically trained coders.

Previous series articles

Day 5 - Refactoring Long Script Includes

A long script include is not necessarily one with many lines (though this can be a sign of such), rather one that is saturated with functionality and various contexts. One that does so many things it becomes a God object.

Examples of long a class:

Handles mathematical functions, handles CRUD, handles POST
reader must scroll through screen upon screen of code
note easy to refactor
context not obvious at-a-glance

Solution

The approach is the same as in others. Group functionality by context then extract it into various script includes. isolating the units of work then, replace caller.

(In more robust terms, the SI should have been derived from a Script Include with a public API, rather than having other code directly access it). It's called programming to an interface. Which refactoring to that level is beyond this post. It's meat for scalability, and to close classes to updates once built.

Why Long Script Includes smell

will grow out of control
increased reasoning difficulty
name doesn't reflect contents
leads to code duplication
becomes harder to refactor
will touch so many parts of the system it becomes fragile
unscalablilty

Example of a "long script include"

var CitibankRESTMessage = function CitibankRESTMessage() {

  return {
    multiply: multiply,
    post: post,
    save: save
  };

  function multiply(firstFactor, secondFactor) {
    return firstFactor * secondFactor;

  }

  function post(current) {

    try {

      var fieldNames = 'description,short_description,number'.split(',');
      var payload = getValuesFromFields(fieldNames, current);


      var request = new sn_ws.RESTMessageV2();

      request.setHttpMethod('post');
      request.setEndpoint('an endpoint');
      request.setAuthenticationProfile('basic', '2df2dfabbdb4b08908870894d0b961994');
      request.setRequestBody(JSON.stringify(payload));

      response = request.execute();
      httpResponseStatus = response.getStatusCode();

      //...

    } catch (ex) {
      var message = ex.getMessage();
      //...
    }

  }

  function getValuesFromFields(fieldNames, gr) {
    return fieldNames.reduce(getFieldValue(gr), {});

  }

  function getFieldValue(gr) {
    return function getValue(collector, fieldName) {
      collector[fieldName] = gr.getValue(fieldName) || '';

      return collector;

    };

  }

  function save(fieldValuePair) {

    var gr = new GlideRecord('sys_user');
    gr.initialize();

    for (var field in fieldValuePair) {
      gr.setValue(fieldm, fieldValuePair[field]);

    }


    return gr.insert();
  }

};

While kept short for demo reasons, the script include contains quite a few items that can be refactored to decrease it's size and clutter. Not just to make it shorter, but to introduce context and scalability.

The clutters are:

handles saving a users
posting messages
mathematical equations
unnecessary boilerplate code: tryCatch, RestMessageV2 code
SI name doesn't account for what might be inside
uses functionality that will be duplicated

Example of Solution

Break the code into multiple SIs, and take the time to refactor long methods as well. The approach will lead to

The beginnings of a CRUD file
a universal try/catch
a universal RestMessageV2 factory
A post specific SI
A math SI
and more

The Try/Catch SI

var tryCatch = function tryCatch(failure, success) {

    return function execute(value) {
      try {
        success(value);

      } catch (error) {
        failure(error);

      }
    };

  };

the tryCatch SI can now replace having to implicitly try {} catch {} every function. This helps in spotlighting the most important part of the code: what the caller of the function is doing.

The Calculator SI

var Calculator = function Calculator () {

  return {
    multiply: multiply
  };

  function multiply(firstFactor, secondFactor) {
    return firstFactor * secondFactor;

  }

};

The calculator function is limited to math functions. Assuring that the reader only reasons and sees math related items.

The beginnings of CRUD

var CRUD = function CRUD(tableName) {

  return {
    save: save, getFieldValues: getFieldValues
  };

  function save(fieldValuePair) {

    var gr = new GlideRecord(tableName);
    gr.initialize();

    for (var field in fieldValuePair) {
      gr.setValue(fieldm, fieldValuePair[field]);

    }


    return gr.insert();
  }

  function getFieldValues(postConfig, gr) {

    var fieldNames = PostConfig.FIELDS_TO_SEND;
    return fieldNames.reduce(getFieldValue(gr), {});

  }

  /****************************Private members ****************************/

  function getFieldValue(gr) {
    return function getValue(collector, fieldName) {
      collector[fieldName] = gr.getValue(fieldName) || '';

      return collector;

    };
  }

};

The CRUD SI is a front end to GlideRecord just to get away from having to build boilerplate, repetitive code each time dealing with GlideRecords.

The RestMessageV2 factory

var makeRestMessage = function makeRestMessage(messageConfiguration) {
  var request = new sn_ws.RESTMessageV2();

  for (var attributeName in messageConfiguration) {
    if (typeof request[attributeName] === 'function') {
      request[attributeName].apply(request, messageConfiguration[attributeName]);
    }

  }

  return request;
};

The factory is a basic function to build and apply parameters the function of RESTMessageV2. This can be enhanced quite drastically, even used to call any function provided OOTB (such as GlideRecord). For our interest, this ought to be expanded with a lookup to the REST Message table to build the message, limiting the Post Configuration to what field values to pull and what functions to execute against them.

The refactored CitbankRESTMessage

var CitibankRESTMessage = function CitibankRESTMessage() {
  var crud = CRUD();

  return {
    post: post
  };


  function post(current) {

    var PostConfig = JSON.parse(gs.getProperty('name.to.sys.proerties'));

    return tryCatch(createAndPostMessage(current), handleError)(PostConfig);

  }

  /***************Private members ********************/

  function createAndPostMessage(current) {

    return function doCreateAndPost(postConfig) {

      var values = crud.getFieldValues(postConfig, current);
      var postSignature = buildPayload(postConfig.signature, values);
      var request = makeRestMessage(postSignature);
      var response = request.execute();

      return handlePostMessageResponse(response);

    };
  }


  function handlePostMessageResponse(response) {
    response = request.execute();
    httpResponseStatus = response.getStatusCode();

    //...
  }

  function handleError(error) {
    //handle it
  }


  function buildPayload(postConfig, fieldValues) {
    postConfig.setRequestBody = JSON.stringify(fieldValues);
    return postConfig;

  }

};

The goal of refactoring Citibank wasn't just to extract classes, but to extract long methods into smaller ones. This concentrates the scope of the class to what is unique to it. The next version would further refactor the SI logically by removing Error handlers functions, introducing an Interface or factory.

Remember not to worry as much about number of lines, rather worry about logical divisions. The more isolated the context, the more condensed the final version will be. This helps in that, although there will be more Script Includes, those script includes will be atomic unite of work that can then be bundled into a larger object as to reduce downstream changes. and, easier to reason where to find core functionality

Example:

function makeCitibank () {
  return Object.assign({}, CRUD, Calculator, CitibankRESTMessage);

}


var Citibank = makeCitibank();
Citibank.multiply(1, 2);
Citibank.post(current);
Citibank.save(current);