Apex Code Development You need to sign in to do that
Don't have an account?
HotCopper_Sales Please critique my code - Newbies first script
Please critique my code. I am very new at Apex and am not a programmer.
The object of this code is to update the Opportunity Next Action field with the earliest associated activity.
Maybe i could have put a join in the SOQL query but i am not confident with that yet.
The object of this code is to update the Opportunity Next Action field with the earliest associated activity.
Maybe i could have put a join in the SOQL query but i am not confident with that yet.
trigger TaskUpdateTrigger on Task (after insert, after update) {
// When a task inserted/updated
For (Task t: Trigger.new){
// check if task is related to an opportunity
String relatedID = String.valueOf(t.whatId);
if(relatedID.substring(0,3)=='006'){
// get all tasks associated with that opportunity
Task[] relatedTasks = [SELECT Id, Subject, ActivityDate FROM Task
WHERE WhatId=:t.WhatId
AND ActivityDate != null
AND Status != 'Completed'
ORDER BY ActivityDate ASC
LIMIT 1];
// get the related opportunity
Opportunity relatedOpp = [SELECT Id, NextStep FROM Opportunity
WHERE Id =:t.WhatId];
// format the string -date very short + next activity
Datetime fullDateTime = relatedTasks[0].ActivityDate;
String formatedDate = '[' + fullDateTime.format('d/M') + '] ';
String NextStepString = String.valueOf(formatedDate + relatedTasks[0].Subject);
// update Opportunity SObject
relatedOpp.NextStep = NextStepString;
// update database
update relatedOpp;
}
}
}
The most obvious problem I see is that you run 2 queries and one DML inside the FOR loop.
Aside from making it inefficient when adding more than a few tasks at once, it will make your code fail when you add more than 50 tasks.
The governor limits ( https://developer.salesforce.com/docs/atlas.en-us.apexcode.meta/apexcode/apex_gov_limits.htm
) allows you to run 100 queries in a transaction.
To make it efficient and safe, it's best practice to keep the queries outside the loops.
Biuld a FOR loop where you put all the opportunity Ids in a set, then run 2 queries selecting all opportunities and tasks based on that set of Ids, then another FOR loop to set the values in the opportunity.
A few more ideas when you implement the above:
- Avoid an loop-inside-loop that builds itself into a cartesian product. Although it's a bit more relaxed than in the past, there is also a limit for CPU usage. Use Maps, they are your efficiency sidekicks :)
- Avoid updating opportunities that you don't need to. For instance, if you set the field on an opportunity and the value is exactly the same it was before, an update on that oppotunity is not needed. It will cause unnecessary triggers and processes to run and consume resources. It's not evident if there are just a few triggers in the org, but once the code base grows, these inefficiencies will surface.
I hope that helps!
Lucian