Take the 2-minute tour ×
Salesforce Stack Exchange is a question and answer site for Salesforce administrators, implementation experts, developers and anybody in-between. It's 100% free, no registration required.

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?

share|improve this question
1  
One issue: you better not have two users with the same name or that map is not going to behave the way it was designed. –  Mark Pond Apr 29 at 22:54
    
Good point, never thought of that –  EricSSH Apr 29 at 23:22

2 Answers 2

up vote 10 down vote accepted

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.

share|improve this answer
    
Brilliant, @eyescream. One wonders how much per hour the 'consultants' received so I can calibrate my own rates :-) –  crop1645 Apr 30 at 1:41
1  
Never heard someone name the components of this pattern like this but I love it! Excellent way to explain the situation :) –  LaceySnr Apr 30 at 3:44
    
This is great, I will read this in more depth tomorrow.. @crop1645 makes you wonder haha.. –  EricSSH Apr 30 at 6:09
    
Enjoyable read. Curious question.... "Learning loop" is this something you made up, or is it an actual official term from somewhere? –  dphil Apr 30 at 13:47
1  
@dphil no way :) It's 100% "original research", luckily this is not Wikipedia... "Collecting loop"? ;) I just tend to do stuff that way, the official docs don't show similar patterns as far as I know. developer.salesforce.com/page/… is from 2009. blog.jeffdouglas.com/2009/04/20/… and sfdc.arrowpointe.com/2008/09/13/bulkifying-a-trigger-an-example are similar to the thing I've described. –  eyescream Apr 30 at 14:06

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 ;
            }
        }
share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.