Reducing loop runs and code repetition in a trigger where junction records are created which match several criteria

Question

I'm new to Apex and coding in general and trying to get the best practices down where possible. This trigger challenge came from David Liu's Apex Academy series and gives the following requirements:

When an Opportunity is created, attach similar deals that have:

• A matching account Industry
• "Closed Won" within the past year

A junction object named "Comparable" is created for the challenge, for which records are inserted which will take the trigger's base Opportunity ID, and the ID of an Opportunity that matches the above criteria. After the new Opportunity is created, you will then be able to see a related list of comparable Opportunities which meet that criteria.

I had no problem executing this trigger with the same methodology as in David's video, but he uses SOQL within the Trigger.new loop. This is noted but the alternative solution isn't provided.

This is what I came up with for my solution to removing SOQL from the loop:

trigger ComparableOpportunitiesTrigger on Opportunity (after insert) {

    //Instantiate set to gather Account Industry data from trigger records

    Set<String> oppIndustries  = new Set<String>();
    for (Opportunity opp : [SELECT Id, Account.Industry FROM Opportunity WHERE Id IN :Trigger.newMap.keySet()]) {     
        oppIndustries.add(opp.Account.Industry);
    }
    
    //Use SOQL to find matching opportunities and add to matching list

    List<Opportunity> matchingOpps = [SELECT Id,
                                             Account.Industry,
                                             StageName,
                                             Amount,
                                             CloseDate
                                        FROM Opportunity
                                       WHERE Id            NOT IN :Trigger.newMap.keyset() //Prevents the new Opps being added to their own comparables list
                                         AND (Account.Industry IN :oppIndustries
                                          OR (StageName         = 'Closed Won' 
                                         AND CloseDate>= N_YEARS_AGO:1))];
    
    //Instantiate Comparables (junction object) list to insert later

    List<Comparable__c> comparables = new List<Comparable__c>();
    
    //Iterate through trigger's new Opportunities and for each opp, find matches to the SOQL-derived list

    for (Opportunity opp : Trigger.new) {
        
        for (Opportunity matchingOpp : matchingOpps) {
            
            //Same criteria is repeated from earlier SOQL query to match Opportunities

            if (  opp.Account.Industry         == matchingOpp.Account.Industry //this line won't work but please ignore for purposes of this question
               && !String.isBlank(opp.Account.Industry) 
               || matchingOpp.StageName        == 'Closed Won'
               && matchingOpp.CloseDate        >= Date.today()-365) {
                
                //Create Comparable junction record with the Trigger Opportunity's base ID and the matching Opportunity's ID

                Comparable__c comparable = new Comparable__c(Base_Opportunity__c = opp.Id,
                                                             Comparable_Opportunity__c = matchingOpp.Id);

                comparables.add(comparable);
            }
        }
    }
    
    insert comparables;
}

It works as intended, but it seems off to me for two reasons:

• I have to repeat the filter criteria from the SOQL query in a nested loop, to match records across the two lists of Opportunities.
• I'm using a nested for each loop, which theoretically could run thousands of times depending on how many records are inserted, and how many matches are found.

Is there a way to streamline this code and make it more efficient? I figure a nested for loop will be necessary since junction records must be created, but if it can only loop as many times as there is matches it would surely be far more optimal. I'm very new to this so can't yet perceive a way to improve it.

The challenge has one additional criteria but I'll leave that for another post.

I'm very thankful for anyone's time and help.

Answer 1

@cropredy Thanks for your advice, I gave this a shot according to your instructions and it still works as intended. The final block should only run as many times as there is records to create using the maps. It also seemed logical to break the SOQL query into two.

trigger ComparableOpportunitiesTrigger on Opportunity (after insert) {

    //Instantiate list to gather Account Industry data records
    Map<Id, String> newOppMap  = new Map<Id, String>();
    Map<String, List<Opportunity>> matchingIndustryMap = new Map<String, List<Opportunity>>();
        
    for (Opportunity opp : [SELECT Id, Account.Industry FROM Opportunity WHERE Id IN :Trigger.newMap.keySet()]) {     
        List<Opportunity> opps = new List<Opportunity>();
        if(opp.Account.Industry != null) {
            matchingIndustryMap.put(opp.Account.Industry, opps); // get industry keys for map that will have list of opps for each industry key
        }    
        newOppMap.put(opp.Id, opp.Account.Industry);         // used in place trigger.new since you can't grab Account.Industry 
    }
    
    //Add any opps with matching industries to the trigger opps to a list - we'll create a map that pivots by industry to a list of opps
    List<Opportunity> matchingIndustryOpps = [SELECT Id,
                                                     Account.Industry,
                                                     StageName
                                                FROM Opportunity
                                               WHERE Id           NOT IN :Trigger.newMap.keySet() //Prevents the new Opps being added to their own comparables list
                                                 AND Account.Industry IN :matchingIndustryMap.keySet()];
    //Add any closed-won opps to a list - these will be comparables on all trigger opps
    List<Opportunity> closedWonOpps =        [SELECT Id
                                                FROM Opportunity
                                               WHERE Id      NOT IN :Trigger.newMap.keySet() //Prevents the new Opps being added to their own comparables list
                                                 AND StageName   = 'Closed Won' 
                                                 AND CloseDate  >= N_YEARS_AGO:1];
    
    //Finalise map of industries mapped to list of opps belonging to that same industry
    for (Opportunity opp : matchingIndustryOpps) {
        if (opp.StageName != 'Closed Won') { //Prevents Closed Won opps being added to two lists
                matchingIndustryMap.get(opp.Account.Industry).add(opp);
        }
    }
    
    //Instantiate comparables list to insert later
    List<Comparable__c> comparables = new List<Comparable__c>();
    
    //This new loop set will only run as many times as junction records need to be created
    for (Id oppId : newOppMap.keySet()) {
        //Create comparable records for Industry matches
        if (newOppMap.get(oppId) != null) {
            for (Opportunity matchingOpp : matchingIndustryMap.get(newOppMap.get(oppId))) { //Seeks out opp list from map according to matched industry
            Comparable__c c = new Comparable__c(Base_Opportunity__c = oppId, Comparable_Opportunity__c = matchingOpp.Id);
            comparables.add(c);
            }
        }
        //Create comparable records for Closed Won opps
        for (Opportunity closedWonOpp : closedWonOpps) {
            Comparable__c c = new Comparable__c(Base_Opportunity__c = oppId, Comparable_Opportunity__c = closedWonOpp.Id);
            comparables.add(c);
        }
    }
    
    insert comparables;
}

I'm not sure if I've 'pivoted' the maps using the best approach as per your comment so would love to hear if there's any further way this can be improved.