Cleaning code to decrease interpretation times

[Javier Arroyo]

This post refactors a piece of code written in the style commonly found in ServiceNow to demonstrate how clean code can lead to decreased interpretation times.  It will first show the original code sample, then what it can look like refactored with clean code ideas.

Original Sample

(function process ( /*RESTAPIRequest*/ request, /*RESTAPIResponse*/ response) {

    // implement resource here
    var reqNumber = request.pathParams.number;
    var requestBody = request.body;
    var requestData = requestBody.data;
    var state = requestData.state;
    var errorMessage = requestData.error;


    var body = {};
    var grSCReq = new GlideRecordSecure('sc_req_item');
    grSCReq.addQuery('number', reqNumber);
    grSCReq.query();


    if (grSCReq.next()) {
        grSCReq.state = state;

        if (null != errorMessage && errorMessage.length > 0) {
            grSCReq.comments = errorMessage;
            grSCReq.variables.error_message = errorMessage;
        } else {
            grSCReq.comments = Comments.getPrefixByState(grSCReq) +
                '\n' + Comments.format(requestData.comments);
        }
        grSCReq.update();
        response.setBody(body);
    } else {
        response.setBody(body);
    }

})(request, response);

The code above is relatively simple yet, how long did it take you to interpret what was going on? How sure were you that what was clearly understood? How long will it take the next person to understand the very same lines? How long will it take you to read the very same lines months down the line? Simple code doesn't necessarily mean easily and clearly understood code. Time in business is translated to cost. The longer it takes to read code, the more costly it becomes. 

Refactoring #1

(function process ( /*RESTAPIRequest*/ request, /*RESTAPIResponse*/ response) {

    var ritm = getRequestedItem(request);
    
    hasRecord(ritm) && addResponseCommentsOrError(request, ritm);


    response.setBody({});

})(request, response);

Refactoring #2

(function process ( /*RESTAPIRequest*/ request, /*RESTAPIResponse*/ response) {
    var ritm = getRequestedItem(request);

    if (foundRecord(ritm)) {
        addResponseCommentsOrError(request, ritm);

    }

    response.setBody({});

})(request, response);

 Refactoring #3 

(function process ( /*RESTAPIRequest*/ request, /*RESTAPIResponse*/ response) {
    
    HttpScReqItem.addResponseCommentsOrErrors(request);

    response.setBody({});

})(request, response);

The refactoring above moves low level code (algorithms that carry out work) into named functions that give the work being carried out human meaning. The functions from refactoring #1 and #2 above, although not method calls or displayed above, belong in a library. The functions in that lib can then be reused, and themselves refactored to become single purpose sharable functions. The need to rewrite boilerplate code as seen in the original snippet will eventually be minimal.

The functions made from refactoring #1 and #2 could be as follows.

function hasRecord (gr) {
    return gr.next();

}


function getRequestedItem (request) {
    var gr = new GlideRecordSecure('sc_req_item');
    gr.addQuery('number', request.pathParams.number);
    gr.query();

    return gr;

}


function addResponseCommentsOrError (request, ritm) {
    var data = request.body.data;

    return hasErrorResponse(data) ?
        showErrorInRequestedItem(data, ritm) :
        addCommentToRequestedItem(data, ritm);

}


function showErrorInRequestedItem (data, gr) {
    gr.comments = data.error;
    gr.variables.error_message = data.error;

    gr.update();

    return gr;
}


function addCommentToRequestedItem (comment, gr) {
    var comments = Comments.getPrefixByState(gr) +
        '\n' + Comments.format(comment);

    gr.comments = comments;
    gs.update();

    return gr;
}


function hasError (requestData) {
    return requestData.message != null && requesteData.message.length > 0;

}

The goal was to break down the original code into independent units of work. Each in charge of a single thought, and named accordingly.

When a reader comes into an initial line of code, it should be clean, nothing more than the initial thought that starts the process. If there is a need to see more, then follow that thought to the next function, and so on until the search that brought her there is satisfied. Once the interest is fulfilled, the search is abandoned. And key, is that each function should be a single unit for work trivially understood; and not until reaching the lowest possible level, those really technical details found. Pretty much like a search engine result where the reader goes into the results of interest...

happy snowing....

[DrewW]

Or you could just add a few comments.  No one seams to think comments are of any use anymore.

[Javier Arroyo]

Maybe because the reader is now tasked with reading comments, verifing the code is correct and matches the comments, keeping them current, sifting through the extra clutter, etc. A key giveaway that the author has failed to communicate is comments when not needed. Comments in code as the original version are know as a code smell. The code was abused by ignoring basic principles and to fix it, more clutter is added. The argument supporting misuse of comments has been addressed since procedural paradigms were replaced by OO. Though it is understandable how one wod consider masking smelly code with comments as a proper solution.

[-O-]

I just have on problem with the solution: you left in some comments:

/*RESTAPIRequest*/

and

/*RESTAPIResponse*/

:-)))

Other than that, a good read as always.

[DrewW]

Comments are code smell to you.  You have ignored the fact that not everyone reads code like you and that the person reading totally understood at a simple glance and that adding all the functions just added confusion because you need to verify/understand that the actual name matches what the function is doing.  In version X someone could have updated one of the functions and its name is not really accurate anymore but changing the name could have broken things.

Your article is about decreased interpretation times and I have found that is a very personal thing and what is confusing to you is not confusing to someone else.  One example is you used a ternary operator, I worked with someone who finds them very confusing to understand and feels that you should always use if/then statements because it is more clear and easier to understand at a glance so to them a ternary operator is code smell.

[Javier Arroyo]

Question 1:
The new hire process is failing to log an already in-flight hire. Audit has raised an issue that an in-flight new hire must be logged accordingly. Your manager hands you the code below and says, you must prove to them we know where the problem could be. He puts you on a stop watch, and says "Save me from audit". How long did it take to tell him that you have a good hunch of where the problem problem is? To raise a problem and we are good with audit. 

function processNewHire (payload) {
  isNewHireNetNew(payload)
     ? orderNewHireCatalogItem(payload)
     : logAlreadyInflightNewHire(payload)
  

}

Question 2:

Offboarding an employee does not notify the manager. Can you quickly figure out where it could possibly be that the offboard employee is broken that a manager is not being notified?. Everything else is working, just not the manager notification. Where, in the process below, is the issue?

function offboardEmployee (employeeNumnber) {
    var pipeline = [getEmployee, disableEmployee, notifyManager, broadcastOffboardingToOutsideSystems, closeOpenRecords, notifyHRProcessInFlight];

    pipie(pipeline)(employeeNumnber);

}

Question 3:

A new SailPoint request is taking too long and times out.  Reading the process whitepaper says that SailPoint requests are synchronous. Informed with that, Can you find where it could possibly be that the sync execution of SailPoint is found and where to begin debugging?

function newSailPointManageRequest (sc_req_item) {

    SailPoint()
        .getManageRequestEndpoint()
        .interpolateEndpointWithNumber()
        .addPayload(sc_req_item)
        .executeSync()
        .respondToPost();
}

No matter how much procedural programming style is defended, nothing about it is readable.  This is partly why it hasn't been a thing in 50 years.

I'm not certain that I'm convinced that familiarity makes 30 lines of code easier to read than 4 human grammar lines. That adding comments further clarifies obscure code. Puts me in a situation where if I stare at chaos long enough, it's just as easy to find an item that is properly ordered. 


In sample 2, even pipes were used to decrease familiarity. Yet, how long did it really take you to solve the problem? Were comments needed to say "hey, I notify a manager; and I disable an employee; and I broadcast off boarding to third party systems.  Or, is your concern misplaced by presuming all the functions can be a misnomer? That someone is going to change the name of my functions, but somehow not a character among 500?  If that's the concern, there are far bigger problems to address than being precise. 

There are quantitative reasons the original code is said to smell. Familiarity biases is not a quantitative characteristic, making reading a ternary operation a greater indictment on the reader's competency than how it can help articulate a piece of code succinctly. A cool fact is that familiarity is the number one reason something is easier to digest. What does that say about ternary vs if/else? This means that if familiarity is removed from the equation, there must be some sort of viable alternative for what it means for something to require low cognitive loads.

This is why it is difficult for me to accept that SNow coders often use anecdotes to support disuse of ternary yet, ask a newbie to read 700 lines of spaghettis code, soaked with dozens of comments, tightly bound, mutating objects, then not only understand it but to change it without impact. So if we really believe in familiarity, then why assert that a ternary should never be used? The logic doesn't seem to point at the right cause.

Some of reason the original code smells
1. Forces duplication by necessity
2. Logic can not be leveraged by anyone else
3. one function handles multiple concerns
4. each piece cannot be tested individually
5. Is order bound
6. is tightly coupled
7. Request.Data is a grouped object that creates a logical binding that shouldn't have been broken into pieces
8. Unnecessary temporary variables
8. There is more surface code in which to hide bugs
9. to understand the code, it must be looked at as a whole. It prevent total focus in each individual unit of work that only depend on its inputs.  This means that to know what it does, one must understand the whole, always keeping in mind how each piece affects the other, keeping in  mind that any one mess at any time, will break the whole.
10. It doesn't scale
11. Overtime it will unnecessarily increase code surface area 

By breaking code into pieces. All of those points are gone. A reader can then concentrate solely on 3 lines of code and be able to quickly determine that if a RITM is there, it gets updated or an error added. All from reading 3 lines. If you feel that spending 6 minutes reading 30 lines of code is justifiable by familiarity then you are better than I am. I honestly didn't even want to read it.

Individual functions can be used independently. This means the beginning of a library that one at a maturity level, can be dipped into without having to rewrite anything again, and again because familiarity dictates it.


Your concerns seem to ignore that a 2 lines of human grammar will always be easier to digest than 20 computer directed lines of code because a reader can be more familiar with the 20 lines.

Here is my anecdote. I worked with a Japanese company that their developers didn't know English. They would hire English speakers to ensure their functions names matched content. Java code, not more than 4 lines long each function. Much like the spring framework. A lack of knowledge and process failure shouldnt cast aside proven industry standards.