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
ckellieckellie 

What is a more efficient way to write this code

I am writing a trigger to copy values in 5 different picklists, lookup the cooresponding user records, and past the ids in 8 cooresponding user ids in the appropriate lookup fields on the account page. At this time I am using 8 different lists to capture the different userids. Below is my code:

trigger SetPCNApprovers on PCN_s__c (Before Insert, Before Update) {

    Set<Id> pId = new Set<Id>();
    string scheduling;
    string OE;
    string redmondeng;
    string shipping;
    string peng;
    string estimating;
    string techw;
    string accounting;
    
    For(PCN_s__c pcn : trigger.new){
     pId.add(pcn.id);
     scheduling = pcn.scheduling__c;
     System.debug('**** 1 scheduling id : '+ pcn.scheduling__c);
     oe = pcn.Order_entry__c;
     redmondeng = pcn.Redmond_ENG__c;
     shipping = pcn.shipping__c;
     peng = pcn.Project_Engineering__c;
     estimating = pcn.estimating__c;
     techw = pcn.tech_writing__c;
     accounting = pcn.A_R_Acctg__c;

    }
                
    List<user> usched = new List<user>([select id, name__c from user where name__c =: scheduling]);                

    List<user> uoe = new List<user>([select id, name__c from user where name__c =: oe]);

    List<user> ured= new List<user>([select id, name__c from user where name__c =: redmondeng]);

    List<user> uship = new List<user>([select id, name__c from user where name__c =: shipping]);

    List<user> upeng = new List<user>([select id, name__c from user where name__c =: peng]);

    List<user> uest = new List<user>([select id, name__c from user where name__c =: estimating]);

    List<user> utechw = new List<user>([select id, name__c from user where name__c =: techw]);
    
    List<user> uaccounting = new List<user>([select id, name__c from user where name__c =: accounting]);

    For(PCN_s__c upcn : trigger.new){
        If(usched.size()>0){
            upcn.Scheduling_User_Lookup__c = usched[0].id;
        }
        If(uoe.size()>0){
            upcn.Order_Entry_Lookup__c = uoe[0].id;
        }
        If(ured.size()>0){
            upcn.Redmond_Engineer_Lookup__c = ured[0].id;
        }
        If(uship.size()>0){
            upcn.Shipping_Lookup__c = uship[0].id;
        }
        If(upeng.size()>0){
            upcn.Project_Engineering_Lookup__c = upeng[0].id;
        }
        If(uest.size()>0){
            upcn.Estimating_Lookup__c = uest[0].id;
        }
        If(utechw.size()>0){
            upcn.Tech_Writing_Lookup__c = utechw[0].id;
        }
        If(uaccounting.size()>0){
            upcn.Accounting_Lookup__c = uaccounting[0].id;
        }
   
    }
}

 How do I write a better trigger?

 

Thanks,

Jared KremerJared Kremer

First off I would remove the logic of this process out into its own class and just pass it the trigger.new and if needed trigger.old records. This makes it easier to test and separate your concerns. 

 

And is this supposed to be bulk friendly because it looks like its only going to query the last records if i follow the code:

 

...
 For(PCN_s__c pcn : trigger.new){
     ...
     scheduling = pcn.scheduling__c;
     ...
 }

 List<user> usched = new List<user>([select id, name__c from user where name__c =: scheduling]);

 I parafrased the code a bit, but the way it looks its looping through the new records, and scheduling will be the last value of scheduling__c in the list of records, Should this instead be a Set of Strings for each pcn.scheduling where the query gets name__c in :schedulings (new List) ?

 

 
liron169liron169

Hello,

 

The part of querying the User (7 SOQL for the same table) seems wrong.
can use something like:

 

List<String> userNameset=new List<String>();
Map<String, User> type_User_Map=new Map<String, User>();

userNameLst.add(scheduling);
userNameLst.add(OE);
userNameLst.add(redmondeng);
userNameLst.add(shipping);
userNameLst.add(peng);
userNameLst.add(estimating);
userNameLst.add(techw);
userNameLst.add(accounting);


for(User userRec : [SELECT id, name__c from USER where name__c IN :userNameLst]
	type_User_Map.put(userRec.name__c, userRec)


For(PCN_s__c upcn : trigger.new)
{
	upcn.Scheduling_User_Lookup__c = type_User_Map.get(upcn.scheduling__c).id;
	upcn.Order_Entry_Lookup__c = type_User_Map.get(upcn.Order_entry__c).id;
	upcn.Redmond_Engineer_Lookup__c = type_User_Map.get(upcn.Redmond_ENG__c).id;
	upcn.Shipping_Lookup__c = type_User_Map.get(upcn.shipping__c).id;
	upcn.Project_Engineering_Lookup__c = type_User_Map.get(upcn.Project_Engineering__c).id;
	upcn.Estimating_Lookup__c = type_User_Map.get(upcn.estimating__c).id;
	upcn.Tech_Writing_Lookup__c = type_User_Map.get(upcn.tech_writing__c).id;
	upcn.Accounting_Lookup__c = type_User_Map.get(upcn.A_R_Acctg__c).id;
}

 
ckellieckellie

Thank you  for the sample code. I am been working with the code and have erecieved the followign error:

SetPCNApprovers: execution of BeforeUpdate caused by: System.NullPointerException: Attempt to de-reference a null object Trigger.SetPCNApprovers: line 32, column 1

  Here is my code:

trigger SetPCNApprovers on PCN_s__c (Before Insert, Before Update) {

List<String> userNameLst=new List<String>();
    string scheduling;
    string OE;
    string redmondeng;
    string shipping;
    string peng;
    string estimating;
    string techw;
    string accounting;

Map<String, User> type_User_Map=new Map<String, User>();

userNameLst.add(scheduling);
userNameLst.add(OE);
userNameLst.add(redmondeng);
userNameLst.add(shipping);
userNameLst.add(peng);
userNameLst.add(estimating);
userNameLst.add(techw);
userNameLst.add(accounting);


for(User userRec : [SELECT id, name__c from USER where name__c IN :userNameLst]){
    type_User_Map.put(userRec.name__c, userRec);
    }


For(PCN_s__c upcn : trigger.new)
{
    upcn.Scheduling_User_Lookup__c = type_User_Map.get(upcn.scheduling__c).id;
    upcn.Order_Entry_Lookup__c = type_User_Map.get(upcn.Order_entry__c).id;
    upcn.Redmond_Engineer_Lookup__c = type_User_Map.get(upcn.Redmond_ENG__c).id;
    upcn.Shipping_Lookup__c = type_User_Map.get(upcn.shipping__c).id;
    upcn.Project_Engineering_Lookup__c = type_User_Map.get(upcn.Project_Engineering__c).id;
    upcn.Estimating_Lookup__c = type_User_Map.get(upcn.estimating__c).id;
    upcn.Tech_Writing_Lookup__c = type_User_Map.get(upcn.tech_writing__c).id;
    upcn.Accounting_Lookup__c = type_User_Map.get(upcn.A_R_Acctg__c).id;
}

}

Thanks
liron169liron169

Hello,

I didn't write for you the whole code, just example for how to do 1 soql instead of 7, by using list/map.

 


In the last example you post, you don't intialize the string variabe (scheduling, OE...), therefor the list userNameLst contains only null value, and the soql from the User return nothing.
Than when you try to get values from the map you get exception.


It is also good to check the value from the map.
e.g.

if(type_User_Map.containsKey(upcn.scheduling__c)) 
upcn.Scheduling_User_Lookup__c=type_User_Map.get(upcn.scheduling__c).id;