1

is there a way to optimize this more?

 for(Program_Standard__c comp: comps) {

        List<CertificateComplianceAssociation__c> tempAsocs = [
                SELECT Id, Certificate_Type__c, Program_Standard__c
                FROM CertificateComplianceAssociation__c
                WHERE Program_Standard__c =: comp.Id
        ];

        List<Id> certIds = new List<Id>();
        for(CertificateComplianceAssociation__c tempAsoc: tempAsocs) {
            certIds.add(tempAsoc.Certificate_Type__c);
        }

        List<Certificate_Type__c> tempCerts = [
                SELECT Id, Training_Expiration_Date__c
                FROM Certificate_Type__c
                WHERE Id IN: certIds
        ];

        Date expDate = null;
        for(Certificate_Type__c tempCert: tempCerts) {

            if (expDate == null || expDate > tempCert.Training_Expiration_Date__c) {
                expDate = tempCert.Training_Expiration_Date__c;
            }
        }
        if(comp.Expiration_Date__c != expDate) {
            comp.Expiration_Date__c = expDate;
            comps2Update.add(comp);
        }
    }

    if(comps2update.size() > 0) { update comps2update; }
}
7

Yes, using SOQL in a loop can almost always be optimized, and this code is no exception. Do not ever put SOQL or DML in a loop, or you risk exceeding governor limits. There's plenty of naive solutions to this, but the most direct route is to simply ask the database to calculate the maximum date for you, which it can do quickly and efficiently. Here's an updated solution (not tested):

Map<Id, AggregateResult> results = 
    new Map<Id, AggregateResult>([
        SELECT MAX(Certificate_Type__r.Training_Expiration_Date__c) expDate, 
            Program_Standard__c Id
        FROM CertificateComplianceAssociation__c
        WHERE Program_Standard__c = :comps
        GROUP BY Program_Standard__c
    ]);
Program_Standard__c[] programStandardUpdates = new Program_Standard__c[0];
for(Program_Standard__c comp: comps) {
    Date maxDate = results.containsKey(comp.Id)? (Date)results.get(comp.Id).get('expDate'): null;
    if(maxDate != comp.Expiration_Date__c) {
        comp.Expiration_Date__c = maxDate;
        programStandardUpdates.add(comp);
    }
}
update programStandardUpdates;
  • I have updated it to these, is these correct right? :) thank you! – Cloud Jul 23 '18 at 15:24
  • apologies, have editted the code to what you have put, now they are updated. thank you or I miss something? – Cloud Jul 23 '18 at 16:05
  • Editing the answer to include code from the OP doesn't seem right...am I missing something here? – Adrian Larson Jul 23 '18 at 16:06
  • @AdrianLarson It shows as my edit, but the OP edited in the code originally. – sfdcfox Jul 23 '18 at 16:08
  • Right but why did you not just reject their edit? That's where I am most confused. – Adrian Larson Jul 23 '18 at 16:09
5

sfdcfox has a better solution for you to achieve your specific objective here. As a more general commentary on how to bulkify this code structure:

You've already used the tool you need to bulkify this properly and remove your SOQL from the loop, but you haven't applied it fully to your code.

    List<Id> certIds = new List<Id>();
    for(CertificateComplianceAssociation__c tempAsoc: tempAsocs) {
        certIds.add(tempAsoc.Certificate_Type__c);
    }

    List<Certificate_Type__c> tempCerts = [
            SELECT Id, Training_Expiration_Date__c
            FROM Certificate_Type__c
            WHERE Id IN: certIds
    ];

That's the right pattern to use - accumulate a Set<Id> of the parent object and then issue one query against the child -, but you need to use it outside your for loop, and you need to do it twice. First you'll iterate and query CertificateComplianceAssociation__c, and then you'll iterate and query Certificate_Type__c.

Your method should use exactly two SOQL queries regardless of input data volume. (Although you can get it down to one if your first second Id set is empty).

You also don't need to do this:

if(comps2update.size() > 0) { update comps2update; }

because update on an empty list is a no-op. Writing this out on a single line suggests to me an attempt to game the code coverage metrics, so I'll note that you may also need to enhance your unit test to result in real action that you can make assertions against.

  • 1
    I got it down to theoretically one query :D – sfdcfox Jul 17 '18 at 15:09
  • I did not read the logic carefully enough to see it's actually just an aggregate! – David Reed Jul 17 '18 at 15:09

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.