+ Start a Discussion

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. 
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); 
            // 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;          

Lucian Mihai CiobanuLucian Mihai Ciobanu

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!
great feedback, thank you!