1
\$\begingroup\$

For some reason I always have a negative feeling towards having many if statements in my JavaScript functions. But I've got some code that is processing an object that defines rules of an event. I need to determine one or many things to do based on the "rules" outlined in each object.

(Particularly four if statements deep within a for loop just gives me the heebie-jeebies for some reason.)

Is there a faster way to handle this type of rules-like processing? Clearly I could break this into some smaller functions to clean up the syntax, but I am mostly looking for feedback/suggestions around speed of processing the request.

The idea here is to capture customer conversion events throughout a user's interaction with a job application. Each conversion has an 'initiate' and a 'complete'. The "rules" which determine when to initiate and when to complete each conversion are setup in a JSON file which I then parse into a JavaScript object: applicationConversionEvents.

The JSON looks something like so:

[
  {
    "eventId": "view",
    "eventName": "App View to Start",
    "initiateOnStepNumber": 100,
    "completeIfAuthenticated": true,
    "completeOnElementClick": "first_name"
  },
  {
    "eventId": "submit",
    "eventName": "App Start to Submit",
    "initiateOnEventComplete": "view",
    "completeOnStepNumber": 450,
    "completeOnEventInitiate": "decline"
  },
  {
    "eventId": "decline",
    "eventName": "App Decline",
    "initiateOnStepNumber": 350,
    "completeOnStepNumber": [
      350,
      375
    ]
  }
]

I am then using the following function to parse the rules for each conversion event, and when appropriate fire off a new event either as an 'initiate' or a 'complete'. I also track what events fire in sessionStorage to avoid duplicates per application.

var _processApplicationConversionEvents = function(pageInfoArrayIndex, options) {
    _log('Processing application conversion events.', LOG_LEVEL.INFO);

    if(!Array.isArray(applicationConversionEvents)) return;
    if(!_isValidPageInfoArrayIndex(pageInfoArrayIndex)) pageInfoArrayIndex = 0;

    _log('Conversions for pageInfoArrayIndex: ' + pageInfoArrayIndex, applicationConversionEvents, LOG_LEVEL.DEBUG);

    var pageInfo = _getPageInfo(pageInfoArrayIndex, options);
    _log('pageInfo details for conversion events:', pageInfo, LOG_LEVEL.DEBUG);

    for(var i = 0, len = applicationConversionEvents.length; i < len; i++) {
        var conversionEvent = applicationConversionEvents[i];
        if(typeof conversionEvent !== 'object') continue;

        if(conversionEvent.initiateOnStepNumber) {
            var appStepNumber = _asArray(conversionEvent.initiateOnStepNumber);

            if(appStepNumber.indexOf(+pageInfo.appStepNumber) > -1) {
                _newConversionEvent('initiate', conversionEvent, pageInfo);

                if(_toLowerCaseString(conversionEvent.completeIfAuthenticated) === 'true' 
                   && pageInfo.authenticated) {
                    _newConversionEvent('complete', conversionEvent, pageInfo);

                    if(!conversionEvent.completeOnElementClick) continue;
                    _removeConversionOnClickListener(conversionEvent.completeOnElementClick);
                }
            }
        }

        if(conversionEvent.completeOnStepNumber) {
            var appStepNumber = _asArray(conversionEvent.completeOnStepNumber);

            if(appStepNumber.indexOf(+pageInfo.appStepNumber) > -1) {
                _newConversionEvent('complete', conversionEvent, pageInfo);
                continue;
            }
        }

        if(conversionEvent.completeOnElementClick) {
            _addConversionOnClickListener('complete', conversionEvent, pageInfo);
        }
    }
}
\$\endgroup\$
1
\$\begingroup\$

There is really not enough context here to talk to any performance improvements. On the surface, there doesn't seem to be any way to optimize on this simple O(n) array iteration. I have no idea what is happening in any of the functions being called though.

You have some room for improvement on coding style IMO.

You understand the continue command but you are not using it effectively. Consider inverting your conditionals in cases like this to get code structured more like this:

if (!someobj.hasOwnProperty(someProp)) continue;

// some logic

if (!someOthetRule) continue;

// some more logic

This eliminates all those nested ifs.

Your code is also extremely densely packed making it hard to read. You should really try to keep your lines under 80 characters, breaking longer lines across lines. And while I am normally a big proponent of meaningful variable and function names, you seem to almost take it to the extreme, using overly verbose variables when sometimes it does not seem necessary. Take for example how in your for loop you store the array length into conversionsLen when something short and simple like len would be just fine since it is clear what is happening here.

\$\endgroup\$
  • \$\begingroup\$ Thank you Mike. There's always room for improvement, so I appreciate the styling feedback. Due to the organization I work with, I do have to be super verbose in my variable and function names. I'm also open for when I can make them short and simple without them being misunderstood. \$\endgroup\$ – Jeremy H. Nov 11 '16 at 16:03

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.