Can I remove this for loop within a for loop?

Question

The following code is called by an after update trigger on Account. It finds all an account's children Account_Configuration__c records (child name for SOQL is Customer_Credentials__r) and -- if the account's Satisfaction_Comments__c field is updated -- it updates all those Account_Configuration__c records' Satisfaction_Comments__c fields.

Now, it doesn't just update the field to be the same as the account's. Rather, the code takes an Account_Configuration__c record's Satisfaction_Comments__c field (Long Text Area) and adds the account's value to what's already there. The final result for that field is a series of the following: Date the account Satisfaction Comment was made appended to the actual value of that field, each on its own line.

The code works, but it involves a for loop in a for loop. Is it possible to remove that nesting?

Here's the code:

public void updateProdImpsWithAccHis(Account[] updatedAccounts, Map<Id, Account> AccountOldMap) {   
    set<Id> accIds = new Set<Id>();
    for(Account updatedAccount : updatedAccounts){
        accIds.add(updatedAccount.Id);
    }

    List<Account> accList = [SELECT Id,Satisfaction_Comments__c,Acct_Sat_Comments_Date__c,LastModifiedDate,(SELECT Id,Satisfaction_Comments__c FROM Customer_Credentials__r) FROM Account WHERE Id IN :accIds];
    List<Account_Configuration__c> prodImpUpdates = new List<Account_Configuration__c>();
    for(Account acc : accList){
        if(acc.Satisfaction_Comments__c != AccountOldMap.get(acc.Id).Satisfaction_Comments__c){
            for(Account_Configuration__c pi: acc.Customer_Credentials__r) {
                if(pi.Satisfaction_Comments__c == null){
                    pi.Satisfaction_Comments__c = (acc.LastModifiedDate).addHours(-5) + ':  ' + acc.Satisfaction_Comments__c;   
                } else {
                    pi.Satisfaction_Comments__c = (acc.LastModifiedDate).addHours(-5) + ':  ' + acc.Satisfaction_Comments__c + '\n' + pi.Satisfaction_Comments__c;
                }   

                prodImpUpdates.add(pi);
            }
        }    
    }

    if(!prodImpUpdates.isEmpty()){
        update prodImpUpdates;
    }
}

Answer 1

The only time you need to avoid a for loop within a for loop is when it results in a multiplicative effect.

Bad Idea

Account[] accounts = [SELECT Name FROM Account];
Contact[] contacts = [SELECT Name, AccountId FROM Contact];
for(Account aRecord: accounts) {
  for(Contact cRecord: contacts) {
    if(aRecord.Id == cRecord.AccountId) {
      // Do something
    }
  }
}

Good Idea

Account[] accounts = [SELECT (SELECT Name FROM Contacts) FROM Account];
for(Account aRecord: accounts) {
  for(Contact cRecord: aRecord.Contacts) {
    // Do something
  }
}

As long as you're iterating over only the relevant children instead all possible children, you're doing the loop-within-a-loop correctly. There are plenty of legitimate use cases where you'd use a loop within a loop, especially when talking about processing a parent child and its children.

Answer 2

Your pattern does not appear to present any serious problems so far as efficiency or governor limits. However, you can indeed flatten the structure. The basic change you need to make is to filter first, then query the child records directly. In general your code will be much easier to test if you break out your filters and actions into separate methods, and compose them in your handler. Using this composition to flatten out your structure is a bit of a side benefit.

public static List<MyObject> haveCommentsChanged(List<MyObject> records, Map<Id, MyObject> oldMap)
{
    // implementation
}
public static void myMethod(List<MyObject> changedRecords)
{
    List<ChildObject> children = [
        SELECT ... FROM ChildObject WHERE LookupField IN :changedRecords
    ];
    Map<Id, MyObject> parentMap = new Map<Id, MyObject>(changedRecords);
    for (ChildObject child : children)
    {
        // manipulate records
    }
    // advisable to add DML handling as well
    update children;
}

Then your handler composition would look like:

MyObjectService.doStuf(MyObjectService.filter(records, oldMap));