Apex Trigger Optimization

Question

I have a child object called "Opportunity_Team__c" whose parent is Opportunity.

This Opportunity_Team__c object has 3 fields

Opportunity__c (lookup to Opportunity)
Role__c (which is a picklist) 
and 
User__c(which is a lookup to User)

Also in Task object we have a custom field called "Task Group Owner" which is a picklist and has the same picklist entries as in Opportunity_Team__c.Role__c

This image can be helpful

Whenever there is an update happening in Opportunity_Team__c record I need to query all the records in Task object with the below join and update the ownerId of the task with the latest value in Opportunity_Team__c.User__c

Task.WhatId = Opportunity_Team__c.Opportunity__c
&&
Task.Task_Group_Owner__c = Opportunity_Team.Role__c

I have developed the following trigger to achieve this.

Is there any way to optimize or improve this code ?

trigger OppTeamReAssignTaskTrigger on Opportunity_Team__c (after update) {

  List<Task> mastertasklist = new List<Task>();
  Set<Id> oids = new Set<Id>();

    for(Opportunity_Team__c ot : trigger.new)
    {
        oids.add(ot.Opportunity__c);
    }

    mastertasklist = [SELECT OwnerId, WhatId, Task_Group_Owner__c FROM Task WHERE WhatId IN :oids];

    List<Task> reassignTaskList = new List<Task>();

    for(Opportunity_Team__c ot : trigger.new)
    {
        for(Task t : mastertasklist)
        {
            if(ot.Opportunity__c == t.WhatId && ot.Role__c == t.task_group_owner__c)
            {
                t.OwnerId = ot.User__c;
            }
        }
    }

    update mastertasklist;    

}

EDIT:

Is there any way to modify the code so that I do not have to use two for-loops ?

Answer 1

Starting from where you get the masterTaskList, you should create a map so you can easily find the records you're looking for.

trigger OppTeamReAssignTaskTrigger on Opportunity_Team__c (after update) {

    Set<Id> oids = new Set<id>();
    Map<Opportunity_Team__c, Id> oppRoleMap = new Map<Opportunity_Team__c, Id>();
    // Create a map of Opportunity/Role to User Id
    for(Opportunity_Team__c record: Trigger.new) {
      oids.add(record.Opportunity__c);
      oppRoleMap.put(
        new Opportunity_Team__c(
          Opportunity__c=record.Opportunity__c, 
          Role__c=record.Role__c),
      record.User__c);
    }
    Task[] masterTaskList = [SELECT OwnerId, WhatId, Task_Group_Owner__c FROM Task WHERE WhatId IN :oids],
      recordUpdateList = new Task[0];
    // Use opportunity/role combination to find new owner
    for(Task record: masterTaskList) {
      Opportunity_Team__c key = new Opportunity_Team__c(
          Opportunity__c=record.WhatId, 
          Role__c=record.Task_Group_Owner__c);
      Id value = oppRoleMap.get(key);
      // Update only if the user differs from the task
      if(value != null && value != record.OwnerId) {
        record.OwnerId = value;
        recordUpdateList.add(record);
      }
    }
    update recordUpdateList;
}

Answer 2

This:

for(Opportunity_Team__c ot : trigger.new)
{
    for(Task t : mastertasklist)
    {
        if(ot.Opportunity__c == t.WhatId && ot.Role__c == t.task_group_owner__c)

is an example of a pattern whose name I don't know, but which might be called something like "combinatorial search": essentially, performing a search by iterating over multiple lists until items match up. It is not an efficient pattern.

Instead of doing this, you can simply iterate over your Opportunity Team objects once (as you already do at the top of the trigger) and construct a Map of Opportunity Id to Team object to obtain the relevant ownership details.

Map<Id, Opportunity_Team__c> opportunityToOppTeam = new Map<Id, Opportunity_Team__c>();

Within for loop over trigger.new, add code to populate this map:

if (ot.Role__c == t.task_group_owner__c) { 
    opportunityToOppTeam.put(ot.Opportunity__c, ot);
}

Then, the nested for loops simplify to a single loop with an update performed only on relevant objects.

for (Task t : mastertasklist) {
    Opportunity_Team__c ot = opportunityToOppTeam.get(t.WhatId);
    if (t.OwnerId != ot.User__c) {
        t.OwnerId = ot.User__c;
        reassignTaskList.add(t);
    }
 }

Then you update reassignTaskList, which is currently an unused variable.