2

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;
    }
}
  • You'll probably be fine. You don't have any queries, or DML statements in your loops, just a few if's and some assignments. If you're really worried, try stress testing it with a huge amount of records, but honestly, it's unlikely you'll figure out a more effective way to set values on child records of an object. Could be worth setting up a formula field or a workflow to provide the text formatting, instead of writing your own code tho. – battery.cord Nov 17 '17 at 19:03
  • Yeah this structure does not seem to present any efficiency or governor problems. – Adrian Larson Nov 17 '17 at 19:04
  • 1
    I didn't notice that you're adding to the long text value (which explains why you aren't using other methods). You might hit a heap limit if the text gets REALLY large, so maybe try testing 200 accounts with full text fields (or close to full - since it looks like you don't have a method of checking to see if the text field has space left), and see how long/how well the code runs. – battery.cord Nov 17 '17 at 19:05
  • if(!prodImpUpdates.isEmpty()){ update prodImpUpdates; } is the same as update prodImpUpdates;, but requires more code and CPU time. – sfdcfox Nov 17 '17 at 19:14
5

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.

|improve this answer|||||
5

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));
|improve this answer|||||
  • That's really interesting. I definitely want to adopt this approach, but I don't yet fully understand it. I wrote a new method that finds relevantly changed records. I then rewrote the original method to query just for the children. Where I'm stuck is somehow connecting the two. My guess is that probably happens in the handler composition you described, but I'm not sure what that is, where it goes, or how to make it fit my example. Also, what is the map for in your proposed code? It's empty, so how would you populate and then use it? Thanks – Gabriel Rivera Nov 18 '17 at 13:20
  • Ok, I experimented with it further and came up with this in my handler's AfterUpdate method, which worked: list<Account> accList = Account_TriggerHandler.haveCommentsChanged( updatedAccounts, AccountOldMap ); updateProdImpsWithAccHis( accList ); Is that essentially what you meant? If so, that just leaves the question of the map. In my children-query method, I picked the first record out of the list (e.g. children.get(0) ) and used the lookup to account to get to the desired value. It worked, but still doesn't seem right. I'm guessing your map is somehow alternatively intended. – Gabriel Rivera Nov 18 '17 at 13:56
  • @Elwin I fixed how the map construction is written in my answer. I guess you could pass in a filtered version of newMap, but that honestly seems more complicated and map construction is quite fast using this built in constructor. – Adrian Larson Nov 18 '17 at 14:58

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.