How to avoid nested for loop condition for code optimization & reusability?

Question

My requirement:

• Restrict users from updating some specific fields.
• These fields can increase or decrease in the future. So, it should be dynamic.
• Checking should be done in the triggerHandler. There are no alternatives.

How I Implemented:

1. Defined a list(will be fetched dynamically in future, size will vary);
2. Then, in the nested loop, I checked whether one of those particular fields is updated.

What I want to do

• Implement the code without the nest for loop.

    //defining set of restricted fields
    List<String> listOfFields = ['Custom1__c', 'Custom2__c', 'Custom3__c',......];

    //iterating through list of updated records
    for (Integer i = 0; i < triggerNew.size(); i++) {
           for (String s: listOfFields){
               if(triggerNew[i].get(s) != triggerOld[i].get(s)){
                   triggerNew[i].addError('You can't update this field!');
               }
        }
    }

Answer 1

tl;dr

Nested loops are fine in some contexts. Here, you may want to enumerate all the fields that are in violation, so the user doesn't have to keep edit/save cycles, or even worse, discard all their changes and start over repeatedly. There is a way to check many fields at once to see if there are any changes, but if you want to identify which fields changed, you need a nested loop. This answer demonstrates several ways you can improve performance and reusability.

---

This specific example can be achieved by way of the often underutilized getPopulatedFieldsAsMap. Here's your code with just one loop:

String[] fieldsToCheck = ...;
for(Integer index = 0, size = Trigger.size(); index++) {
    Map<String, Object> oldValues = Trigger.old[index].getPopulatedFieldsAsMap();
    Map<String, Object> newValues = Trigger.new[index].getPopulatedFieldsAsMap();
    oldValues.keySet().retainAll(fieldsToCheck);
    newValues.keySet().retainAll(fieldsToCheck);
    if(oldValues != newValues) {
        Trigger.new[index].addError('You cannot update a field.');
    }
}

As a generic function, one could write something like this:

public static List<List<sObject>> findChangedRecords(String[] fields, sObject[] firstRecordList, sObject[] secondRecordList) {
    List<List<sObject>> results = new List<List<sObject>>();
    for(Integer index = 0; index < firstRecordList.size(); index++) {
        sObject firstRecord = firstRecordList[index];
        sObject secondRecord = secondRecordListList[index];
        Map<String, Object> oldValues = firstRecord.getPopulatedFieldsAsMap();
        Map<String, Object> newValues = secondRecord.getPopulatedFieldsAsMap();
        oldValues.keySet().retainAll(fieldsToCheck);
        newValues.keySet().retainAll(fieldsToCheck);
        if(oldValues != newValues) {
            results.add(new List<sObject>{ firstRecord, secondRecord });
        }
    }
    return results;
}

I'm using a List<List<sObject>> here only because I feel it reduces ambiguity as to which records are returned, and also allows you to inspect the results yourself, if you need to know which field, etc, but you could choose to return a wrapper class or something else.

From here, your trigger can just do something like this:

List<List<sObject>> changedCases = Utils.findChangedRecords(fieldsToCheck, Trigger.old, Trigger.new);
for(List<sObject> changedCasePair: changedCases) {
  changedCasePair[1].addError('You cannot edit a field you tried to edit.');
}

If you want to be more specific, as in which field cannot be edited, then you're back to needing a nested for loop.

However, the findChangedRecords method above is very efficient compared to a nested for loop, especially as the number of fields grow.

How efficient, you may ask?

I wrote this demo in my org (your results will vary):

String[] fields = new List<String>(sObjectType.Account.fields.getMap().keySet());
Account[] accounts = Database.query('SELECT '+String.join(fields,',')+ ' FROM Account LIMIT 2');
Integer loopCount = 100;
Long start1 = DateTime.now().getTime();
for(Integer index = 0; index < loopCount; index++) {
    for(String field: fields) {
        if(accounts[0].get(field) != accounts[1].get(field)) {
            // Do nothing
        }
    }
}
Long end1 = DateTime.now().getTime();
Long start2 = DateTime.now().getTime();
for(Integer index = 0; index < loopCount; index++) {
    Map<String, Object> oldValues = accounts[0].getPopulatedFieldsAsMap();
    Map<String, Object> newValues = accounts[1].getPopulatedFieldsAsMap();
    if(oldValues != newValues) {
        // Do nothing
    }
}
Long end2 = DateTime.now().getTime();
System.debug(LoggingLevel.ERROR, end2-start2);
System.debug(LoggingLevel.ERROR, end1-start1);

Outputs sample values like:

USER_DEBUG [22]|ERROR|138 // Efficient check time, ms
USER_DEBUG [23]|ERROR|1432 // Inefficient check time, ms

This output shows that the findChangedRecords method performs about 10x better. It is most efficient when you presume that most of the time, the checks will pass. If you assume that the checks will not pass, however, you can break as soon as you find the first error:

for (Integer i = 0; i < triggerNew.size(); i++) {
    for (String s: listOfFields){
        if(triggerNew[i].get(s) != triggerOld[i].get(s)){
            triggerNew[i].addError('You can't update this field!');
            break;
        }
    }
}

By adding a break here, if there is a prohibited edit, it can be more efficient than findChangedRecords on average up to 50x or more, depending on the number of fields you're comparing and if we assume that the edits could be on any random field (e.g. some may take less than 1 millisecond, others might take tens of milliseconds).

However, this approach will only find one field; if multiple are present, you have to still go through the entire list. I'd recommend this to avoid frustrating your users.

As such, if you want to provide the user with a list of fields they can't edit, and still want decent performance, you can create a hybrid approach:

public class Result {
    public String[] changedFields = new String[0];
    public sObject record1, record2;
    Result(String[] changedFields, sObject record1, sObject record2) {
        this.changedFields = changedFields;
        this.record1 = record1;
        this.record2 = record2;
    }
}
Result[] findChangedRecords(String[] fields, sObject[] firstRecordList, sObject[] secondRecordList) {
    Result[] results = new Result[0];
    for(Integer index = 0, size = firstRecordList.size(); index < size; index++) {
        sObject firstRecord = firstRecordList[index];
        sObject secondRecord = secondRecordList[index];
        Map<String, Object> oldValues = firstRecord.getPopulatedFieldsAsMap();
        Map<String, Object> newValues = secondRecord.getPopulatedFieldsAsMap();
        String[] changedFields = new String[0];
        for(String field: fields) {
            if(oldValues.get(field) != newValues.get(field)) {
                changedFields.add(field);
            }
        }
        if(changedFields.size() > 0) {
            results.add(new Result(changedFields, firstRecord, secondRecord));
        }
    }
    return results;
}

This has middle-ground performance (about 3-4x slower than comparing the maps directly, but about 3-4x faster than your original implementation), but identifies every field that violates your rules, allowing you to write something like:

Utils.Result[] changedCases = Utils.findChangedRecords(fieldsToCheck, Trigger.old, Trigger.new);
for(Utils.Result change: changedCases) {
  change.record2.addError('You cannot edit the following fields: '+String.join(change.changedFields,', '));
}

This utility method could be used for all kinds of purposes. Off the top of my head, you can: check which fields were edited in a Visualforce page, check for changes in triggers, use as a filter to avoid unnecessary DML operations (e.g. to avoid unwanted trigger recursion), etc.

Or, even better, you can now attach errors directly to a field:

Utils.Result[] changedCases = Utils.findChangedRecords(fieldsToCheck, Trigger.old, Trigger.new);
for(Utils.Result change: changedCases) {
  for(String field: change.changedFields) {
    change.record2.addError(
      field, 
      'You cannot edit this field. Expected value: ' + 
      change.record2.get(field)
    );
  }
}

This is particularly user friendly, as the user will know which field they edited, and what the original value was. You'll notice we're back to nested loops, and this is okay. This is how nested loops are supposed to be used.

Answer 2

If you are looking for code reusability:

Create these utility method in TriggerUtility.cls

public static Boolean fieldWasChanged(SObject newRecord, SObject oldRecord, String field) {
    return newRecord.get(field) != oldRecord.get(field);
}

public static Boolean fieldWasChangedAndNotEqualNull(SObject newRecord, SObject oldRecord, String sObjectField) {
    return newRecord.get(sObjectField) != oldRecord.get(sObjectField) && newRecord.get(sObjectField) != null;
}

But you cannot escape loops by any means for your use case. As earlier commented inner loops are not always bad. In your use case you have a defined set of values which inner loop will run on, so its not an issue.