+ Start a Discussion
duke1duke1 

Can someone explain how to best avoid using SOQL statements in a for loop in a trigger?

The essential information is:

 

Obect 1 has a master-detail relationship with Account , but is one-to-one with Account.

Object 1 also has a master-detail relationship with Opportunity that is one-to-many, (many Object1's to one Opportunity).

Each Opportunity has fields that need to be acted on in a for loop and then Object 1 is updated after that loop.

 

The original trigger essentially does the following:

 

create Set accountIDs from trigger.new

List<Object1> objList = [select Account__c ... from sObject1 where Account__c in :accountIDs]

for (Object1 acc : accountIDs) {

   // some code

    List<Opportunity> oppList = [select AccountID ... from sObject1 where AccountID = :accountIDs]

  // some more code

 change fields in objList

}  // end for

upsert objList

 

I need to take the SOQL statement out of the for loop, so the revised trigger does this:

 

create Set accountIDs from trigger.new

List<Object1> objList = [select Account__c ... from sObject1 where Account__c in :accountIDs]

// next line uses "in :accountIDs" instead of "= :accountIDs"

List<Opportunity> oppList = [select AccountID ... from sObject1 where AccountID in :accountIDs]

for (Object1 acc : accountIDs) {

   // some code

   for (Opportunity opp : oppList) {

       if (opp.AccountID == acc.Id) {

       // some code

       } // end if

   } // end inner for

  // some more code

 change fields in objList

}  // end outer for

upsert objList

 

The problem with the structure of the second trigger is that I am iterating over all the Opportunities every time, to find the relevant Opportunities with AccountID = acc.Id, which seems terribly inefficient.  I considered removing the "used" Opportunites after they have been processed, but then I'm still iterating over many irrelevant Opportunities on each loop.

 

There must be a better solution.  Can anyone help?  Thanks.

Best Answer chosen by Admin (Salesforce Developers) 
WesNolte__cWesNolte__c

Sure, it could be more efficient. Consider this for example:

 

 

Map<String id, List<Opportunity> oppMap = new Map<String Id, List<Opportunity>>();

for(Opportunity o : [select AccountID ... from sObject1 where AccountID in :accountIDs]){
List<Opportunity> oppList;
if(oppMap.contains(o.AccountId)){
  oppList = oppMap.get(o.AccountId);
  oppList.add(o);
}else{
  oppList = new List<Opportunity>()
  oppList.add(o);
}

oppMap.put(o.AccountId, oppList);

}

 You could then pull each list out of the Map as required. I'm not sure what gains you'd get, but it might be considered less sloppy.

 

Wes

 

All Answers

WesNolte__cWesNolte__c

Hey

 

In the second piece of code you're iterating over the same number of opportunities, but without having to fetch anythign from the DB so it's more efficient. Both sections of code filter with the same criteria.

 

Wes

duke1duke1

But from a coding point of view, it seems sloppy.  There's got to be a more efficient way to code this.

WesNolte__cWesNolte__c

Sure, it could be more efficient. Consider this for example:

 

 

Map<String id, List<Opportunity> oppMap = new Map<String Id, List<Opportunity>>();

for(Opportunity o : [select AccountID ... from sObject1 where AccountID in :accountIDs]){
List<Opportunity> oppList;
if(oppMap.contains(o.AccountId)){
  oppList = oppMap.get(o.AccountId);
  oppList.add(o);
}else{
  oppList = new List<Opportunity>()
  oppList.add(o);
}

oppMap.put(o.AccountId, oppList);

}

 You could then pull each list out of the Map as required. I'm not sure what gains you'd get, but it might be considered less sloppy.

 

Wes

 

This was selected as the best answer
duke1duke1

Thanks, I like your solution.