IF with && and || condition SalesForce Apex

Question

I have a trigger that checks the stage of an OPP before it updates.

What I want to do is check if the new Stage is 'X' then the Old Stage is not 'Y' or 'Z' there could be 4 more stages, so if a new stage is this and older stages is not one of the 'a, B, C' then go in and throw an Error. Please see the code below for example.

trigger StageCheck on Opportunity (before update) {

    for(Opportunity OPP : Trigger.new)
        
    {
    string OldStage  = trigger.old[0].Stage;
    string NewStage  = trigger.new[0].Stage;
        
    if(OldStatus == 'A' && NewStatus != 'B')
    {
        OPP.addError('Status can only move to B');
        return;
    }
     else if(OldStatus == 'B' && NewStatus != 'C' || NewStatus != 'A' )
        {
        OPP.addError('Status can only move to Stage A or C');
        return;
        }
 
    }
}

Currently, it keeps firing even when my new stage is A or C. What am I doing wrong? Please let me know, thanks!

Answer 1

At a high level this is an issue of one of the basics of programming, operator precedence.

Given the following expression
X && Y || Z

It could be evaluated two ways.

• You can evaluate X && Y first, then use the result in the final operation <IntermediateResult> || Z
• You can evaluate Y || Z first, then use the result in the first operation X && <IntermediateResult>

When there is more than one syntactically valid way to evaluate an expression, it's said to be ambiguous. It's important that one possible evaluation is chosen, and that it's chosen consistently.

The rules to determine how to evaluate expressions are fairly common, and much like math. Evaluation happens from left to right, and follows the Operator Precedence for Apex.

&& has higher precedence than ||, so your expression is evaluated as (X && Y) || Z.

Another thing

There's also short-circuit evaluation to keep in mind. If you have TRUE || <something>, it doesn't matter what the result of <something> is because if any part of an OR expression is true, the entire thing is true. We (well, Salesforce) can skip evaluating the entirety of <something>. Why waste time calculating something that won't effect the end result?

Same thing with FALSE && <something>. If any part in an AND expression is FALSE, the result is guaranteed to be FALSE.

Putting it together

I have to make the assumption that the thing you're having trouble with is the else if(OldStatus == 'B' && NewStatus != 'C' || NewStatus != 'A' ) in your example (your question isn't clear enough to know that this is indeed the issue).

If that's the case, then the fix is to use parenthesis to make your expression less ambiguous (again, much like in math).

else if(OldStatus == 'B' && (NewStatus != 'C' || NewStatus != 'A'))
would ensure that the NewStatus parts are evaluated before AND-ing with OldStatus == 'B'.

Answer 2

What you need is a "valid map" to determine which stages can be gotten to from which other stage. You don't need any complicated if statements with such a setup.

Here's a hard-coded example:

Map<String, List<String>> validChanges = new Map<String, List<String>> {
  'A' => new String[] { 'B' },
  'B' => new String[] { 'A', 'C' },
  // ... more mappings ... //
};

Once you have this all mapped out, you can reduce your main logic to just:

for(Integer i = 0, s = Trigger.size; i < s; i++) {
  String oldStatus = Trigger.old[i].StageName, newStatus = Trigger.new[i].StageName;
  String[] validStatusChanges = validChanges.get(oldStatus);
  if(oldStatus != newStatus && !validStatusChanges.contains(newStatus)) {
    Trigger.new[i].StageName.addError(
    'You cannot move from '+oldStatus+' to '+newStatus+
    '. Valid changes are: '+String.join(validStatusChanges,', '));
  }
}

(Note, you could write it other ways, I just find this seems to read pretty easy).

Even better, this allows you to specifically tell the user which valid changes they may make.

If you wanted to make this not hard-coded, you could use a Custom Metadata object to store the valid mappings, and decode that during runtime.

Map<String, List<String>> validChanges = new Map<String, List<String>>();
for(StageChange__mdt change: StageChange__mdt.getAll().values()) {
  String[] stageChanges = validChanges.get(change.OldStage__c);
  if(stageChanges == null) {
    validChanges.put(change.OldStage__c, stageChanges = new String[0]);
  }
  stageChanges.add(change.NewStage__c);
}

This requires a bit more setup, but allows you to change the legal steps without updating any code at all.

Sometimes, the best solution to a problem isn't to try and attack it head-on, but to find an easier method. Hard-coding a list of values in a huge if-else chain is just asking for trouble. Not only do you have to deal with maintaining code, but code coverage for a head-on attack would be challenging compared to more dynamic methods.

Answer 3

Derek F's post has it most of the context around this issue, but I just wanted to add one more thing.

An evaluation of (NewStatus != 'C' || NewStatus != 'A') will always result in TRUE.

The evaluation is either it is NOT C OR NOT A. Each of those conditions is evaluated on their own.

Let use the example where NewStatus = 'C',

First, it looks at the first part: is NewStatus not 'C'? No, that part is false.

Then evaluate the next part, is it not 'A', Yes. Therefore the whole condition evaluates to TRUE.