1

I'm refactoring legacy code and I'm having a challenging time unraveling the order in which these conditions are evaluated. I'm assuming the two most nested conditions take precedence but I haven't seen double parentheses used in an if condition like this before.

if(isTest == false &&
            (l.school__c == 'name_of_school1' 
            || l.school__c == 'name_of_school2'
            || l.school__c == 'name_of_school3')
            &&((l.status != 'Closed' && 
            l.Rating == 'App Pending')
            ||(l.status == 'Application'
            && l.Rating == 'App Submitted'))       
            && l.app_tool__c == true
            &&(trigger.isInsert
            ||(trigger.isUpdate        
            &&((trigger.oldMap.get(l.Id).App_Submitted_DateTime__c == null
            && l.Rating == 'App Submitted')
            ||(trigger.oldMap.get(l.Id).App_Pending_DateTime__c == null
            && l.Rating == 'App Pending')))))
            {...  

What is the best way to make this code readable?

2

Eesh.

Fortunately, none of the expressions here have side effects. Otherwise, the order of evaluation and the use of short-circuit evaluation (a && b does not evaluate b if a is false) would really play havoc. Just remember as you're reading this that each nested set of parentheses is evaluated "depth-first" to yield a Boolean value that becomes part of the parent expression.

A && (B || C) 

is true if A is true and either B or C is true. If B was a more complicated, parenthesized sub-expression, nothing changes - that sub-expression is simply evaluated first to find B's value.

Indenting the code out and adding newlines can help unravel the logical structure. Here's a first pass:

isTest == false 
&& (l.school__c == 'name_of_school1' 
    || l.school__c == 'name_of_school2'
    || l.school__c == 'name_of_school3'
   )
&& (
    (l.status != 'Closed' 
     && l.Rating == 'App Pending')
    || (l.status == 'Application'
        && l.Rating == 'App Submitted')
   )       
&& l.app_tool__c == true
&& (trigger.isInsert
    || (trigger.isUpdate        
        && (
            (trigger.oldMap.get(l.Id).App_Submitted_DateTime__c == null
             && l.Rating == 'App Submitted')
            || (trigger.oldMap.get(l.Id).App_Pending_DateTime__c == null
                && l.Rating == 'App Pending')
           )
       )
   )

A next step can be to group by levels - move all the single-element conditionals first, and then descend plies to try to simplify and reorganize the more complex elements. It's also much less verbose to use boolean_var and !boolean_var rather than the explicit comparisons to true and false.

!isTest 
&& l.app_tool__c
&& (l.school__c == 'name_of_school1' 
    || l.school__c == 'name_of_school2'
    || l.school__c == 'name_of_school3'
   )
&& (
    (l.status != 'Closed' && l.Rating == 'App Pending')
    || (l.status == 'Application' && l.Rating == 'App Submitted')
   )       
&& (trigger.isInsert
    || (trigger.isUpdate        
        && (
            (trigger.oldMap.get(l.Id).App_Submitted_DateTime__c == null
             && l.Rating == 'App Submitted')
            || (trigger.oldMap.get(l.Id).App_Pending_DateTime__c == null
                && l.Rating == 'App Pending')
           )
       )
   )

That gets us here. It's not a ton better, but it's starting to become clearer to me that this is probably an elaborate error checking system. The biggest concern to me is the explicit references to trigger context variables, which suggests this is part of a badly-structured trigger.

Some next steps to work on factoring this out could be:

  • move the school names into a Set<String> and use a single condition, schoolNames.contains(l.school__c).
  • break the logic into nested if statements rather than this incredibly elaborate conditional.
  • don't handle trigger events differently here; instead, factor error-checking logic into a separate insert and update handler.
  • consider factoring elements of the logic out into their own methods, like hasValidStatusAndRating().
  • move portions of the logic into validation rules.

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.