0

As a beginner I'm currently learning how to bulkify my Apex Classes.

I wrote an Apex Class with the following logic:

When an Opportunity's stage is changed to Closed Won ...

1. Create an Asset for each Opportunity Line Item of the Opportunity and relate it to the respective account.

2. Set the Opportunity's description field to "Hello World".

NOTE: It's just an example for practicing. It's not a real business case.

The below code works fine and produces the desired outcome - also when multiple records are in the Trigger. But there's a certain smell: It comes from the nested loop.

In order to match each Opportunity Line Item with the respective Opportunity and Account I used an IF-statement and a nested loop. I took notice that nested loops are ineffective, may slow down the org and are enemies to the governor limits. I read several times that using Maps is the best way to escape nested loops.

public class opportunityTriggerHandler {    

public static void createAssets (Map<Id,Opportunity> myNewOpps, Map<Id,Opportunity> myOldOpps) {

    List<OpportunityLineItem> myOlis = [SELECT Id, Name, TotalPrice, Opportunity.AccountId, OpportunityId
                                        FROM   OpportunityLineItem
                                        WHERE  OpportunityId IN :myNewOpps.keySet()];

    // We create a list for our new Assets - we will add Assets into this list and insert it at the end
    List<Asset> myAssets = new List<Asset>();  

    // The logic in the loop applies to each Opportunity that enters our Trigger
    for (Opportunity myOpp : myNewOpps.values()) {

        // Only run the logic if the Stage is *CHANGED* to 'Closed Won'
        if (myOpp.StageName == 'Closed Won' && myNewOpps.get(myOpp.Id).StageName != myOldOpps.get(myOpp.Id).StageName) {

            // 1. Set the Opportunity's Description field to 'Hello World'
            myOpp.Description = 'Hello World';

            // 2. Create an Asset for each of the Opportunity Line Items associated with the Opportunity
            for (OpportunityLineItem myOli : myOlis ) {

                /* We need to make sure the logic only goes for the Opportunity Line Item associated with the 
                   current Opportunity in the Trigger - that's the purpose of the IF statement */

                if (myOli.OpportunityId == myOpp.Id ) {

                    Asset myAsset      = new Asset();
                    myAsset.Name       = myOli.Name;
                    myAsset.Price      = myOli.TotalPrice;
                    myAsset.AccountId  = myOpp.AccountId;
                    myAssets.add(myAsset);     

                }
            }
        }
    }  

    // We're inserting the list with the new Assets
    insert myAssets;

}

Question 1: Can someone give me a hint / tell me how to apply the power of Maps to my example?

Question 2: Just out of interest, does the only reference of the myOlis collection variable in the nested loop count towards the SOQL governor limit? The actual, "written" query is outside of the loop. I tested it in bulk and checked the debug logs - it says 1. Just want to make sure.

Any help is highly appreciated!

  • N.B. you need to consider the use case of an Oppo going from notCW -> CW -> notCW -> CW to avoid creating duplicate assets (or at least to clean up assets on reversion from CW) – cropredy Sep 30 at 21:51
1

Question 1: Can someone give me a hint / tell me how to apply the power of Maps to my example?

This skeleton is what you want to avoid:

for (Opportunity myOpp : myNewOpps.values()) {
        for (OpportunityLineItem myOli : myOlis ) {

            /* We need to make sure the logic only goes for the Opportunity Line Item associated with the 
               current Opportunity in the Trigger - that's the purpose of the IF statement */

            if (myOli.OpportunityId == myOpp.Id ) {

You never want to be iterating over one collection inside a loop that iterates over another collection to find the elements of Collection A that correspond to Collection B. This requires you to make N*M comparisons, for N in Collection A and M in Collection B; it's extremely inefficient.

Instead, the general pattern is to iterate over Collection B (here, myOlis) once and process it into a Map<Id, List<CollectionBObject>>, where the key is the Id of the parent object. That ensures that inside your

for (Opportunity myOpp : myNewOpps.values()) {

you can obtain and iterate over exactly the OLIs, the Collection B objects, that relate to the Opportunity, the Collection A object, you're working with.

This looks something like this:

Map<Id, List<ObjectB>> childMap = new Map<Id, List<ObjectB>>();

for (ObjectB b : objectBCollection) {
    Id parent = b.ParentId; // Let's assume for now it cannot be `null`.
    if (!childMap.containsKey(parent)) {
        childMap.put(parent, new List<ObjectB>();
    }
    childMap.get(parent).add(b);
}

Then, in your parent object loop, you loop only over related objects:

for (ObjectA a : objectACollection) {
    for (ObjectB b : childMap.get(a.Id)) {
        // do stuff.
    }
}

Some error-checking is omitted here; you'd want to validate that the parent a actually does have children, for example.

If the two objects are parent-child, as they are here, you may also choose to perform a parent-child SOQL query, so that your sObject data comes back from the query already "nested" in appropriate collections.

Question 2: Just out of interest, does the only reference of the myOlis collection variable in the nested loop count towards the SOQL governor limit? The actual, "written" query is outside of the loop. I tested it in bulk and checked the debug logs - it says 1. Just want to make sure.

You are running exactly one SOQL query and will be charged for one query against your governor limits.

The governor limit you should be more concerned about here is CPU time, due to the computational inefficiency of this pattern.

  • Thank you, David. This is already eye-opening and well-explained. But I think I need some more guidance. So we're starting off creating a Map: Map<Id, List<OpportunityLineItem>> oliMap = new Map<Id, List<OpportunityLineItem>>(); Then we're iterating over myOlis: for (OpportunityLineItem myOli : myOlis ) { Asset myAsset = new Asset(); myAsset.Name = myOli.Name; -- and so on --' } How exactly do I process the List<OpportunityLineItem> into the Map? – David Sep 30 at 20:31
  • If you do this as a parent-child subquery, the limits situation is a little more complex. For SOQL queries, it'll only show 1 query used. If you check Limits.getAggregateQueries() before and after, however, it'll also show that you used 1 "aggregate query". A query without a subquery doesn't consume any "aggregate" queries (which is a terrible name, really, because it's not the same as using an aggregate function or getting an aggregateResult). I believe the transaction limit is 300 "aggregate" queries. Can be useful in reducing "normal" SOQL queries used, just a bit of an extra wrinkle. – Derek F Sep 30 at 21:09
  • @David I've expanded with a sketch of what this pattern looks like. – David Reed Sep 30 at 21:35
  • @DavidReed Thanks for your help, David! One question before I go ahead and try it by myself: The second part of the pattern you posted shows again a nested loop. The difference to my initial nested loop is that we don't have an IF-statement and the list of object B only contains relevant records. Does it mean that nested loops are not completely forbidden? Is the question more about "good" nested loops and "bad" nested loops"? With regards to best practices, is it actually possible to write the above class without a nested loop? – David Sep 30 at 22:33
  • @David "Is the question more about "good" nested loops and "bad" nested loops"?": Yes. It's about computational complexity. As an illustration, see if you can figure out how many total iterations would take place in the first version and in the second version, for some count of Opps and OLIs. – David Reed Sep 30 at 22:34

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.