function readOnly(count){ }
Sign Up ›
Starting November 20, the site will be set to read-only. On December 4, 2023,
forum discussions will move to the Trailblazer Community.
Learn More

Browse by Topic
ShowAll Questionssorted byDate Posted
+ Start a Discussion
sfdc-admin-smsfdc-admin-sm 

Please critique my trigger

Hello,

I wrote the trigger code below and it works fine. However I feel it can be optimized better to follow best practices. Can you please review it and let me know what enhancements I could have used?

I know 1 best practice is that I shouldn't write all this logic in Trigger but instead use a helper class and let the Trigger call that help class. Any other improvements? Please provide code samples if possible. I am trying to learn.

Thanks in advance... 
//Trigger that updates the AnnualAmount Account field based on the most recent Opportunity's AnnualAmount associated with the Account where the Opportunity Owner is not the test user and //AnnualAmount is > $0.

trigger UpdateAnnualAmountAcct on Opportunity (after insert, after update) {
    
    list<Opportunity> lstOpp = new list<Opportunity>();
    
    for(Opportunity o : Trigger.new){
        if(o.AnnualAmount__c > 0 && o.OwnerId != '00580000003mBZb'){ //id of test user
        lstOpp.add(o);
        }     
    }
    
    list<Opportunity> mostRecentOppty = new list<Opportunity>();
    
    if(lstOpp != NULL){
        mostRecentOppty = [SELECT Id, CloseDate, AnnualAmount__c, AccountId FROM Opportunity WHERE Id IN : lstOpp ORDER BY CloseDate DESC LIMIT 1];       
    }
    
    list<Account> acctToUpdate = new list<Account>();
    
    for(Opportunity o : lstOpp){
        Account a = new Account();
        a.Id = o.AccountId;
        a.AnnualAmount__c = o.AnnualAmount__c;
        acctToUpdate.add(a);
    }
    update acctToUpdate;
}

 
Best Answer chosen by sfdc-admin-sm
pconpcon
I do not think your trigger does what you expect it to.  To start with, you are never using the data you have in your mostRecentOppty query.  That query also will only return one row regardless of the number of opportunities you are updating/inserting.  Additionally, you will not get the most recent one for the account, only the ones you are acting on.  I have taken the liberty to update the trigger to do what you are intending to do.  Please let me know if there is any of this that does not make sense to you.
  
trigger UpdateAnnualAmountAcct on Opportunity (after insert, after update) {
    // I'd really recomend against doing this.  There should be no reason to ignore
    //   test users since the data in test classes is removed at the end of test run
    //   and you should be using a sandbox to test other things in, not production
    static final Id TEST_USER_ID = '00580000003mBZb';

    Set<Id> accountIds = new Set<Id>();

    for (Opportunity o: Trigger.new){
        if (o.AnnualAmount__c > 0 && o.OwnerId != TEST_USER_ID) {
            accountIds.add(o.AccountId);
        }  
    }

    if (!accountIds.isEmpty()) {
        Map<Id, Opportunity> accountToOppMap = new Map<Id, Opportunity>();

        Map<Id, Account> accountMap = new Map<Id, Account>([
            select AnnualAmount__c
            from Account
            where Id in :accountIds
        ]);

        for (Opportunity opp: [
            select AccountId,
                AnnualAmount__c,
                ClosedDate
            from Opportunity
            where AccountId in :accountIds and
                IsClosed = true and
                AnnualAmount__c
            order by ClosedDate
        ]) {
            if (!accountToOppMap.containsKey(opp.AccountId)) {
                accountToOppMap.put(opp.AccountId, opp);
            } else {
                if (accountToOppMap.get(opp.AccountId).ClosedDate > opp.ClosedDate) {
                    accountToOppMap.put(opp.AccountId, opp);
                }  
            }
        }   

        List<Account> accountsToUpdate = new List<Account>();

        for (Id id: accountIds) {
            Account account = accountMap.remove(id);
            Opportunity opp = accountToOppMap.remove(id);
            if (account.AnnualAmount__c != opp.AnnualAmount__c) {
                account.AnnualAmount__c = opp.AnnualAmount__c;
                accountsToUpdate.add(account);
            }
        }
        
        if (!accountsToUpdate.isEmpty()) {
            update accountsToUpdate;
        }
    }
}

NOTE: This code has not been tested and may contain typographical or logical errors.

All Answers

pconpcon
I do not think your trigger does what you expect it to.  To start with, you are never using the data you have in your mostRecentOppty query.  That query also will only return one row regardless of the number of opportunities you are updating/inserting.  Additionally, you will not get the most recent one for the account, only the ones you are acting on.  I have taken the liberty to update the trigger to do what you are intending to do.  Please let me know if there is any of this that does not make sense to you.
  
trigger UpdateAnnualAmountAcct on Opportunity (after insert, after update) {
    // I'd really recomend against doing this.  There should be no reason to ignore
    //   test users since the data in test classes is removed at the end of test run
    //   and you should be using a sandbox to test other things in, not production
    static final Id TEST_USER_ID = '00580000003mBZb';

    Set<Id> accountIds = new Set<Id>();

    for (Opportunity o: Trigger.new){
        if (o.AnnualAmount__c > 0 && o.OwnerId != TEST_USER_ID) {
            accountIds.add(o.AccountId);
        }  
    }

    if (!accountIds.isEmpty()) {
        Map<Id, Opportunity> accountToOppMap = new Map<Id, Opportunity>();

        Map<Id, Account> accountMap = new Map<Id, Account>([
            select AnnualAmount__c
            from Account
            where Id in :accountIds
        ]);

        for (Opportunity opp: [
            select AccountId,
                AnnualAmount__c,
                ClosedDate
            from Opportunity
            where AccountId in :accountIds and
                IsClosed = true and
                AnnualAmount__c
            order by ClosedDate
        ]) {
            if (!accountToOppMap.containsKey(opp.AccountId)) {
                accountToOppMap.put(opp.AccountId, opp);
            } else {
                if (accountToOppMap.get(opp.AccountId).ClosedDate > opp.ClosedDate) {
                    accountToOppMap.put(opp.AccountId, opp);
                }  
            }
        }   

        List<Account> accountsToUpdate = new List<Account>();

        for (Id id: accountIds) {
            Account account = accountMap.remove(id);
            Opportunity opp = accountToOppMap.remove(id);
            if (account.AnnualAmount__c != opp.AnnualAmount__c) {
                account.AnnualAmount__c = opp.AnnualAmount__c;
                accountsToUpdate.add(account);
            }
        }
        
        if (!accountsToUpdate.isEmpty()) {
            update accountsToUpdate;
        }
    }
}

NOTE: This code has not been tested and may contain typographical or logical errors.
This was selected as the best answer
sfdc-admin-smsfdc-admin-sm
Looks good pcon. Thank you for taking the time to help.