Apex Code Development You need to sign in to do that
Don't have an account?
Sree_1678 Need APEX code verification (working code)
Hi,
I have written below apex trigger to populate salesreps manager email and salesreps manager's manager email addresses; code is working good. I want to know if i follwed best practices
// This trigger will be used to populate Rep's manager and Rep's -> Manager's -> Manager email address on quote
// This will be used only in approval processes will not be displayed to users
trigger PopulateEmails on Quote (before insert)
{
for ( Quote c : trigger.new)
{
// to get & store current users manager id
Id getManagerID = [select id,ManagerId from user where id=: Userinfo.getUserId()][0].Managerid;
// to get reps/record creator managers email address
list<user> reps_manager = [select id,managerid,email from user where id=:getManagerID];
// to get managers manager email address
list<user> managers_manager = [select id,managerid,email from user where id=:reps_manager[0].managerid];
if(c.createdby.Name != 'xx' )
{
c.Managers_Email__c= reps_manager[0].email;
c.managers_manager_email__c= managers_manager[0].email;
}
}
}
FYI, you can always run a "security scan" against your code, it would tell you if you missed anything. As it is, you have a problem with this code, and that problem is that your code will fail to execute correctly if you use the Apex Data Loader to mass insert records (or if you later decide to integrate with a system that will create quotes in batch, etc). The reason why the code would fail is that you have multiple SOQL queries within a for loop, which is definitely not a best practice. Also, you're not checking for blank values, etc... and you can also combine the entire query into one...
Here's one way you could fix it:
trigger PopulateEmails on Quote( before insert, before update ) { Map< Id, User > owners = new map< id, user >( ); Map< Id, Opportunity > opps = new map< id, opportunity >( ); // Find all related opportunities. for( quote record: trigger.new ) { opps.put( record.opportunityid, null ); } // Query owner id for those opportunities opps.putall( [select id,ownerid from opportunity where id in :opps.keyset()] ); // find all owner ids for( opportunity record: opps.values( ) ) { owners.put( record.ownerid, null ); } // query all owners, manager emails, and manager's manager's emails owners.putall( [select id,name,manager.email,manager.manager.email from user where id in :owners.keyset()] ); for( quote record: trigger.new ) { // Get opportunity owner's user record. user owner = owners.get( opps.get( record.opportunityid ).ownerid ); // if this is not a dummy user if( owner != null && owner.name != 'xx' ) { // ... and this user has a manager if( owner.manager != null ) { // copy the manager's email. record.managers_email__c = owner.manager.email; // if the manager has a manager... if( owner.manager.manager != null ) { // copy that email address as well record.managers_managers_email__c = owner.manager.manager.email; } } } } }This reduces the number of queries from 3 per quote to 2 queries* for up to 200 quotes, makes no assumptions about who created the quote (e.g. if an administrator imports quotes, it will still work correctly), and doesn't assume that user has a manager, or that the manager has a manager.
* You could actually reduce this to a single query with a little imagination, but I didn't think that far ahead when I started writing this example.
Edit: I always forget the value in Map.put...
All Answers
FYI, you can always run a "security scan" against your code, it would tell you if you missed anything. As it is, you have a problem with this code, and that problem is that your code will fail to execute correctly if you use the Apex Data Loader to mass insert records (or if you later decide to integrate with a system that will create quotes in batch, etc). The reason why the code would fail is that you have multiple SOQL queries within a for loop, which is definitely not a best practice. Also, you're not checking for blank values, etc... and you can also combine the entire query into one...
Here's one way you could fix it:
trigger PopulateEmails on Quote( before insert, before update ) { Map< Id, User > owners = new map< id, user >( ); Map< Id, Opportunity > opps = new map< id, opportunity >( ); // Find all related opportunities. for( quote record: trigger.new ) { opps.put( record.opportunityid, null ); } // Query owner id for those opportunities opps.putall( [select id,ownerid from opportunity where id in :opps.keyset()] ); // find all owner ids for( opportunity record: opps.values( ) ) { owners.put( record.ownerid, null ); } // query all owners, manager emails, and manager's manager's emails owners.putall( [select id,name,manager.email,manager.manager.email from user where id in :owners.keyset()] ); for( quote record: trigger.new ) { // Get opportunity owner's user record. user owner = owners.get( opps.get( record.opportunityid ).ownerid ); // if this is not a dummy user if( owner != null && owner.name != 'xx' ) { // ... and this user has a manager if( owner.manager != null ) { // copy the manager's email. record.managers_email__c = owner.manager.email; // if the manager has a manager... if( owner.manager.manager != null ) { // copy that email address as well record.managers_managers_email__c = owner.manager.manager.email; } } } } }This reduces the number of queries from 3 per quote to 2 queries* for up to 200 quotes, makes no assumptions about who created the quote (e.g. if an administrator imports quotes, it will still work correctly), and doesn't assume that user has a manager, or that the manager has a manager.
* You could actually reduce this to a single query with a little imagination, but I didn't think that far ahead when I started writing this example.
Edit: I always forget the value in Map.put...
Perfect; your code works like a charm. only change i made is added null to below map. Thanks a lot for the suggestions and the modified code
// find all owner ids
for(opportunity record: opps.values())
{
owners.put(record.ownerid, null);
}