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
Jodi SpitlerJodi Spitler 

My very first Trigger: updating a child record from a value on a parent record. Could anyone help me optimize?

Hi All, 

I'm very proud of my first trigger :), but I'm sure that it could be improved considerably.  Could anyone assist? It works, but I will still need another trigger on the Opportunity Product (as it only works on update and not on insert). ...and I'm sure I missed out on some basics as this is my first shot.

What the code does:  Updates a field (Duration) on the opportunity products based on a field on the opportunity line item (phase) and 3 fields on the opportunity (dur_nnCMRR_1, _2,or _3)

trigger UpdateDurations on Opportunity (after update) 
//removed after insert as opps don’t have any product line when they are first created//
{for(Opportunity opp: Trigger.new)
{Opportunity oldOpp = Trigger.oldMap.get(Opp.Id);
if(oldOpp.Dur_2_nnCMRR__c != Opp.Dur_2_nnCMRR__c )                              
{ List<OpportunityLineItem> children = [ SELECT Id, OpportunityId, Duration__c,Phase__c from 
  OpportunityLineItem where OpportunityId = :Opp.Id AND Phase__c='Evaluation'];
  List<OpportunityLineItem> newids = new List<OpportunityLineItem>();
        for(OpportunityLineItem LI: children)
        {if(LI.Duration__c != Opp.Dur_2_nnCMRR__c)
        {LI.Duration__c = Opp.Dur_2_nnCMRR__c;newids.add(LI);}}
         if (newids.isEmpty()== false)
         {update newids;}} 
if(oldOpp.Dur_1_nnCMRR__c != Opp.Dur_1_nnCMRR__c )                                 
{ List<OpportunityLineItem> Install = [ SELECT Id, OpportunityId, Duration__c,Phase__c from 
  OpportunityLineItem where OpportunityId = :Opp.Id AND Phase__c='Installation'];
  List<OpportunityLineItem> newids = new List<OpportunityLineItem>();
        for(OpportunityLineItem LI: Install)
        {if(LI.Duration__c != Opp.Dur_1_nnCMRR__c)
        {LI.Duration__c = Opp.Dur_1_nnCMRR__c;newids.add(LI);}}
         if (newids.isEmpty()== false)
         {update newids;}}
if(oldOpp.Dur_3_nnCMRR__c != Opp.Dur_3_nnCMRR__c )                                 
{ List<OpportunityLineItem> Use = [ SELECT Id, OpportunityId, Duration__c,Phase__c from 
  OpportunityLineItem where OpportunityId = :Opp.Id AND Phase__c='Usage'];
  List<OpportunityLineItem> newids = new List<OpportunityLineItem>();
        for(OpportunityLineItem LI: Use)
        {if(LI.Duration__c != Opp.Dur_3_nnCMRR__c)
        {LI.Duration__c = Opp.Dur_3_nnCMRR__c;newids.add(LI);}}
         if (newids.isEmpty()== false)
         {update newids;}}}}
 
Best Answer chosen by Jodi Spitler
venkat-Dvenkat-D
trigger UpdateDurations on Opportunity (after update) {
List<OpportunityLineItem> childrentoupdate = new List<OpportunityLineItem>();

List<OpportunityLineItem> children = [ SELECT Id, OpportunityId, Duration__c,Phase__c from 
  OpportunityLineItem where OpportunityId = :Trigger.NewMap.Keyset()  AND (Phase__c='Evaluation' OR Phase__c = 'Installation' OR Phase__c = 'Usage'];

for(OpportunityLineItem child : children){
if(child.Phase__C == 'Evaluation' && child.Duration__c != Trigger.NewMap.get(child.OpportunityId).Dur_3_nnCMRR__c ){
child..Duration__c = Trigger.NewMap.get(child.OpportunityId).Dur_3_nnCMRR__c;

childrentoupdate .add(child);
}

else if(child.Phase__C == 'Installation' && child.Duration__c != Trigger.NewMap.get(child.OpportunityId).Dur_2_nnCMRR__c ){
child..Duration__c = Trigger.NewMap.get(child.OpportunityId).Dur_2_nnCMRR__c;

childrentoupdate .add(child);
}

else if(child.Phase__C == 'Usage' && child.Duration__c != Trigger.NewMap.get(child.OpportunityId).Dur_1_nnCMRR__c){
child..Duration__c = Trigger.NewMap.get(child.OpportunityId).Dur_1_nnCMRR__c;

childrentoupdate .add(child);
}

}

if(childrentoupdate .size()>0){
update childrentoupdate ;
}


}

 

All Answers

venkat-Dvenkat-D
1. Replace below 
{for(Opportunity opp: Trigger.new)
{Opportunity oldOpp = Trigger.oldMap.get(Opp.Id);
if(oldOpp.Dur_2_nnCMRR__c != Opp.Dur_2_nnCMRR__c ) 

with {for(Opportunity opp: Trigger.new){
if(opp.Dur_2_nnCMRR__c != Trigger.oldmap.get(opp.Id).Dur_2_nnCMRR__c )

thats one statement vs 2 statements in 

2. Do not  use SOQL inside for loops as you wilt hit governor limits in mass update

{ List<OpportunityLineItem> children = [ SELECT Id, OpportunityId, Duration__c,Phase__c from 
  OpportunityLineItem where OpportunityId = :Opp.Id AND Phase__c='Evaluation'];

This should be outside for loop 
List<OpportunityLineItem> children = [ SELECT Id, OpportunityId, Duration__c,Phase__c from 
  OpportunityLineItem where OpportunityId = :Trigger.NewMAp.keyset() AND Phase__c='Evaluation'

and since you need three different lists you can use 

List<OpportunityLineItem> children = [ SELECT Id, OpportunityId, Duration__c,Phase__c from 
  OpportunityLineItem where OpportunityId = :Trigger.NewMAp.keyset() AND (Phase__c='Evaluation' OR  Phase__c='Usage' OR Phase__c='Installation')

Now loop through and do check like

for(OpportunityLineItem child : children){

// do logic like check for phase etc a
}
venkat-Dvenkat-D
trigger UpdateDurations on Opportunity (after update) {
List<OpportunityLineItem> childrentoupdate = new List<OpportunityLineItem>();

List<OpportunityLineItem> children = [ SELECT Id, OpportunityId, Duration__c,Phase__c from 
  OpportunityLineItem where OpportunityId = :Trigger.NewMap.Keyset()  AND (Phase__c='Evaluation' OR Phase__c = 'Installation' OR Phase__c = 'Usage'];

for(OpportunityLineItem child : children){
if(child.Phase__C == 'Evaluation' && child.Duration__c != Trigger.NewMap.get(child.OpportunityId).Dur_3_nnCMRR__c ){
child..Duration__c = Trigger.NewMap.get(child.OpportunityId).Dur_3_nnCMRR__c;

childrentoupdate .add(child);
}

else if(child.Phase__C == 'Installation' && child.Duration__c != Trigger.NewMap.get(child.OpportunityId).Dur_2_nnCMRR__c ){
child..Duration__c = Trigger.NewMap.get(child.OpportunityId).Dur_2_nnCMRR__c;

childrentoupdate .add(child);
}

else if(child.Phase__C == 'Usage' && child.Duration__c != Trigger.NewMap.get(child.OpportunityId).Dur_1_nnCMRR__c){
child..Duration__c = Trigger.NewMap.get(child.OpportunityId).Dur_1_nnCMRR__c;

childrentoupdate .add(child);
}

}

if(childrentoupdate .size()>0){
update childrentoupdate ;
}


}

 
This was selected as the best answer
Jodi SpitlerJodi Spitler
Thanks for the help!  I knew I'd make some newbie mistakes.  I implemented it in Sandbox and everything is working perfectly.  I tweaked the code a bit (line up with 1,2,3 and the names of the phases, and a ] ). With updates it's below.

trigger UpdateDurations on Opportunity (after update) {
List<OpportunityLineItem> childrentoupdate = new List<OpportunityLineItem>();

List<OpportunityLineItem> children = [ SELECT Id, OpportunityId, Duration__c,Phase__c from 
  OpportunityLineItem where OpportunityId = :Trigger.NewMap.Keyset()  AND (Phase__c='Evaluation' OR Phase__c = 'Installation' OR Phase__c = 'Usage')];

for(OpportunityLineItem child : children){
if(child.Phase__C == 'Evaluation' && child.Duration__c != Trigger.NewMap.get(child.OpportunityId).Dur_2_nnCMRR__c ){
child.Duration__c = Trigger.NewMap.get(child.OpportunityId).Dur_2_nnCMRR__c;

childrentoupdate .add(child);
}

else if(child.Phase__C == 'Installation' && child.Duration__c != Trigger.NewMap.get(child.OpportunityId).Dur_2_nnCMRR__c ){
child.Duration__c = Trigger.NewMap.get(child.OpportunityId).Dur_2_nnCMRR__c;

childrentoupdate .add(child);
}

else if(child.Phase__C == 'Usage' && child.Duration__c != Trigger.NewMap.get(child.OpportunityId).Dur_3_nnCMRR__c){
child.Duration__c = Trigger.NewMap.get(child.OpportunityId).Dur_3_nnCMRR__c;

childrentoupdate .add(child);
}

}

if(childrentoupdate .size()>0){
update childrentoupdate ;
}}

Also a shout out to ebsta that started me on the right track :)

https://www.ebsta.com/blog/salesforce-apex-trigger/