Is this good coding practice?

Question

Today I have been going through some triggers written by former consultants and it seems like they "brute force" some stuff. It may be fine that that did it this way but it seems like to me it could have been done a little more efficiently.

So what this trigger does is it auto-populates fields on our opportunity after querying Users

For instance here, they are taking every single user, which is about 900(dev, I don't know about production). Couldn't that cause a governance situation? There is only 5 Region Directors that they need to find..

user newRegionDirector;
Map<String, User> userMap = new Map<String, User>();
List<user> userList = [select id, Name, isActive from User where isActive=true];
for(user u: userList){
    userMap.put(u.Name, u);     
}

It then loops through Opportunity searching for the guys.

for(Opportunity opp:Trigger.new){

    if(opp.Design_Region__c != null && opp.Design_Territory__c != null  && opp.Design_Region__c == 'EUROPE' && opp.Design_Territory__c == 'NORTH' ){
        newRegionDirector= userMap.get('//the user');
        if(newRegionDirector!=null){
            opp.Regional_Director__c = newRegionDirector.id ;
        }
       // opp.ISR__c = '//the id';

    }

I'm not having any issue with this trigger, its more of a general knowledge question. Could this have been done better and would there be an issue with more users?

Answer 1

It's rubbish :)

I'm not sure how complex is the thing you've anonymised here:

newRegionDirector= userMap.get('//the user');

Is it somehow dependent on the Opportunity Owner / other Opp. fields or is it always going to be "John Doe"? Even if it's always "John Doe" - in ideal world you shouldn't have him hardcoded like that but identified by Role for example... or just some kind of checkbox / picklist on his record that only SysAdmins can modify...

And remember that in different locales the User.Name != User.FirstName + ' ' + User.LastName. Hungarians and Chinese prefer the "Doe John" notation so if user has set a hungarian locale preference - this trigger won't find the manager :D

But I digress.

---

Roughly speaking you should have:

1. "learning loop"
2. query only the stuff you have to
3. "fixing loop"

pattern.

"Learning loop" - collect from trigger.new info which data you have to fetch. Most of the time it's some kind of Set<String> of values you need to check against reference object. In this particular scenario - it can be as stupid as a boolean flag.

I've commented out an example here that would add Opp. owners' managers to the set to show how it could look like. You can skip all the commented out lines if you're reading it for the first time.

// Set<Id> usersToCheck = new Set<Id>();
Boolean needToFetchTheUser = false;

for(Opportunity o : trigger.new){
    if(o.Design_Region__c == 'EUROPE' && o.Design_Territory__c == 'NORTH' ){
        // usersToCheck.add(o.OwnerId);
        needToFetchTheUser = true; // you can even add "break;" here ;)
    }
}

Query - if the set is empty / boolean is false then we don't have to do anything, no need to waste the query. And certainly no need to retrieve info about 900 users.

if(needToFetchTheUser){
    User regionalManager = [SELECT Id FROM User WHERE Name = 'John Doe'];
    // Map<Id, User> oppOwners = new Map<Id, User>([SELECT Id, ManagerId FROM User WHERE Id IN :usersToCheck );

    // (the "fixing loop" part should be embedded in this if statement)
}

"Fixing loop" - one more pass through trigger.new, this time setting the stuff we know. If the reference data was added to a Map it's very fast & effective.

for(Opportunity o : trigger.new){
    if(o.Design_Region__c == 'EUROPE' && o.Design_Territory__c == 'NORTH' ){
        o.Regional_Director__c = regionalManager.id ;
        // o.Regional_Director__c = oppOwners.get(o.OwnerId).ManagerId;
    }
}

---

You've written that you need up to 5 users in total... So let's say you do have a picklist on the User object with values like 'North Europe', 'South Europe' and so on (again, one option would be to have these as Roles but that's going a bit too far).

Set<String> keys = new Set<String>();

for(Opportunity o : trigger.new){
    if(o.Design_Region__c != null && o.Design_Territory__c != null ){
        keys.put(o.Design_Territory__c + ' ' + o.Design_Region__c); // composite key
    }
}

if(!keys.isEmpty()){
    Map<String, User> regionalManagers = new Map<String, User>();
    for(User u : [SELECT Id, Region__c FROM User WHERE isActive = true and Region__c IN :keys]){
        regionalManagers.put(u.Region__c.toLower(), u); // will overwrite existing entries if there's more than 1 guy in that "Role"!
    }

    for(Opportunity o : trigger.new){
        if(o.Design_Region__c != null && o.Design_Territory__c != null ){
            String mapKey = (o.Design_Territory__c + ' ' + o.Design_Region__c).toLower();
            if(regionalManagers.containsKey(mapKey)){
                o.Regional_Director__c = regionalManagers.get(mapKey).Id;
            }
        }
    }
}

You could even take it one step further. If you think about it in a way "unique region => user id" then maybe a Custom Setting with the mapping (type = list) would be a nice solution? That would mean 0 query rows wasted.

Answer 2

1) Don't map on the user name, it's better to map on an unique identifier like the user id

2) if opp.Design_Region__c != null && opp.Design_Region__c == 'EUROPE': you don't need to check if it's different than null, it will never be different than null and equal to 'EUROPE' in same time ;-)

3) I guess you call your trigger before insert, before update

Map<Id, User> userMap = new Map<Id, User>([select id, Name, isActive from User where isActive=true]);

    for(Opportunity opp:Trigger.new){
        if(opp.Design_Region__c == 'EUROPE' && opp.Design_Territory__c == 'NORTH' ){
            newRegionDirector= userMap.get(opp.OwnerId).Region__c;
            if(newRegionDirector!=null){
                opp.Regional_Director__c = newRegionDirector ;
            }
        }