1

I built a trigger to update account owner depending on an external id value (Salesperson). If the salesperson code exists in a user then he will be the owner, if it doesn't exists or the code it's empty then, asssign in to an admin user. This is working as expected and I tested with single records to 50 records and it's working as expected. I'm not sure if this trigger follows the best practices in terms of bullk processing.

Is there any recomendation or a better approach that anyone can recommend?

trigger PopulateAccount on Account (before insert, before update) {

if(Trigger.isBefore && (Trigger.isInsert || Trigger.isUpdate)){ 
//Find Ownership according to the Salesperson_Code__c       
    Set<Integer> SAPID = new set<Integer>();
    Set<String> SAPIDOwner = new set<String>();
    Map<Integer,User> userIdObj = new map<Integer,User>();
    Map<String,User> ownerIdObj = new map<String,User>();
    if (trigger.isBefore && trigger.isUpdate){
        for (Account acc:trigger.New){
            if((trigger.oldMap.get(acc.Id).ownerId != trigger.newMap.get(acc.Id).ownerId) && (acc.ownerId != null)){
                SAPIDOwner.add(acc.ownerId);
            } else {
                if(acc.Salesperson_Code__c != null){
                    SAPID.add(acc.Salesperson_Code__c.intValue());
                }
            }               
        }
    }else{
        for(Account acc:trigger.New){   
            if(acc.Salesperson_Code__c == null){
                SAPIDOwner.add(acc.ownerId);//Admin?
            }else{
                if(acc.Salesperson_Code__c != null){
                    SAPID.add(acc.Salesperson_Code__c.intValue());
                } 
            }                   
        }
    }
    if(SAPID.size() > 0){
        for (User userSAP : [SELECT Id,Salesperson_Code__c  FROM user WHERE Salesperson_Code__c =:SAPID]){
            userIdObj.put(userSAP.Salesperson_Code__c.intValue(),userSAP);
        }
    }
    if(SAPIDOwner.size() > 0){
        for (User ownerObj: [SELECT Id,Salesperson_Code__c  FROM user WHERE Id =:SAPIDOwner]){
            ownerIdObj.put(ownerObj.Id,ownerObj);
        }
        if(ownerIdObj.size()>0){
            for(Account acc1:trigger.New){
                acc1.Salesperson_Code__c = ownerIdObj.get(acc1.ownerID).Salesperson_Code__c;
            }

        }   
    }else{
        if(userIdObj.size() > 0){
            for(Account acc1:trigger.New){
                if(acc1.Salesperson_Code__c != null){ 
                    if(userIdObj.get(acc1.Salesperson_Code__c.intValue()) != null){
                        acc1.ownerID = userIdObj.get(acc1.Salesperson_Code__c.intValue()).Id;                           
                }
            }

        }
    }else{
        List<User> users = [SELECT Id,username FROM user];
        User SAPIntegration = new User();
        for (User SAP:users){
            if (SAP.username == 'admin@company.com'){
                SAPIntegration = SAP; break;
            }
        }
        if(SAPIntegration.username == 'admin@company.com'){
            for (Account acc1:trigger.New){
                acc1.ownerId = SAPIntegration.Id;
            }
        }
    }
    }

}       
//END Find Ownership according to the Salesperson_Code__c          
}//END of trigger
5

If you google "apex trigger best practices", the very first article you will find is titled Trigger Frameworks and Apex Trigger Best Practices, and you are ignoring a few more or less universally accepted best practices listed therein:

  • One Trigger Per Object
    I'll just say that the naming of your trigger strongly implies it was written for a specific purpose, and if you approach your triggers with this best practice in mind, you will name them much more generically. I would just name my one trigger on the Account object simpy Account. In other words:

    trigger Account on Account (/*events*/)
    

    The one common exception being the Case object, which is a reserved keyword in Apex so I tend to name it CaseTrigger.

  • Logic-less Triggers
    You are cramming a ton of logic into your triggers. Whether or not you adopt a handler pattern, you should move nearly all of it into an Apex Class rather than an Apex Trigger.

  • Context-Specific Handler Methods
    Opinion is somewhat more divided on handler patterns, perhaps, but in general they are a good idea. My typical trigger body is (in my opinion) very clean and simple:

    trigger Account on Account (/*events*/)
    {
        AccountTriggerHandler handle =
            new AccountTriggerHandler(trigger.new, trigger.oldMap);
        if (trigger.isBefore)
        {
            if (trigger.isInsert) handle.beforeInsert();
            if (trigger.isUpdate) handle.beforeUpdate();
            if (trigger.isDelete) handle.beforeDelete();
        }
        if (trigger.isAfter)
        {
            if (trigger.isInsert) handle.afterInsert();
            if (trigger.isUpdate) handle.afterUpdate();
            if (trigger.isDelete) handle.afterDelete();
            if (trigger.isUndelete) handle.afterUndelete();
        }
    }
    

    Of course there are many patterns and you can choose whichever you prefer, but a handler helps you better separate your concerns between your questions of what actions to perform, when to perform each action, which records to act on, and how to perform each action.

    In my typical pattern, the Trigger specifies when, the Handler specifies what and which, and the Service specifies the how of each filter and action.

  • 1
    Thanks for your recommendation....It's brilliant, can I ask you if I use this context handler methods, should those methods be static public or just public methods? I don't fully understand how the context handler will return to the trigger the Account object witht the new values before the update process because on the trigger you just call the handler : if (trigger.isInsert) handle.beforeInsert(); – MANUELAN00 Mar 24 '17 at 4:56
  • @MANUELAN00 If you notice in the trigger pattern I show, I construct a single instance of the handler, passing in the trigger records as state. Then I call the instance method specific to the event being fired. – Adrian Larson Mar 24 '17 at 17:03

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.