0

Is it a good practice to write a SOQL for each loop inside another SOQL foreach loop?

Can the below scenario can be considered as best practice or not?

public static void myMethod(Set<Id> oppOldIds , Set<Id> oppNewIds){
    for(Opportunity oppOld : [Select Id From Opportunity Where Id In: oppOldIds AND StageName != 'Closed Won']){
        for(Opportunity oppNew : [Select Id From Opportunity Where Id In: oppNewIds AND StageName = 'Closed Won']){
            if(oppOld.Id == oppNew.Id){
                taskToInsertList.add(new Task(WhatId = oppOld.Id, Subject = 'Follow Up Test Task'));
            }
        }
    }
}
  • 5
    it is a bad practice. Imagine , that first soql return you more, then 100 records, then you will hit number of SOQL query governor limit. – Oleksandr Berehovskyi Jun 19 '17 at 7:36
  • Thanks @AlexanderBerehovskiy I also wanted to know the mechanism behind the SOQL for each loop querry. Does a SOQL for each loop adds a single count to SOQL Query governor limit? – Prem Kumar Jun 20 '17 at 16:41
4

It is extremely easy for a query to return +100 records and in that case, you would hit a governor limit. Never include queries in loops.

And even if it weren't Salesforce, it is still a bad practices since a DB query in a loop can bottleneck your code.

See limits here: Apex Governor Limits

In any case, I recommend you check out some best practices that will help you make more robust and cleaner code: Apex best practices

2

A common mistake is that queries or DML statements are placed inside a for loop. There is a governor limit that enforces a maximum number of SOQL queries. There is another that enforces a maximum number of DML statements (insert, update, delete, undelete). When these operations are placed inside a for loop, database operations are invoked once per iteration of the loop making it very easy to reach these governor limits.

Instead, move any database operations outside of for loops. If you need to query, query once, retrieve all the necessary data in a single query, then iterate over the results. If you need to modify the data, batch up data into a list and invoke your DML once on that list of data.

Apex best practice : https://developer.salesforce.com/page/Apex_Code_Best_Practices

2

FOR THE LOVE OF THE FLYING SPAGHETTI MONSTER, DON'T DO THAT.

What you have boils down to SOQL inside a for loop and I'm pretty sure you know that's bad.

You can use the INTERSECTION operator that Set<T> has to remove one of the loops completely and also remove a comparison. Your function becomes:

public static void createFollowups(Set<Id> oppOldIds, Set<Id> oppNewIds){
    Id[] oppNewIdsSet = new List<Id>(oppNewIds);
    oppOldIds.retainAll(oppNewIdsSet); //apply intersection operator

    for (Opportunity opp : [SELECT Id FROM Opportunity WHERE Id IN: oppOldIds AND StageName != 'Closed Won']){
      taskToInsertList.add(new Task(WhatId = opp.Id, Subject = 'Follow Up Test Task'));
    }
}

(retainAll performs a intersection on a set)

1

Using a SOQL for loop syntax to iterate through a queried result set is a good option however, it would not be appropriate to do so in the code you have specified.

The reason being, your code, would result in execution of the second for loop for equal number of times for which the first for loop is executed, Considering the number of records returned in the first for loop is high i.e. greater than 200, it will cause your code to violate, the governor limits on the number of SOQL queries being executed in a single execution context. Hence a nested soql for loop is a bad option here.

Though you can modify the code as shown below to effectively use the SOQL for loop.

public static void myMethod(Set<Id> oppOldIds, Set<Id> oppNewIds){

    //Fetch the list of new OppIds first
    List<Opportunity> newClosedWonOppList = new List<Opportunity>();
    oldClosedWonOppList.addAll([Select Id From Opportunity Where Id In: oppNewIds AND StageName = 'Closed Won']);

    //Iterate on the old Opportunity records using a Soql for loop and then use that as a comaprison
    for(Opportunity oppOld :[Select Id From Opportunity Where Id In: oppOldIds AND StageName != 'Closed Won'])
        for(Opportunity oppNew : newClosedWonOppList){
            if(oppOld.Id == oppNew.Id){
                taskToInsertList.add(new Task(WhatId = oppOld.Id, Subject = 'Follow Up Test Task'));
            }
        }
    }
}

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.