How to properly update fields via a before update trigger handler

Question

As a part of a larger project surrounding Apex triggers, I need to move what was functional code from a before update trigger on the lead object to a trigger handler called by a trigger on the Lead object.

While working through this I am struggling with how to best update OwnerID and Reassignment_Status__c (see the two if statements in the code below, I know that the LD variable does not exist).

In addition, I would like to get feedback regarding the overall structure of how this is written because:

1. I'm concerned that the way that I am using the SELECT statements here may be inefficient

2. I have another project upcoming in which the plan is to leverage after an after delete trigger handler to grab values from merged lead records and insert them into a custom object. How/whether I am able to address the issues with the handler I am posting about here will inform how I approach that task as well.

Below is my current code.

public void OnBeforeUpdate( List<Lead> newLead, List<Lead> oldLead, Map<ID, Lead> newLeadMap , Map<ID, Lead> oldLeadMap )
{
    system.debug('Lead Trigger On Before Update ');

    try
    {
        Set<Id> ldIdSet = new Set<Id>();    //Initialize set of LeadIDs
        List<Lead> ldList = new List<Lead>(); //Initialize list of lead records for update
        List<ProcessInstance> piList = new List<ProcessInstance>();   //Initialize list of approval process items


        ldList = [SELECT Id, OwnerID, Reassignment_Status__c FROM Lead WHERE Id IN :ldIdSet];
        piList = [SELECT SubmittedById,SystemModstamp,TargetObjectId, Status,  (SELECT ID, ProcessNodeId, StepStatus, Comments,TargetObjectId,ActorId,CreatedById,IsDeleted,IsPending, OriginalActorId,ProcessInstanceId,RemindersSent,CreatedDate 
        FROM StepsAndWorkitems ) FROM ProcessInstance pi WHERE pi.TargetObjectId IN :ldIdSet order by createddate DESC limit 1];

            if(ldList.size() > 0) {
                for(Lead l : ldList)
                {
                    ldIdSet.Add(l.Id);   //Add Ids from ldList to the LeadID set list
                }

                        If (ldList[0].Reassignment_Status__c == 'Pending' && piList[0].Status == 'Approved')   //Update Owner to the approval process requestor
                        {

                        LD.OwnerId = piList[0].SubmittedById;
                        LD.Reassignment_Status__c = '';
                        system.debug('Reassignment Approved - Owner Changed from '+ldList[0].OwnerId+'to ' +piList[0].SubmittedById);

                        }


                        If (ldList[0].Reassignment_Status__c == 'Pending' && piList[0].Status == 'Rejected') //Clear the reassignment status indicator, make no change to Owner


                            LD.Reassignment_Status__c = '';


                                system.debug('Reassignment Rejected - Owner not changed');

                                  }            
    }

    catch(Exception e)
    {
        system.debug('The following exception has occured:' +e.getMessage());
        system.debug('The following Line:' + e.getLineNumber());
    }   

}

Answer 1

(Please, excuse the indentation in the code snippets)

Because you are passing the Lists of sObjects to the handler, they are passed by reference and the only thing you need to do to update them is modify the fields you are interested in (just like you are currently doing using the equal sign for assignment). The Salesforce execution framework and the before update trigger will reference the new values. Your code has some issues which I will point out in the following code snippet.

See code comments:

public void OnBeforeUpdate( List<Lead> newLead, List<Lead> oldLead, Map<ID, Lead> newLeadMap , Map<ID, Lead> oldLeadMap )
{

    system.debug('Lead Trigger On Before Update ');

    try
    {
    Set<Id> ldIdSet = new Set<Id>();  //Why do you need a Set? pass the newLead List directly. You are missing the reference to the records coming from the trigger!!!!
    List<Lead> ldList = new List<Lead>(); // call this leadList, cleaner to read
    List<ProcessInstance> piList = new List<ProcessInstance>(); 


    ldList = [SELECT Id, OwnerID, Reassignment_Status__c FROM Lead WHERE Id IN :ldIdSet]; //ldIdSet IS EMPTY!!! This query will retun 0 records
    piList = [SELECT SubmittedById,SystemModstamp,TargetObjectId, Status,  (SELECT ID, ProcessNodeId, StepStatus, Comments,TargetObjectId,ActorId,CreatedById,IsDeleted,IsPending, OriginalActorId,ProcessInstanceId,RemindersSent,CreatedDate 
    FROM StepsAndWorkitems ) FROM ProcessInstance pi WHERE pi.TargetObjectId IN :ldIdSet order by createddate DESC limit 1]; //ldIdSet IS EMPTY!!! This query will retun 0 records

        if(ldList.size() > 0) { //Because of the query before, this will never evaluate to true (END OF TRY BLOCK)
            for(Lead l : ldList) // don't name a variable with a single letter, not good practice in long code classes
            {
                ldIdSet.Add(l.Id);   //Too late, your set was already referenced in the queries when it was empty
            }

                    If (ldList[0].Reassignment_Status__c == 'Pending' && piList[0].Status == 'Approved')   // Huge issue, all your helper logic assumes only one record. If 20 records come through they will update that same record 20 times. Everything from here on has this same issue
                    {

                    LD.OwnerId = piList[0].SubmittedById;
                    LD.Reassignment_Status__c = '';
                    system.debug('Reassignment Approved - Owner Changed from '+ldList[0].OwnerId+'to ' +piList[0].SubmittedById);

                    }


                    If (ldList[0].Reassignment_Status__c == 'Pending' && piList[0].Status == 'Rejected') 


                    LD.Reassignment_Status__c = '';


            system.debug('Reassignment Rejected - Owner not changed');

    }            
}

catch(Exception e)
{
    system.debug('The following exception has occured:' +e.getMessage());
    system.debug('The following Line:' + e.getLineNumber());
}   

}

Here is my version of the same code leveraging maps to tie Lead to ProcessInstance.

public void OnBeforeUpdate( List<Lead> newLead, List<Lead> oldLead, Map<ID, Lead> newLeadMap , Map<ID, Lead> oldLeadMap )
{

    system.debug('Lead Trigger On Before Update ');

    try
    {
    Map<Id,ProcessInstance> leadIdToPi = new Map<Id,ProcessInstance>(); 


    Map<Id,Lead> leadMap = new Map<Id,Lead>([SELECT Id, OwnerID, Reassignment_Status__c FROM Lead WHERE Id IN :newLead]); 
    List<ProcessInstance> piList = [SELECT SubmittedById,SystemModstamp,TargetObjectId, Status,  (SELECT ID, ProcessNodeId, StepStatus, Comments,TargetObjectId,ActorId,CreatedById,IsDeleted,IsPending, OriginalActorId,ProcessInstanceId,RemindersSent,CreatedDate 
    FROM StepsAndWorkitems ) FROM ProcessInstance pi WHERE pi.TargetObjectId IN :leadMap.keySet() order by createddate DESC limit 1]; 

    for(ProcessInstance processInstance : piList){
        leadIdToPi.put(processInstance.TargetObjectId,processInstance);
    }

    for(Lead lead : leadList){
        ProcessInstance processInstance = leadIdToPi.get(lead.Id);
        if (lead.Reassignment_Status__c == 'Pending' && processInstance.Status == 'Approved')   
        {

        lead.OwnerId = processInstance.SubmittedById;
        lead.Reassignment_Status__c = '';
        system.debug('Reassignment Approved - Owner Changed from '+lead.OwnerId+'to ' +processInstance.SubmittedById);

        }


        if (lead.Reassignment_Status__c == 'Pending' && processInstance.Status == 'Rejected') 


        lead.Reassignment_Status__c = '';


        system.debug('Reassignment Rejected - Owner not changed');
    }




    }            
}

catch(Exception e)
{
    system.debug('The following exception has occured:' +e.getMessage());
    system.debug('The following Line:' + e.getLineNumber());
}   

}

Hope this helps.