Code Architecture: Decreasing bugs by decreasing code surface

Javier Arroyo
Mega Guru

on ‎04-04-2020 01:32 PM

This post arose from seeing the same type of code question, same type of bug over and over and feeling that the acceptable solutions don't really address the problem: Code structure is causing us headaches by having too many lines in which to hide bugs.

Sample use case: Query a table and grab X number of fields values from it. 

Example 1: Familiar ServiceNow Code

try {

    var choices = new GlideRecord('sys_choice');
    choices.addQuery('element','u_function');
    choices.addQuery('name','incident');
    choices.addQuery('inactive',false);

    choices.query();

    var data = [];
    while (choices.next()) {
        data.push({
            'label': choices.getValue('label'),
            'dependent_value': choices.getValue('dependent_value'),
            'name': choices.getVaue('name')
        });

    }

} catch (error) {
    //do something
}

In the sndev slack channel code as the one above is the norm. It goes through reviews, best practices, and any issues will undoubtedly get an answer. Slack channel is certainly the very best place to ask code questions.  go there if you are not already a member.

The problem that is not immediately visible is that its structure inherently leads to too many logical units crammed into one space, creating more lines than needed, thereby increasing the chance for bugs to hide. The basic principle is that the more involved a while(gr.next()) loop grows, the more space there is for bugs. 

The solution is to decrease code surface by introducing functions to the repetitive nature of ServiceNow APIs such as breaking the code in the following form:

  1. Create the glide record
  2. query the glide record
  3. loop
  4. functions to execute within the loop
  5. a try catch

Example 2: Refactored Example 1 (Outside functions names and hardcoded parameters, this point-free style is in our Production Instance)

function dependentChoices(element) {
   return DBTable('sys_choice')
      .queryWhileCompose([valuesForFields(['label', 'dependent_value','name'])])
      ('element='+ element +'^name=incident^inactive=false');

}

Example 2 has decrease code space, increased scalability through function queryWhileCompose that accepts any number of functions to execute against each glide record. Made new GlideRecord and while isolated to DBTable: they won't be found anywhere else in code. And allow for trivial updates of functionality.

Though I tend to prefer point-free coding as shown in example 1, that LISP-like style can be a mouthful to swallow. In that case the more familiar temporary variables helps while keeping the code concise.

Example 3: Using temporary variables to break down the process

var fieldNames = ['label', 'dependent_value'];
var getValues = valuesForFields(fieldNames);
var query = 'element=u_function^name=incident^inactive=false';

var choices = DBTable('sys_choice')
    .queryWhileCompose([getValues])(query);

Example 3: Adding more functionality 

var fieldNames = ['label', 'dependent_value'];
var getValues = valuesForFields(fieldNames);
var alertBusinessOwner = alertBusinessOwnerOfNewActiveChoice(eventName);

var query = 'element=u_function^name=incident^inactive=false';

var getDependentChoices = [getValues, activateIfDeactive, alertBusinessOwner, logMetaDataChange];

var choices = DBTable('sys_choice')
    .queryWhileCompose(getDependentChoices)(query);

In the example above, there have been 3 additional functions added: activateIfDeactive, alertBusinessOwner, logMetaDataChange. Each handling single responsibility. If an error is introduce it must be one of those functions. this makes the function easier to digest. It readily informs the reader what is happening, expelling all ambiguity while being drastically less code in which to hide bugs. 

Summary

Bugs love large surfaces, the more code available the greater the chances are for bugs. If you find yourself geographically navigating code, somethings is wrong. It is extremely difficult to debug a large surface of code.

  • 835 Views
Comments
Mwatkins
ServiceNow Employee
ServiceNow Employee
‎08-18-2020 10:26 AM

Hello Javier,

Very interesting post! I like it! 

If you will allow me to play devil's advocate here, the following are general concerns that I have about any layer of abstraction and specifically about one that sits in front of GlideRecord:

  1. While creating less code surface, API's introduce complexity in the internal logic
    1. Protection against large arrays, deep loops
    2. Protection against bad inputs, invalid data types, null/undefined values
    3. Support for other methods of GlideRecord (setLimit(), orderBy(), addOrCondition(), addJoinQuery()...)
    4. Error handling and surfacing failures to developers
    5. Code should conditionally apply all normal coding best practice that people are normally expected to follow. See a list of scripting best practices here: Performance Best Practices for Server-side Coding in ServiceNow
  2. The creation of more complexity inside the API introduces performance overhead
    1. Since we don't know when we need to protect against large arrays we must always protect
    2. Since we don't know what inputs are invalid we must make some heuristic
    3. The new API will need to protect against invalid usage - e.g. have they called the right methods in the right order?
    4. What if you are looking for a certain value or combination of values within the result set and you could stop processing once you find it? Do you need to finish the whole loop anyway?
    5. Logic might move out of critical section to a secondary loop. The API here returns the entire array of values. That's fine if we actually just want to display all the values somewhere. But what if we want to use those values to execute some logic? Now we must loop through all those values again, requiring two loops instead of one.
  3. Terse, deterministic code introduces a level of obfuscation because the developers now have a reliance on a more abstracted API.
    1. Developers risk losing the skills to understand more concrete coding patterns
    2. Solutions to business problems are filtered through a more rigid set of options, creating less flexibility and creativity

Again, I am just playing devil's advocate. All in all, I REALLY LIKE your idea and obviously it is very helpful for you! You seem to be referencing a project that you have created. I'm curious. Is that available as a store or share app?

FYI - there is a new official code API that is going to be generally available in the ServiceNow Paris release called GlideQuery (official documentation should be available soon - Aug/Sep 2020??) that also is geared toward the same idea; helping developers avoid hitting "same type of bug over and over ". In the meantime, check out these resources for more info on GlideQuery:

- It was discussed during K20 recently by the ServiceNow developer Peter Bell, whose team is developing it.
https://events.servicenow.com/widget/servicenow/knowledge2020/myagenda/session/1581555110988001mNP1?... (this link might only be available to K20 attendees for limited time, not sure)
- Jace Benson explained how it can be installed and used in Orlando.
https://jace.pro/post/2020-05-29-how-to-install-glidequery/

Please 👍 if Helpful

0 Helpfuls
Javier Arroyo
Mega Guru
‎08-18-2020 03:35 PM

Hi Mwatkins.

Thank you for the response and taking the time reading and play devils advocate. Enjoyed reading it.

In terms of the new GlideQuery, I read what I found, tested it and came with the conclusion that it leaves issues that interest me unsolved. While it is a thumbs up and step in the right direction, I disagree with its design (from a month or so of sporadic exploration). It feels like it should have had in injection of advanced JS practices. 

Could it have been made into some sort of monad? or support for function composition, the use of unary methods, array and gliderecord handled through transposing? even partial application introduced? That leaves me considering it a fluent GlideRecord that I will need to wrap to configure lookups. 

 

My code isn't really an API that extends or makes up for GlideRecord (nor available in the store), it's more of a factory for GlideRecord that given a configuration file, builds a GlideRecord for a specific table. It has 2 additional methods called (1 for demo purpose above) queryWhileCompose and retrieve. 

It is meant to be used functionally/declarative. Because it is configuration/options driven, it readily supports new functionality. If SNow creates a new GlideRecord, it takes one code change in my entire codebase to support it: the factory. It is not meant as an adapter, nor really as a decorator upon GlideRecord, it behaves exactly as GlideRecord would at all times, for better or worse... 

Because the factory is configuration driven and the factory not replacing OOTB behavior, points 1 and 2 don't come into play. The goal was to build something I don't have to touch based on vendor updates.

Point 3:
  "Terse, deterministic code introduces a level of obfuscation because the developers now have a reliance on a more abstracted API."

Obfuscation I understand it to mean not really knowing native APIs caused by replacing them with factories. Because my factory is configuration driven, developers must know how the native API works. The behavior of GlideRecord is intact. 

However, in general, I agree that abstractions move focus away from native APIs, which isn't necessarily a bad thing; just different. I place very little value in a developer with intimate knowledge of native APIs. All this tells me is that he's lookup the apis enough times to learn them. API knowledge isn't necessarily a sign of a coder who can clearly articulate his thoughts on to code well enough to minimize interpretation times, regardless of how basic that code is. Say, something like a GlideRecord query.

I.E: A 7 line glide record to grab open incidents by active users, compared to coding: Incident().openRecordsByActiveUsers(). Who, in this case, created more readable code?

Point 3a:
"Developers risk losing the skills to understand more concrete coding patterns".

There is much to be said about this. What sort of programmers are in question? are they low level, high level, api builders, process builders, their competency level, etc.

Articulation requires a heighten level of vocabulary. It helps with identifying when and how code should be terse, when patterns should be used and why. For example, the closer to the user a piece of code is located, the less code it should contain, and the more general it should be; it needs to be declarative. Not until it begins pulling away into low level that those strict patterns should appear. that's the place for the sort of concrete pattern I presume you mean. The skills don't diminish, they shift focus to when, where, why and how more than using them lavishly. This is the part where I don't want to know if a developer has intimate knowledge of patterns or apis. I want to know if he understands when to use it and why. 

Point 3b:
"Solutions to business problems are filtered through a more rigid set of options, creating less flexibility and creativity"

It makes me feel warmer and fuzzier to think of this as a desire to reduce comprehension times for the next reader. This really is because I'm fully on the side that supports the use of abstractions to create extremely compose-able, flexible, and scalable code. Abstractions help programmers spotlight atomically, rather than in too great a surface area. This frees them to solve a new problem rather than resolving an already solved problem differently.

It's like using Flow Designer rather than workflow. Lets configure things on the front end, and let the low level code take care of what i want to do. I don't care that I don't know it's doing, rather that it is doing it as expected. No reason to ask programmers to re-build the very same lines, each and every time the same problem comes up. It's already built for you, use it...

 

While my GlideRecord factory leads to cleaner code than do both GlideRecord, and GlideQuery, I'm not happy enough with it to introduce it to the public. And, I'm honestly looking for ServiceNow to build something I like so I don't have to do it myself.  though, if GlideQuery is a prelude to what's coming, there will be no option but to build a GlideMonad myself with transducing support. 😞

Samples of my factory:

var gr = makeDBFor(tableName).query(encodedQuery);
var gr = makeDBFor(tableName).retrieve(optionsObject);
var gr = makeDBFor().composeWhileRetrieveFor([functionsToExecuteAgainstGR])(encodedQuery);
var gr = makeDBFor().composeWhileRetrieve([functionsToExecuteAgainstGR])(optionsObject);

makeDBFor(tableName).deleteMultiple(optionsObject);

the benefit:
Build the encoded query or options object independently with the ability to reuse anywhere. 
Build all the functions to be executed for each record in the result set, or after they've been while.next().
Each function can run for every record exactly one time, while going through while.next(). It's basically composing over each glide record.
Or, the query is created, then all the functions executed after the while.  

this frees the developer to solve new problems rather than building boilderplate code. The more mature our codebase grows, the less code is required to solve problems.

As an example.
A SI 300 lines long was refactored to 10 lines.
Such practices then leads to our latest piece of code in a scoped application.

function canUserCollaborate (sysUserSysId) {
        var sysId = sysUserSysId || gs.getUserID();

        return SysUserRole().hasRole(sysId, Role.RESTRICT_RECORDS_BY_DEPARTMENT_ITIL);

    }

  


 

Mwatkins
ServiceNow Employee
ServiceNow Employee
‎08-18-2020 04:11 PM

Very nice! I'll pass this along to the GlideQuery folks. Have you posted this in the IDEA portal for a product enhancement? Not sure if anyone has put in an idea for a monad for common GlideRecord patterns. If you do, I'll be the first to upvote.

0 Helpfuls
Javier Arroyo
Mega Guru
‎08-18-2020 05:05 PM

I faithfully presumed Monads and function composition were the logical next step for everything ServiceNow. I shall take your advice and post it at IDEA.

0 Helpfuls
Peter Bell
ServiceNow Employee
ServiceNow Employee
‎08-19-2020 10:31 AM

Hi Javier,

GlideQuery's only purpose as this point is to facilitate querying tables. It does however use its' own Stream monad for purposes of reading/transforming records lazily, so as not to run out of memory. Stream is generic in that it takes a function which returns items and could be used outside the context of GlideQuery if needed. For example:

new Stream(Math.random)
    .map(Math.round)
    .map(function (num) { return num === 1 ? 'heads' : 'tails'; })
    .limit(10)
    .forEach(gs.info)

Function composition (or partial application, currying, etc.), while indeed useful isn't a goal of GlideQuery. Our approach is to create an API with safety, modern functional goodness, while not being too intimidating to existing GlideRecord customers. This is why, for example, GlideQuery uses the familiar pattern of

.where(field, operator, value)

as it reflect's GlideRecord's addQuery method.

I enjoy piping and composing functions, I just see that as a possibility for another API, but who knows 🙂

0 Helpfuls
Javier Arroyo
Mega Guru
‎08-19-2020 03:55 PM

Good stuff@Peter Bell . Thank you for the run down. I'll be on the look out for future exciting apis. 


My pipe-dream 🙂 is to have SNow provide its own OOTB functional API (without the of this, constructors (no new keyword to instantiate), apply, bind, etc).  

0 Helpfuls
Mwatkins
ServiceNow Employee
ServiceNow Employee
‎08-19-2020 04:42 PM

Interesting distinction between API and factory. I've had to go brush up on my programming concepts 🙂 Perhaps the phrase, abstraction layer, would have been better than API.

After thinking a bit more about your replies to my initial comment. There is one reply that kept coming back to my mind. You said, "Because the factory is configuration driven and the factory is not replacing OOTB behavior, points 1 and 2 don't come into play."

Points 1 and 2, in this case were:

1. While creating less code surface, API's introduce complexity in the internal logic

2. The creation of more complexity inside the API introduces performance overhead

I think my points may have lacked clarity. When I said "API's introduce complexity in the internal logic", what I meant was "[abstraction layers require] complexity in [their own] logic". That is to say, I don't mean introducing complexity to any out-of-box code, like GlideRecord. I meant the logic of the javascript loop itself. The loop logic is being abstracted behind the factory methods, like composeWhileRetrieveFor(), and therefore must be careful to follow best practices relevant to loops.

Furthermore, the creation of the logic to ensure efficient loops would become complex because it would need to anticipate possible use and misuse and have logic that applied the appropriate best practices. As I'm sure you know, looping through GlideRecord is not a complete wash/rinse/repeat exercise and certainly can have major performance implications if not done correctly. For example, writing a successful performant loop against a table with only 10 records is very different than writing one against a table with 10,000,000 records. Since the code inside composeWhileRetrieveFor() doesn't know ahead of time if it will run against 10 records or 10,000,000 records, it will need to always apply the same protective logic - whether that logic is really needed or not.

All the sub-points that I listed under topic #1 are best practices that apply to the loop logic itself. Therefore a performant factory pattern that replaces looping through GlideRecords would need the corresponding best practices built-in, right?

e.g. 

  1. Protection against large arrays, deep loops
  2. Protection against bad inputs, invalid data types, null/undefined values
  3. ...
0 Helpfuls
Version history
Last update:
‎04-04-2020 01:32 PM
Updated by: