0
trigger oppTrigger on Opportunity (after insert,after update) {
    Set<Id> accIds = new Set<Id>();
    Set<Id> oppIds = new Set<Id>();
    List<Account> accToAdd = new List<Account>();
    for(Opportunity opp:[SELECT Id, StageName, Account.Id FROM Opportunity WHERE Id in :Trigger.New 
                         AND StageName = 'Closed Won']){
            accIds.add(opp.Account.Id);
            oppIds.add(opp.Id);
    }
    List<Account> accList = [SELECT Name, Total_Amount__c, (SELECT Id, Amount FROM Opportunities WHERE 
                           Id IN :oppIds) FROM Account WHERE Id IN :accIds];
    For(Account a :accList){
        Integer totAmt = 0;
        for(Opportunity op :a.Opportunities){
            if(Trigger.oldMap.get(op.id).stageName != 'Closed Won'){
                 totAmt += (Integer)op.Amount;
            }
        }
        a.Total_Amount__c = a.Total_Amount__c == null ? totAmt : a.Total_Amount__c + totAmt;
        accToAdd.add(a);
    }
    update accToAdd;
}
2
  • 2
    This is perfectly OK since there isn't a search in the inner loop for a match against the outer loop. You might consider a roll up formula instead?
    – Phil W
    Commented Apr 3, 2022 at 11:43
  • Thank you @PhilW Commented Apr 3, 2022 at 15:06

1 Answer 1

3

As I've said before, nested loops are only a problem when you specifically have the following design:

sObject[] parents, children;
parents = getParents();
children = getChildren();
for(sObject parent: parents) {
  for(sObject child: children) {
    if(child.get(childParentField) == parent.Id) {
       // Do something
    }
  }
}

When the nested loop doesn't have that type of if statement, it is almost certainly acceptable.

However, in your specific case, there's a much easier way to summarize the data, known as aggregate queries. This follows the standard Aggregate-Query-Update Pattern, as I call it. That looks like this:

trigger rollupAccounts on Opportunity (after insert, after update, after delete, after undelete) {
  Map<Id, Account> accounts = new Map<Id, Account>();
  // Aggregate
  if(Trigger.new != null) {
    for(Opportunity record: Trigger.new) {
      accounts.put(record.AccountId, new Account(Total_Amount__c=null));
    }
  }
  if(Trigger.old != null) {
    for(Opportunity record: Trigger.old) {
      accounts.put(record.AccountId, new Account(Total_Amount__c=null));
    }
  }
  accounts.remove(null);
  // Query summary information
  for(AggregateResult sum: [
      SELECT SUM(Amount) sum, AccountId Id 
      FROM Opportunity 
      WHERE IsWon = TRUE AND AccountId = :accounts.keySet()]) {
    accounts.get((Id)sum.get('Id')).Total_Amount__c = (Decimal)sum.get('sum');
  }
  // Update the account records
  update accounts.values();
}

And, as Phil W said, you might want to just use existing declarative functionality instead. A Rollup Summary Field can do this for you automatically with exactly zero code and no maintenance. Write a trigger only if you need to.

Your code had a couple of other bugs, too, such as not supporting updating the totals when records are deleted/undeleted and truncating the decimal part (fractions of a currency unit) by casting to Integer. Using a Rollup Summary field or an aggregate query design on all four DML operations, as demonstrated in my code, avoids these problems.

1
  • Thanks @sfdcfox , now I got it. Commented Apr 3, 2022 at 15:04

You must log in to answer this question.

Not the answer you're looking for? Browse other questions tagged .