How to avoid for loop inside a for loop?

Question

I am facing CPU Run time exception while update opportunity through Data Loader. Because of my rollup trigger. How to avoid for loop inside a for loop?

Trigger:

trigger BudgetAllocationRollup on Budget_Allocation__c (after delete, after insert, after update) {

    Set<Id> Opportunityids=new Set<Id>();
    Map<id,Opportunity__c> mapOpportunity=new Map<id,Opportunity__c>();
    Map<id,List<Budget_Allocation__c>>  mapBudget=new Map<id,List<Budget_Allocation__c>>();
     if(Trigger.isInsert || Trigger.isUpdate){
        List<Budget_Allocation__c> lstBudget=[select id,name,Budget__c,Opportunity1__c ,Opportunity_Flag__c from Budget_Allocation__c where id in:Trigger.newMap.keyset()];
        for(Budget_Allocation__c budget:lstBudget){
             Opportunityids.add(budget.Opportunity1__c );
        }
        List<Opportunity__c> opplst=[select id,name,Budget_Allocation_Total__c,(select Opportunity1__c ,id,Budget__c,name,Opportunity_Flag__c from Budget_Allocations__r) from Opportunity__c where id in:Opportunityids];
        for(Opportunity__c opp:opplst){
            mapOpportunity.put(opp.id,opp);
            mapBudget.put(opp.id,opp.Budget_Allocations__r);
        }
        AggregateResult i=[select sum(Budget__c) from Budget_Allocation__c where Opportunity1__c in:mapOpportunity.keyset()];
        for(Id id:mapOpportunity.keyset()){

                mapOpportunity.get(id).Budget_Allocation_Total__c=0;
                decimal total=0;
                for(Budget_Allocation__c budget: mapBudget.get(id)) {
                    if(budget.Budget__c!=null && budget.Opportunity_Flag__c==true)

                        mapOpportunity.get(id).Budget_Allocation_Total__c=mapOpportunity.get(id).Budget_Allocation_Total__c+budget.Budget__c;
                }   

        }
        if(mapOpportunity!=null && mapOpportunity.size()>0){
          try{
                update mapOpportunity.values();
          }
          catch(Exception e){

          }
        }
     }
    if(Trigger.isDelete){

            List<Budget_Allocation__c> lstBudget=[select id,name,Budget__c,Opportunity1__c ,Opportunity_Flag__c from Budget_Allocation__c where id in:Trigger.oldMap.keyset()];
            //system.debug('--lstBudget--->'+lstBudget); 
            if(lstBudget!=null){
            for(Budget_Allocation__c budget:lstBudget){
                 Opportunityids.add(budget.Opportunity1__c );
            }
       }
            List<Opportunity__c> opplst=[select id,name,Budget_Allocation_Total__c,(select Opportunity1__c ,id,Budget__c,name from Budget_Allocations__r) from Opportunity__c where id not in:Opportunityids];
                for(Opportunity__c opp:opplst){
                    mapOpportunity.put(opp.id,opp);
                     mapBudget.put(opp.id,opp.Budget_Allocations__r);
                }
            AggregateResult i=[select sum(Budget__c) from Budget_Allocation__c where Opportunity1__c in:mapOpportunity.keyset()];
            for(Id id:mapOpportunity.keyset()){
                mapOpportunity.get(id).Budget_Allocation_Total__c=0;
                decimal total=0;
                for(Budget_Allocation__c budget: mapBudget.get(id)) {
                    if(budget.Budget__c!=null)

                        mapOpportunity.get(id).Budget_Allocation_Total__c=mapOpportunity.get(id).Budget_Allocation_Total__c+budget.Budget__c;
                }   

            }
            if(mapOpportunity!=null && mapOpportunity.size()>0){
              try{
                    update mapOpportunity.values();
              }
              catch(Exception e){

              }
            }
     }

}

Answer 1

For inside FOR loop is not causing this issue as there is no SOQL inside FOR loop.

There are couple of things i thought will reduce your CPU times

1)Use Inline SOQL rather than looping over the list.This consumes less CPU times

The below consumes more CPU time since the entire list is queried and iterated

List<Budget_Allocation__c> lstBudget=[select id,name,Budget__c,Opportunity1__c ,Opportunity_Flag__c from Budget_Allocation__c where id in:Trigger.newMap.keyset()];
    for(Budget_Allocation__c budget:lstBudget){
         Opportunityids.add(budget.Opportunity1__c );
    }

Inline SOQL reduces the CPU time as well as Heap

//add inline SOQL
    for(Budget_Allocation__c budget:[select id,name,Budget__c,Opportunity1__c ,Opportunity_Flag__c from Budget_Allocation__c where id in:Trigger.newMap.keyset()]){
         Opportunityids.add(budget.Opportunity1__c );
    }

2)Your aggregate SOQL is no where used so i have removed the same from trigger .

Below is revised trigger with my comment added wherever modified

trigger BudgetAllocationRollup on Budget_Allocation__c (after delete, after insert, after update) {

Set<Id> Opportunityids=new Set<Id>();
Map<id,Opportunity__c> mapOpportunity=new Map<id,Opportunity__c>();
Map<id,List<Budget_Allocation__c>>  mapBudget=new Map<id,List<Budget_Allocation__c>>();
 if(Trigger.isInsert || Trigger.isUpdate){
  //add inline SOQL
    for(Budget_Allocation__c budget:[select id,name,Budget__c,Opportunity1__c ,Opportunity_Flag__c from Budget_Allocation__c where id in:Trigger.newMap.keyset()]){
         Opportunityids.add(budget.Opportunity1__c );
    }
    //add inline SOQL
    for(Opportunity__c opp:[select id,name,Budget_Allocation_Total__c,(select Opportunity1__c ,id,Budget__c,name,Opportunity_Flag__c from Budget_Allocations__r) from Opportunity__c where id in:Opportunityids]){
        mapOpportunity.put(opp.id,opp);
        mapBudget.put(opp.id,opp.Budget_Allocations__r);
    }
    //Removed Aggregate SOQL
    for(Id id:mapOpportunity.keyset()){

            mapOpportunity.get(id).Budget_Allocation_Total__c=0;
            decimal total=0;
            for(Budget_Allocation__c budget: mapBudget.get(id)) {
                if(budget.Budget__c!=null && budget.Opportunity_Flag__c==true)

                    mapOpportunity.get(id).Budget_Allocation_Total__c=mapOpportunity.get(id).Budget_Allocation_Total__c+budget.Budget__c;
            }   

    }
   // if(mapOpportunity!=null && mapOpportunity.size()>0){
      try{
            update mapOpportunity.values();
      }
      catch(Exception e){

      }
    //}
 }
if(Trigger.isDelete){
  //add inline SOQL
        for(Budget_Allocation__c budget:[select id,name,Budget__c,Opportunity1__c ,Opportunity_Flag__c from Budget_Allocation__c where id in:Trigger.oldMap.keyset()]){
             Opportunityids.add(budget.Opportunity1__c );
        }
        //add inline SOQL
        for(Opportunity__c opp:[select id,name,Budget_Allocation_Total__c,(select Opportunity1__c ,id,Budget__c,name from Budget_Allocations__r) from Opportunity__c where id not in:Opportunityids]){
            mapOpportunity.put(opp.id,opp);
             mapBudget.put(opp.id,opp.Budget_Allocations__r);
        }
         //Removed Aggregate SOQL
        for(Id id:mapOpportunity.keyset()){
            mapOpportunity.get(id).Budget_Allocation_Total__c=0;
            decimal total=0;
            for(Budget_Allocation__c budget: mapBudget.get(id)) {
                if(budget.Budget__c!=null)

                    mapOpportunity.get(id).Budget_Allocation_Total__c=mapOpportunity.get(id).Budget_Allocation_Total__c+budget.Budget__c;
            }   

        }
      //  if(mapOpportunity!=null && mapOpportunity.size()>0){
          try{
                update mapOpportunity.values();
          }
          catch(Exception e){

          }
    //    }
   }

}

Answer 2

Use following technique to reduce For loop. and it is used to reduce dependent queries in For loop. PS> This is for general concept, you can use it according to your logic, as you didn't mentioned what you are trying to achieve.

Technique 1: To Reduce for loops

Map<id,Opportunity__c> mapOpportunity=new Map<id,Opportunity__c>();
//^ this can be write as below, as you are not having conditions
Map<Id, Opportunity__c> mapOpportunity = new Map<Id, Opportunity__c>([select id,name,Budget_Allocation_Total__c,(select Opportunity1__c ,id,Budget__c,name,Opportunity_Flag__c from Budget_Allocations__r) from Opportunity__c where id in:Opportunityids]);

Technique 2:

Suppose your query is dependent on another object's id.,,,

Let's see the problem first:

for(objectDemo oTemp : listDemo)
{
   objectDemo1 oDemo1Temp = [select id from objectDemo1 where id =: oTemp.id];
   //processing of objectDemo1
}

Proposed Solution:

//taking list of objectDemo1 so that we don't have to repeatedly call it
Map<Id, objectDemo1> mapDemo1 = new Map<Id, objectDemo1>([select id from objectDemo1]);
for( objectDemo oTemp: listDemo)
{
  objectDemo1 oDemo1Temp = mapDemo1.get(oTemp.id);
  //processing of objectDemo1
}