Design Patterns for IF Statements

Javier Arroyo · ‎08-01-2018

Ever question how an IF statement managed to spawn out of control? Ever witness its maturation from one, to three, to ten, to 100 to 200 conditions?

Everyone must have gone through it. The:
why should it be changed?

It's simple to update, just add it to the end. Easy pickings and move on to the next item.

Consultant: "I've been coding for years, since before you were born. Look, right here, add it right at the end. As more come, keep on adding to the end."

Bewildered Client: "what happens if the choices change?"

Consultant: "The use case we were given read that the choices will never change but, if the do, just go right here, make the update and you are done. No more than 5 minutes"

Bewildered Client: "Can I get back my 300K?"

Consultant: "do you require a service agreement? our professional services is the best in the industry."

Conversations as the one above are ubiquitous in our field. A platinum partner and consulting firm, staffed with ServiceNow accredited Sr. Developers and Architects implementing solutions that are accompanied by the likes of a single script 1700 lines long, using 299 IF conditions as flow control.

Quality is being ignored somewhere along the line of programming. Should we, as Implementation or Code Architects, not be able to identify when code won’t scale? When to use one design over another? When to push back against developers for trivial programming techniques?

The programming world has guidelines that have surfaced through trial and error, at times plain ol’e ingenuity. It is those very practices that should be familiar... from Sr. Developers, Code Evangelists, Code Reviewers, all the way to Implementation Architects. Unmanageable code should not be an issue at that stage of one's career, at the Stage of product maturity of ServiceNow. It is simply impossible to solution a quality implementation if basic design and coding architectural knowledge is a weak point.

This article will examine IF statements, attempting to introduce established ideas for manageable and readable conditional controls.

The basic guidelines that work well are:
1. An IF statement should roughly have no more than three conditions.
2. Refactor IF statements with more than three conditions into a switch statement.
3. Switch statements with more than five conditions should become Objects.

Guidelines in action:

The target will be the excerpt of inline code below. It will be refactored into a scalable solution, as if maturing over time.

if (carBrand === 'Honda') {
    gs.info('It has pretty round wheels');

} else if (carBrand === 'Toyota') {
    gs.info('It has big round wheels');

} else if (carBrand === 'Ford') {
    gs.info('It has nice wheels and a spare');

} else {
    gs.info("your car wheels aren't nice");

}

Step 1:
Wrap inline code with a properly named function to begging addressing scalability, cleanliness and readability, while massaging it some to return a value rather than directly printing it.

function saySomethingNiceAboutACarBrandIF(carBrand) {

    var message = null;

    if (carBrand === 'Honda') {
        message = 'It has pretty round wheels';

    } else if (carBrand === 'Toyota') {
        message = 'It has big round wheels';

    } else if (carBrand === 'Ford') {
        message = 'It has nice wheels and a spare';

    } else {
        message = "Oops. your car wheels aren't nice";

    }

    return message;

}

Step 2:

With the addition of a new car brand (GMC), the IF will be turned into a SWITCH to improve readability.

function saySomethingNiceAboutACarBrandSwitch(carBrand) {

    var message = null;

    switch (carBrand) {
        case 'Honda':
            message = 'It has pretty round wheels';

            break;

        case 'Toyota':
            message = 'It has big round wheels';

            break;

        case 'Ford':
            message = 'It has nice wheels and a spare';

            break;

        case 'GMG':
            message = 'None have the wheels I have';

            break;

        default:
            message = "Oops. your car wheels aren't nice";

            break;
    }

    return message;
}

Upon inspection it becomes aparent that neither the IF or SWITCH staments will lead to scalable approcaches. Redability will also deteriorate as more car brands are added; more importantly, as more complex logic is added to each condtion. Because the goal is to create maintainable code, then numerous lines of code under each condition can not be acceptable.

In addition, are the IF or SWITCH flexible enough to account for executing a specific brand without having to traverse the condition tree? or can the conditions be 'overwritten' or extended? If the execution tree can be traversed up to as many times as there are conditions, this implies the IF and SWITCH grow slower.

By refactoring IF/SWITCH statements into Objects code can be decoupled into single units, allowing for a plethora of positive results, including a performance boost.

Quality code leads to reduce maintenance and cost.

3rd and Final Step:

3.A:
Extract SWITCH cases into a Data Object/Data Dictionary

var carBrandNiceThings = {

    honda: 'It has pretty round wheels',
    toyota: 'It has big round wheels',
    ford: 'It has nice wheels and a spare',
    gmc: 'None have the wheels I have',
    generic: "Oops. your car wheels aren't nice"

}

3.B:
Refactor function to use the Data Object

function saySomethingNiceAboutACarBrandObject(carBrand) {

    carBrand = carBrand.toLowerCase();

    //user ternary to get correct brand
    var message = ((carBrandNiceThings[carBrand]) ?
                       carBrandNiceThings[carBrand] :
                           carBrandNiceThings['generic']);

    return message;

}

Thoughts on final step.
By introducing a data object, the artifact becomes fully scalable, maintainable and easy on the eyes. It also separates messages from code, a quick win...

As business needs evolve, each property may grow independently without cluttering the object.

The maintenance advantage to Data Dictionaries is the ability to expose the item as configurable, where it can be updated as necessary. Be it through sys_properties, sys_ui_message or table.

The final code is a robust solution to IF/SWITCH statements.

Extending functionality based on a new requirement
Use Case:

Add tracking specific to each available car brand.

Step 1:

Refactor Data Object into a configuration item and create a factory method to build the messages

var carBrandNiceThingsData = (function loadCarBrandNiceThingsData() {

    var niceThings = gs.getMessage('carBrand.niceThings.data'); //messages have become a sys_ui_message that contains a JSON string

    niceThings = JSON.parse(niceThings);

    return niceThings;

})();

Step 2:
Introduce tracker object BrandTracker

BrandTracker = {

    trackNiceThingsAbout( carBrand ) {
        gs.log( carBrand + 'is being tracked');

    },

    trackNiceThingsAboutFord: function( carBrand ) {
        BrandTracker.trackNiceThingsAbout(carBrand);

        //do ford specific stuff 
    },

    trackNiceThingsAboutGeneric: function(carBrand) {
        BrandTracker.trackNiceThingsAbout(carBrand);

        //do generic stuff 
    },

    trackNiceThingsAboutGMC: function(carBrand) {
        BrandTracker.trackNiceThingsAbout(carBrand);

        //do gmc stuff 
    },

    trackNiceThingsAboutHonda: function(carBrand) {
        BrandTracker.trackNiceThingsAbout(carBrand);

        //do honda stuff 
    },

    trackNiceThingsAboutToyota: function(carBrand) {
        BrandTracker.trackNiceThingsAbout(carBrand);
        
        //do toyota stuff
    }
}

Step 3:
Refactor Data Dictionary into a "functional" Mediator, leaving untouched the return value expected by saySomethingNiceAboutACarBrandObject function

var carBrandNiceThings = {

    honda: function (carBrand) {
        var message = carBrandNiceThingsData.honda;
        BrandTracker.trackNiceThingsAboutHonda(carBrand);

        return message;
    },

    toyota: function (carBrand) {
        var message = carBrandNiceThingsData.toyota;
        BrandTracker.trackNiceThingsAboutToyota(carBrand);

        return message;
    },

    ford: function (carBrand) {
        var message = carBrandNiceThingsData.ford;
        BrandTracker.trackNiceThingsAboutFord(carBrand);

        return message;
    },

    gmc: function (carBrand) {
        var message = carBrandNiceThingsData.gmc;
        BrandTracker.trackNiceThingsAboutGMC(carBrand);

        return message;
    },

    generic: function (carBrand) {
        var message = carBrandNiceThingsData.generic;
        BrandTracker.trackNiceThingsAboutGeneric(carBrand);

        return message;

    }
}

Step 4:
Refactor saySomethingNiceAboutACarBrandObject function to account for new functionality. Because there hasn't been a change to the return value of the function, changes to the code are limited to how it calls the data dictionary. (Executing them as parameterized functions)

function saySomethingNiceAboutACarBrandObject(carBrand) {

    carBrand = carBrand.toLowerCase();

    //execute carBrand mediator functions by carBrand       
    var message = ((carBrandNiceThings[carBrand]) ?
                        carBrandNiceThings[carBrand](carBrand) :
                            carBrandNiceThings['generic']('generic'));

    return message;

}

Javier Arroyo · ‎08-01-2018

Rather than authoring another post, I will refactor a Production Code sample from a vendor to show a proper Business Use Case for refactoring undesirable IF practices.

Original "Production Ready" Code

    varmac=source.mac.toString();
    varip=source.ip.toString();
    
    //need the sanity check on the mac and ip and all other import fields.
    if (mac.length==0) {
    ignore
    =true;
    gs.info('Ignore row, no mac address');
    }
    
    vardeviceNetFunc
    =source.netfunc.toString();
    vardeviceOsFinger
    =source.osfingerprint.toString();
    // Customer Integration: Nedd set the correct target.sys_class_name according to
    // customer's own cmdb table environment!
    // reset the target tables based on netFunc.
    if (deviceNetFunc.length>0) {
    gs.info(" Network Function is: "+deviceNetFunc);
    vardeviceType;
    if (deviceNetFunc=='Apple Mac OS X') {
    target.sys_class_name='cmdb_ci_computer';
    } else if (deviceNetFunc == 'CounterACT Device') {
    target.sys_class_name='cmdb_ci_server';
    } else if (deviceNetFunc == 'Linux Desktop/Server') {
    target.sys_class_name='cmdb_ci_linux_server';
    } else if (deviceNetFunc
    =='Mobile Device') {
    target.sys_class_name='cmdb_ci_computer';
    } else if (deviceNetFunc == 'Network Device') {
    target.sys_class_name='cmdb_ci_netgear';
    } else if (deviceNetFunc == 'Printer') {
    target.sys
    _class_name='cmdb_ci_computer';
    } else if (deviceNetFunc == 'Server') {
    target.sys_class_name='cmdb_ci_server';
    } else if (deviceNetFunc == 'Storage') {
    target.sys_class_name='cmdb_ci_storage_device';
    } else if (deviceNetFunc == 'Terminal Server') {
    target.sys_class_name='cmdb_ci_server';
    } else if (deviceNetFunc == 'Unix Server/Workstation') {
    target.sys_class_name='cmdb_ci_unix_server';
    } else if (deviceNetFunc == 'VoIP Device') {
    target.sys_class_name='cmdb_ci_computer';
    } else if (deviceNetFunc == 'Windows Machine') {
    target.sys_class_name='cmdb_ci_computer';
    } else {
    //ignore = true;
    gs.info("Unknown NetFunc from CounterACT");
    }
    } else if (deviceOsFinger.length > 0) {
    if (/mac/i.test(deviceOsFinger)) {
    target.sys_class_name='cmdb_ci_computer';
    } else if (/counteract/i.test(deviceOsFinger)) {
    target.sys_class_name='cmdb_ci_server';
    } else if (/linux/i.test(deviceOsFinger)) {
    target.sys_class_name='cmdb_ci_linux_server';
    } else if (/hp-ux/i.test(deviceOsFinger)) {
    target.sys_class_name='cmdb_ci_unix_server';
    } else if (/mobile/i.test(deviceOsFinger)) {
    target.sys_class_name='cmdb_ci_computer';
    } else if (/network device/i.test(deviceOsFinger)) {
    target.sys_class_name='cmdb_ci_netgear';
    } else if (/sunos/i.test(deviceOsFinger)) {
    target.sys_class_name='cmdb_ci_unix_server';
    } else if (/windows/i.test(deviceOsFinger)) {
    target.sys_class_name='cmdb_ci_computer';
    } else if (/vmware/i.test(deviceOsFinger)) {
    target.sys_class_name='cmdb_ci_vm_instance';
    } else {
    gs.info(" Unknown OSFingerPrint from CounterACT");
    }
    } else {
    ignore=true;
    gs.info(" No netFunct classification value from CounterACT");
    }
    })(source, map, log, target);

Because I don't know the original business case for some of the questionable practices of the code, I will be limited to commenting on, rather than fixing them.

Step 1:

Build Data Objects from IF Statements. Because the original code states that these next two items are customizable to match the client's CMDB structure. This will drop out of Script Includes to become configuration items as a sys_ui_property to allow adding items on the fly.

var CMDBCIClassNameNetFunc = {

    "apple mac os x": "cmdb_ci_computer",
    "counteract device": "cmdb_ci_server",
    "linux desktop/server": "cmdb_ci_linux_server",
    "mobile device": "cmdb_ci_computer",
    "network device": "cmdb_ci_netgear",
    "printer": "cmdb_ci_computer",
    "server": "cmdb_ci_server",
    "storage": "cmdb_ci_storage_device",
    "terminal server": "cmdb_ci_server",
    "unix server/Workstation": "cmdb_ci_unix_server",
    "voip device": "cmdb_ci_computer",
    "windows machine": "cmdb_ci_computer"

};

var CMDBCIClassNameOSFinger = {
    
    "mac": "cmdb_ci_computer",
    "counteract": "cmdb_ci_server",
    "linux": "cmdb_ci_linux_server",
    "hp-ux": "cmdb_ci_unix_server",
    "network device": "cmdb_ci_netgear",
    "sunos": "cmdb_ci_unix_server",
    "windows": "cmdb_ci_computer",
    "vmware": "cmdb_ci_vm_instance"

}

I have purposely not wrapped this functionality into a function as my desire is just to show what the body of the function will look like. The original is also wrapped in a function

Step 2:
Use Data Objects to replace IF conditions. I have used an IIF to decrease the amount of structured spaghetti code.

   var mac = source.mac.toString();
    var ip = source.ip.toString(); //JIBARICAN: Is this being used anywhere other than an unused placeholder?

    //need the sanity check on the mac and ip and all other import fields.
    if (mac.length === 0) {
        //JIBARICAN: Is there any reason this condtion doesn't skip over the functionality below?
        //JIBARICAN: Why test for class assignments when this row has been ignored?
        ignore = true;

        //JIBARICAN: change log to property file accessible to client.
        //JIBARICAN: This practice will clutter the logs unnecessarily
        //gs.info('Ignore row, no mac address');
    }

    //JIBARICAN: Should this functionality be even reached when mac === 0?
    var targetClass = (functionselectClass(deviceNetFunc, deviceOSFinger) {

        var className = getClassName(deviceNetFunc, CMDBCIClassNameNetFunc);

        if (className) {
            return className;

        }

        //@TODO: log something properlly about NetFunc

        className = getClassName(deviceOSFinger, CMDBCIClassNameOSFinger);

        if (className) {
            return className;
        }

        //@TODO: log something properlly for OSFinger

        function getClassName(key, ClassLookup) {
            if (key && ClassLookup.hasOwnProperty(key)) {
                return ClassLookup[key];

            }

            return;
        }

        //@TODO: log no classification found
        return className;

    })(source.netfunc.toString().toLowerCase(), source.osfingerprint.toString().toLowerCase());


    if (targetClass) {
        target.sys_class_name = targetClass;

    } else { //ignore when no class is found
        ignore = true;

    }

Extracting Comments would yield:

   var mac = source.mac.toString();
    var ip = source.ip.toString(); 

    if (mac.length === 0) {
        ignore = true;

    }

    var targetClass = (function selectClass(deviceNetFunc, deviceOSFinger) {

        var className = getClassName(deviceNetFunc, CMDBCIClassNameNetFunc);
        if (className) {
            return className;

        }

        className = getClassName(deviceOSFinger, CMDBCIClassNameOSFinger);
        if (className) {
            return className;
        }

        function getClassName(key, ClassLookup) {
            if (key && ClassLookup.hasOwnProperty(key)) {
                return ClassLookup[key];

            }

            return;
        }

        return className;

    })(source.netfunc.toString().toLowerCase(), source.osfingerprint.toString().toLowerCase());


    if (targetClass) {
        target.sys_class_name = targetClass;

    } else { //ignore when no class is found
        ignore = true;

    }

Daniel Draes · ‎08-01-2018

Thanks for this post. I do appreciate people taking time to think about good coding practice ! 🙂

Reading your steps I like the change from if to switch to data object, what I do not like is the Object oriented approach. This takes quite some developer skills which most SysAdmin do not possess. Debugging in case of failure for operation teams also is impacted.

In the world of ServiceNow I would much rather use a data lookup table and store the values there. A simple GlideRecord-Query is easy to write and understand. In addition we move the data maintenance piece to a place where no script knowledge is involved.

Javier Arroyo · ‎08-02-2018

I had written a reply which seems not to have posted, if it did, my apologies. Here is a second attempt. You are correct in that it is a higher skill solution, yet it is far more maintainable and easier to debug. I'll explain by using the refactored code from the freely available vendor code in my second post. what happens when the if, even when refactored to a switch, needed to account for all 700+ cmdb classes? what would happen, if on top of that, lookups are added? imagine the strain on the database when loading 500k CIs. Just imagine the size of the code to support a non object approach. The lookup approached, when decoupled from an object approach is a safe bet to hinder scalability a 100% of the time. The object approach would eliminate 100s if not 1000s lines of code. as well as the 500k db lookups required for each condition. what then happens when more CI classes are introduced? by shying away from the Object, a maintenance nightmare has been introduced. Who is going to maintain conditional statements 1000s of lines long? What admin will dream of changing conditions when a new requirement emerges? the object approach is no more than a representation of data stored somewhere. the somewhere often times happens to be in a db. the issue then is not that it is an object, rather HOW the object is built. in regards to debugging, this too is drastically reduced, there is one and only one place to look. If a key is failing to trigger code, it's not the code, but the data. with object as flow control, the mechanism that tests the condition needs to be tested once and only once. if it works for one key, it will work for all. in a switch or if, every condition has to be checked up to twice. in the CMDB example it would be once for each class, then again each time a key fails. what then if the failure is not at the conditional level but down further?

Kalaiarasan Pus · ‎08-02-2018

People need to print this blog and paste it at their desk.

Personally, I am questioning myself now as I have often overlooked at the abuse of IF statements and nested IF's.

Javier Arroyo · ‎08-02-2018

Thank you so much for the kind thoughts iamkalai.
Much appreciated.

sabell2012 · ‎08-27-2018

Jibarican nice job. I especially like your examples for refactoring. The next question inevitably is asked: How many "case" statements are allowable before the structure becomes untenable? This question led me some time ago to have yet a third structure/solution on-hand: the decision engine. This solution takes the case statement and reduces it to two components: 1) data, 2) decision engine. I have found this solution to be the ultimate in scaleability as it allows for fairly infinite growth of rules (and if designed right you basically have one table that can cover multiple engines. Also the table can then be added as an "admin" function to the navigation menu. You take a slight performance hit with this model when you read in the data from the table, but the trade-off is well worth it. I have implemented such engines in a number of solutions including workflows.

What made me tip over into this data driven model was when I arrived at the "object" lookups you mention in your refactoring. This lends itself nicely to being converted to that model.

Steven.

Javier Arroyo · ‎08-27-2018

Thank You sabell.

That is certainly it! How can design help scalability. If this concepts are the last to be considered, it might be a bit costly when we do attempt to refactor. I find that beyond 5 'case' statements, especially with some logic accompanying the case, are enough to warrant refactoring.

We use various sources for our data Objects such as Tables, UI Messages, Properties, Script Includes, or Incoming from webservices yet, same goal in mind. Can all the implementations be scalable and easily refactored, don't hard-code, don't hard-code!

The strategy is to enable trivial support of various Data Format as well as Data Source and still be handle it by the same engine, while having handing support of the conditions to admins:

1. decouple the interaction between data and engine with a factory-like object that returns the Data Dictionary based on an injected templates for Data Source transforming into the expected structure for the data selection engine (JSON format). Sort of like a "universal" adapter. Turn Data into the engines expected format to load to memory, say bootstrap a BeforeBusinessRules expecting the case conditions, then call the BootStrapped Object from other Business Rules. This eliminates more than one lookup to the original data source.

2. Pass in the result of the factory to the decision engine along with the parameter to match and return the result.

Now the engine can handle all incoming data sources or formats and still work. Not even a need to build another adapter. 🙂

Matt Hernandez · ‎02-03-2020

This is kind of fascinating. I took some time this weekend and translated this into a script include to use with "cost center types," to see if I could use it. I got the basics to work in my include but later when I tried incorporate my sample back into my real code I found my sample use case didn't quite apply to what I needed to do..ie. the brand-specific area of "do Honda stuff" "do Toyota stuff" (on mine it was "do accounts_receivable stuff", "do fund stuff", ....) would have required several more input parameters in my case to be useful.

But it was really provocative and now I have an example of something cool and will look for cases where I can change to this kind of coding. Thanks for a great article!

Javier Arroyo · ‎02-03-2020

Super glad you found it usefule, Matt!

Let me know if I can be of further assistance.

Tom Sienkiewicz · ‎02-05-2020

That is a great observation. I absolutely agree with the approach to minimize/simplify IF statements by turning them to switches etc.

With an object-oriented approach, i tcould work depending on the use case. One thing I always ask in such situations is: why should the client not be able to add their new conditions ad-hoc? Why rely on developers each time they just want to add a new case?

And then, of course depending on the use case, you can employ one of the following

- sys_choice table for simple values

- data lookups

- custom tables / m2m tables for more complex situations

- custom table with the use of Condition field where clients can easily build their own conditions. Very easy to evaluate with the use of GlideFilter

https://developer.servicenow.com/app.do#!/api_doc?v=newyork&id=r_ScopedGlideFilterCheckRecord_GlideR...

...and probably many more creative approaches.

I wish there was a place curated by experts, where we could collect all good ideas and create a best practice library. Community posts are a bit all over the place.

One last thought - with the new licensing model, you unfortunately have to think twice about using any custom tables and run it by your client, which is a bummer, because instead of focusing on great architecture, you need to become an accountant. oh well...

Javier Arroyo · ‎02-06-2020

Great thoughts! thank you, Tomas.

Data and behavior driven approaches should be the goal of customization; a topic seldom addressed, if ever. But, with each passing release, it's apparent that solutions once built to make-up for missing functionality, such as Decision Tables, have been implemented natively, actively rendering customization redundant. This raises the importance of quickly and painlessly reverting to OOTB to benefit from new offerings. Too, though making software engineers less of a demand, it raises the importance of clean and well crafted architecture. Which speaks to your point that there needs to be a library curated by experts to unsure scalability, not only of customization but, a painstaking return to OOTB.

The new model, while certainly understandable, means the platform is trying to prevent unscallable practices; they are ensuring their own survival. Whether the licensing does that is still up for debate but, what is already known is the challenged, even limitations presented to architects, possibly leading to "smart/creative" solutions that otherwise would have not been made.

What I do question, however, is if the ServiceNow space is in a position for an expert panel that makes code/architectural recommendations when the absence of software engineers, even qualified developers is still in the development phase?

Tom Sienkiewicz · ‎02-06-2020

Hi Javier,

absolutely agree that the trend is continuously moving towards more OOB and less customization. And yes this definitely requires well thought-out architecture.

I guess the shift is from freely designing whatever you need to carefully evaluating existing building blocks and new features popping up all the time and making use of that.

Your point regarding new limitations/challenges leading to new creativity is quite interesting. I agree with it generally, I just wonder if there is more positive creativity or just trying to work around the licensing implications (e.g. I heard of people abusing CMDB tables as they are not counted, or using complex JSON objects instead of adding a new table XD). Still, the new model is constantly evolving and perhaps good practices will be worked out in the process.

I think store apps have great potential here. Although having many API constraints, they are not limited in terms of tables, entitlements etc. - which I think means more and more customization will be emerging in this form, and can be shared by everyone. This is what I am focusing on now anyway.