So, one of my junior developers came to me the other day with a question that I thought would be great for a wider discussion as to what NOT to do to your ServiceNow instance and how to go about refactoring code to squeeze out the best performance you possibly can.
It all started innocently enough. Due to some shuffling of the internal staff at our client, our allocations have also shifted somewhat, and we’ve inherited some work from a previous development team. So, when attempting to close off the update sets for this piece of work, they flagged some issues with our automated code quality checks.
The flag was raised on some client script that was using synchronous AJAX calls, and the question to me was “Should I look to fix that issue or ignore it as it’s not something I’ve worked on?”
Now, my attitude to fixing code is, if you release it, then you’re responsible for it, no matter who wrote it, so yes, it should at least be looked at to see if you can fix it.
So, I sidled over to his desk and this is what I was seeing:
There are several things wrong with this script that are immediately apparent:
- Default comment left in the script (not that this script requires much in the way of comments, but a specific single-liner of what the GlideAjax call is doing wouldn’t go amiss
- The ‘if’ really is redundant leading to repetition of code
- Nomenclature is confusing, you should never conflate the number field on a ticket with sys_id – if you’re going to call something sys_id then you should use the actual sys_id of the record
… but the real elephant in the room is:
- That synchronous AJAX call.
Now, before we dive deeper into this script, note that it’s an onLoad client script. We’ve just come from the server, so why on earth would we want to go back there whilst the form is loading? Surely with a little thought we could simply ship the data over the wire to the client instead of forcing a round-trip?
With this in mind, let’s look at the Script Include that the AJAX call is executing:
On seeing this, my initial thought was why not simply have this calculation in a business rule and ship the data to the client in the scratchpad? This initial thought was quashed however as there are also some onChange client scripts which also call this function anew. You could argue that the onChange calls are probably redundant as the likelihood of the number of incidents being assigned to this problem record changing whilst you’re editing the record is practically zero, it’s probably better to err on the side of caution since we’re not aware of the full end-to-end process they’re going through at this point.
However, this script also serves as an object lesson in shoddy code. To wit:
- There are no comments in the script detailing exactly what the method is for. In script includes, methods should always be commented (and those comments kept up to date) so the developer coming in behind you knows what it’s supposed to be doing.
- The one comment that is there is incorrect and doesn’t match with the logic for the line of code that’s been implemented. After checking it turns out that the comment is correct, and the logic is incorrect, which is lucky given my point immediately below…
- Comments in the code should be about why you’ve done something the way you have and not what the code they’re commenting does (unless the code is very opaque).
- The code is inefficient. To get the count of lines in a recordset, we really should be using a GlideAggregate and not a GlideRecord to cut down on network traffic and time to execute. Furthermore, looping over what could be a large recordset to obtain a single number that is not going to change is extremely bad coding.
- Again, the ‘if’ block is redundant and could be collapsed.
So, there we have it. A simple case of some very poor code. So now, let’s look at how to fix it.
First off, we want to eliminate the round-trip in the onLoad client script. There are a couple of options here:
- We could break out the body of the server function to a generic, reusable script include and then have another client-callable wrapper which calls it (so we can still make GlideAjax calls) and add a Display Business Rule to handle the onLoad part
- We could replicate the code in the Script Include into a Display Business Rule
- We could amend the Script Include so that the method can be called from either client or server with a little trickery
We’ll go with option 3 since option 1 adds further to the script sprawl we’re already encountering, and option 2 just adds to the maintenance nightmare since we would have decoupled code that needs maintenance in multiple locations should anything change.
Checking if a client callable Script Include has been called from the server is simple enough. If you look at the AbstractAjaxProcessor object, you’ll notice that there is a ‘request’ element in there. When calling from the client, this contains the HTTP request details, yet if instantiating from the server this remains as ‘undefined’. We can use this to our advantage to pass in a parameter to the method. Thus, our method call might now look like:
This is also definitely a case where you would want to have specific comments in the code explaining just what you’re doing and why as this is out of the ordinary for most standard ServiceNow code patterns.
Still, now that we’ve got this, we can craft a simple Display Business Rule to call the method and return the result on the scratchpad for the client:
(The observant amongst you will also notice that I’ve used current.sys_id as the input parameter – more on that later).
This then allows us to rewrite the original onLoad script in a much better fashion:
Finally, we can now turn our attention to the Script Include proper and look at how we can make that as fast as possible (and correcting the logic errors along the way…).
We’ve already seen how we can make the method callable from both client and server, but we also need to look at the following:
- Logic error in query
- Use of sys_id rather than ‘number’ as sys_id
- Use of GlideAggregate rather than an inefficient GlideRecord query.
The logic error is easy. That’s a simple change to the query parameter thus:
Use of the actual sys_id is also easy, and again is just a simple change to the query. However, we must be mindful to go back and change this in the client scripts that do call this method so that they use g_form.getUniqueValue() instead of g_form.getValue(‘number’);
Using the GlideAggregate is a little more of a stretch but pays dividends since the aggregation is performed in the database (so should be lightning fast), and doesn’t return a full recordset, so the amount of data shuttled between ServiceNow and the database is far smaller, and hence faster.
Our final version of the Script thus becomes:
So, let’s recap over what we’ve done:
- We’ve removed a round-trip to the server on form load, thus speeding up the responsiveness of the application for the user – always a good thing.
- We’ve reduced the load on the database and the network by eliminating the volume of data that’s being shipped over the wire between the database and application tiers.
- We’ve refactored the server-side code to eliminate a very wasteful loop.
- We’ve corrected a logic error (although I’m not entirely convinced that the logic on showing/hiding the form section isn’t reversed, although if it is it’s a trivial fix).
- We’ve made the code easier to read by formatting consistently and adding appropriate comments, thus reducing maintenance overhead in the future.
Could we go further? Absolutely. The naming of the method is, to my mind, incorrect. We don’t get the incident severity, we’re determining a count of linked incidents having impact < 5 so the server script and method should either be renamed to something more appropriate or amended to return the counts of various incident severities (and the client scripts amended accordingly). Furthermore, I’m not a huge fan of script sprawl. Having many Script Includes containing a single method is wasteful of resources and more likely to result in a cache miss when trying to instantiate.
Therefore, I’d much rather see a general PM_UtilsAJAX script containing all the various client callable scripts in one single place. Makes it much easier to find what you’re looking for, and ServiceNow is more likely to keep it in its cache.
… but those are battles for another day.