+ Start a Discussion
BCSScottBCSScott 

Good Code vs. Bad Code

I am new to Apex and have just finished building my first trigger. Before I set up the test code I wanted to make sure that I wasn't going to submit code that was efficient or poorly written. According to an article I read, it is considered poor coding to include a SOQL query within a For loop. I am posting my code in this message and would really appreciate any advice on how to clean up my code so that it works as efficiently as possible.

 

I have highlighted the section I think might be poorly written in red.

 

 

 

 

trigger CreateLGAEvent on LGA__c (after insert, after update){



Map<String, LGA__c> lgaMap = new Map<String, LGA__c>();



List<Event> lgaEvents = new List<Event>();



    if(trigger.isinsert)

    

        for (LGA__c lga : System.Trigger.new){ 



            Event event = new Event(

            

                StartDateTime = lga.LGA_Start_DateTime__c ,

                EndDateTime = lga.LGA_End_Date_Time__c,

                Subject = 'LGA Scheduled',

                IsRecurrence = FALSE,

                Description = lga.Event_Description__c, 

                Location = lga.Event_Subjects__c, 

                Event_ID__c = lga.Event_ID__c,

                IsAllDayEvent = False); 

                

            lgaEvents.add(event);

            

            insert lgaEvents;

              

           }

         

    else

    

        for (LGA__c lga : System.Trigger.New) {

          

            Event event = [Select id, Event_ID__c from Event where Event_ID__c = :lga.Event_ID__c];

              

                event.StartDateTime = lga.LGA_Start_DateTime__c;

                event.EndDateTime = lga.LGA_End_Date_Time__c;

                event.Subject = 'LGA Scheduled'; 

                event.Description = lga.Event_Description__c; 

                event.Location = lga.Event_Subjects__c; 



            lgaEvents.add(event);



            update lgaEvents;

            

            }

              

}

 

 

Thanks in advance.

 

 

JVH

Best Answer chosen by Admin (Salesforce Developers) 
ThomasTTThomasTT

Ok, you have 3 things to improve your code/post.

1. Use code box to post your code

When you post your post, you see an brief case with "C" icon in the editor at the top. If you use it, the code is posed as code and it looks prettier. So, I'll copy and paste yours for my post.

2. isInsert = true section - insert records at once

So, you define List<Event> lgaEvents out of the loop. I guess you saw it in someone's code.

The reason is, to save the # of DML statement. You can actually execute only 20 DML statements (insert/update/delete) in 1 execution (including triggers/workflows triggered by the trigger), so inserting a record one by one is not a good idea (Trigger.new could have up to 200 records).

And insert/update/delete accept an object and List, so you can actually do insert lgaEvent;.

List<Event> lgaEvents = new List<Event>();
if(trigger.isinsert)
for (LGA__c lga : System.Trigger.new){
Event event = new Event(
StartDateTime = lga.LGA_Start_DateTime__c ,
EndDateTime = lga.LGA_End_Date_Time__c,
Subject = 'LGA Scheduled',
IsRecurrence = FALSE,
Description = lga.Event_Description__c,
Location = lga.Event_Subjects__c,
Event_ID__c = lga.Event_ID__c,
IsAllDayEvent = False);
lgaEvents.add(event);
insert lgaEvents;
}

if(lgaEvents.size() > 0) insert lgaEvents;

}else{
....
}

3.  Query records at once

The # of Query is also up to 20 in 1 execution, so as you said, you need to save the # of query.

If you are a developer for Java or any other language, you may think this is weird, but if it is about the governor limit, you also should use the gorverner limit for your good.

The point is, you use "WHERE - IN" clause to get all required records at once. the "IN" clause accept List and Set so SFDC expects us to use it. Define a Set (to avoid duplication) to hold all Event_ID__c to query, and use it for "IN" clause. Since the Trigger.new has up to "only" 200 records due to the limit of max batch size, the Set won't have more than 200 IDs in this case. Well, that's not so bad.

For your convenience, you should put the query result to Map<Id, Event> so that you can easily get the Event object by Event_ID__c.

And you don't update the record one by one.

 

Set<Id> idSetEvent = new Set<Id>();
for (LGA__c lga : System.Trigger.New) {
idSetEvent.add(lga.Event_ID__c);
}

Map<String, Event> evMap = new Map<String, Event>();

for(Event ev :[
SELECT
id, Event_ID__c
FROM
Event
WHERE
Event_ID__c in :idSetEvent
]){

evMap.put(ev.Event_ID__c, ev);

}

for (LGA__c lga : System.Trigger.New) {
Event event = evMap.get(lga.Event_ID__c);
if(event == null){
// do something like event.addError('Event ID is wrong');
continue;
}

event.StartDateTime = lga.LGA_Start_DateTime__c;
event.EndDateTime = lga.LGA_End_Date_Time__c;
event.Subject = 'LGA Scheduled';
event.Description = lga.Event_Description__c;
event.Location = lga.Event_Subjects__c;
lgaEvents.add(event);
}
if(lgaEvents.size() > 0) update lgaEvents;

 ThomasTT

 

 

Message Edited by ThomasTT on 10-23-2009 03:09 PM
Message Edited by ThomasTT on 10-23-2009 03:11 PM

All Answers

ThomasTTThomasTT

Ok, you have 3 things to improve your code/post.

1. Use code box to post your code

When you post your post, you see an brief case with "C" icon in the editor at the top. If you use it, the code is posed as code and it looks prettier. So, I'll copy and paste yours for my post.

2. isInsert = true section - insert records at once

So, you define List<Event> lgaEvents out of the loop. I guess you saw it in someone's code.

The reason is, to save the # of DML statement. You can actually execute only 20 DML statements (insert/update/delete) in 1 execution (including triggers/workflows triggered by the trigger), so inserting a record one by one is not a good idea (Trigger.new could have up to 200 records).

And insert/update/delete accept an object and List, so you can actually do insert lgaEvent;.

List<Event> lgaEvents = new List<Event>();
if(trigger.isinsert)
for (LGA__c lga : System.Trigger.new){
Event event = new Event(
StartDateTime = lga.LGA_Start_DateTime__c ,
EndDateTime = lga.LGA_End_Date_Time__c,
Subject = 'LGA Scheduled',
IsRecurrence = FALSE,
Description = lga.Event_Description__c,
Location = lga.Event_Subjects__c,
Event_ID__c = lga.Event_ID__c,
IsAllDayEvent = False);
lgaEvents.add(event);
insert lgaEvents;
}

if(lgaEvents.size() > 0) insert lgaEvents;

}else{
....
}

3.  Query records at once

The # of Query is also up to 20 in 1 execution, so as you said, you need to save the # of query.

If you are a developer for Java or any other language, you may think this is weird, but if it is about the governor limit, you also should use the gorverner limit for your good.

The point is, you use "WHERE - IN" clause to get all required records at once. the "IN" clause accept List and Set so SFDC expects us to use it. Define a Set (to avoid duplication) to hold all Event_ID__c to query, and use it for "IN" clause. Since the Trigger.new has up to "only" 200 records due to the limit of max batch size, the Set won't have more than 200 IDs in this case. Well, that's not so bad.

For your convenience, you should put the query result to Map<Id, Event> so that you can easily get the Event object by Event_ID__c.

And you don't update the record one by one.

 

Set<Id> idSetEvent = new Set<Id>();
for (LGA__c lga : System.Trigger.New) {
idSetEvent.add(lga.Event_ID__c);
}

Map<String, Event> evMap = new Map<String, Event>();

for(Event ev :[
SELECT
id, Event_ID__c
FROM
Event
WHERE
Event_ID__c in :idSetEvent
]){

evMap.put(ev.Event_ID__c, ev);

}

for (LGA__c lga : System.Trigger.New) {
Event event = evMap.get(lga.Event_ID__c);
if(event == null){
// do something like event.addError('Event ID is wrong');
continue;
}

event.StartDateTime = lga.LGA_Start_DateTime__c;
event.EndDateTime = lga.LGA_End_Date_Time__c;
event.Subject = 'LGA Scheduled';
event.Description = lga.Event_Description__c;
event.Location = lga.Event_Subjects__c;
lgaEvents.add(event);
}
if(lgaEvents.size() > 0) update lgaEvents;

 ThomasTT

 

 

Message Edited by ThomasTT on 10-23-2009 03:09 PM
Message Edited by ThomasTT on 10-23-2009 03:11 PM
This was selected as the best answer
BCSScottBCSScott

ThomasTT thanks again for your help. I have gone over the adjustments that you suggested and they have shown me a lot on organization and avoiding governor caps. The only issue I am running into right now is that the code won't compile because it doesn't recognize the variable idSetEvent. I will highlight where I get the error below.

 

ErrorError: Compile Error: Variable does not exist: idSetEvent at line 30 column 30

 

trigger CreateLGAEvent on LGA__c (after insert, after update){ List<Event> lgaEvents = new List<Event>(); if(trigger.isinsert) for (LGA__c lga : System.Trigger.new){ Event e = new Event( StartDateTime = lga.LGA_Start_DateTime__c , EndDateTime = lga.LGA_End_Date_Time__c, Subject = 'LGA Scheduled', IsRecurrence = FALSE, Description = lga.Event_Description__c, Location = lga.Event_Subjects__c, Event_ID__c = lga.Event_ID__c, //WhatID = lga.LGA__c.WhatID, IsAllDayEvent = False); lgaEvents.add(e); } if(lgaEvents.size() > 0) insert lgaEvents; else Set<Id> idSetEvent = new Set<Id>(); for (LGA__c lga : System.Trigger.New) { idSetEvent.add(lga.Event_ID__c); } Map<String,Event> evmap = new Map<String,Event>(); for(Event ev :[ Select id, Event_ID__c From Event Where Event_ID__c in : idSetEvent ]){ evmap.put(ev.Event_ID__c, ev); } for (LGA__c lga : System.Trigger.New) { Event event = evmap.get(lga.Event_ID__c); if(event = null){ event.addError('Wrong Event ID'); continue; } event.StartDateTime = lga.LGA_Start_DateTime__c; event.EndDateTime = lga.LGA_End_Date_Time__c; event.Subject = 'LGA Scheduled'; event.Description = lga.Event_Description__c; event.Location = lga.Event_Subjects__c; lgaEvents.add(event); } if(lgaEvents.size() > 0) update lgaEvents; }

 

 

I will reply if I figure out the compile issue, but I figured I would post it here in case it was another easy fix. Thanks again.

ThomasTTThomasTT

You are missing {} for if(trigger.isinsert) else... posting message takes sometime. Take some time for debugging by yourself first, especially for compile error.

*I guess you already found out what was wrong...

ThomasTT

BCSScottBCSScott

Code is working great now, I appreciate the help. I will post it here so others can use it for reference.

 

trigger CreateLGAEvent on LGA__c (after insert, after update){ List<Event> lgaEvents = new List<Event>(); //creates a list that holds new events if(trigger.isinsert){ //actions if record is being inserted for (LGA__c lga : System.Trigger.new){ //for loop that defines new event Event event = new Event( StartDateTime = lga.LGA_Start_DateTime__c , EndDateTime = lga.LGA_End_Date_Time__c, Subject = 'LGA Scheduled', IsRecurrence = FALSE, Description = lga.Event_Description__c, Location = lga.Event_Subjects__c, Event_ID__c = lga.Event_ID__c, IsAllDayEvent = False); lgaEvents.add(event); } //closes for loop if(lgaEvents.size() > 0) insert lgaEvents; //adds events if lgaEvents list > 0 }else{ //closes if statement and states actions for all other triggers besides insert Set<String> idSetEvent = new Set<String>(); //creates a list that holds Event IDs for (LGA__c lga : System.Trigger.New) { //for loop that adds the Event_ID__c idSetEvent.add(lga.Event_ID__c); } //closes for loop Map<String, Event> evMap = new Map<String, Event>(); // creates a map to hold the event_ID__c that needs to be updated for(Event ev :[SELECT Event_ID__c FROM Event WHERE Event_ID__c in : idSetEvent]){ //for loop that assigns Event_ID__c into evMap evMap.put(ev.Event_ID__c, ev); //inserts Event_ID__c, ev into evmap Map } //closes for loop for (LGA__c lga : System.Trigger.New) { //for loop Event event = evMap.get(lga.Event_ID__c); if(event == null){ // if no Event_ID__c is found present error message event.addError('Event ID is wrong'); continue; } //closes if statement event.StartDateTime = lga.LGA_Start_DateTime__c; event.EndDateTime = lga.LGA_End_Date_Time__c; event.Subject = 'LGA Scheduled'; event.Description = lga.Event_Description__c; event.Location = lga.Event_Subjects__c; lgaEvents.add(event); } //closes for loop if(lgaEvents.size() > 0) update lgaEvents; // updates event if lgaEvents > 0 } } //closes trigger

 

 

- JVH