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;
}
-
2This 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 WCommented Apr 3, 2022 at 11:43
-
Thank you @PhilW– SAURABH GOSWAMICommented Apr 3, 2022 at 15:06
1 Answer
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.