0

I have a code duplication here. How can I fix it? I want to make my code better.

Helper:

public with sharing class JobHelper {

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

public void triggerInsertHandler(List<Job__c> jobList){

    for(Job__c job : jobList){ 
        if(job.Stage__c == 'Close'){
            createHandler(job.OwnerId, 'Rejection');
        }
        if(job.Stage__c == 'Offer'){
            createHandler(job.OwnerId, 'Offer');
        }
        insertHandler(jobList);     
    }
}

public void triggerUpdateHandler(List<Job__c> jobList, Map<Id, Job__c> oldMap){

    for(Job__c job : jobList){ 
        if(job.Stage__c == 'Close' && oldMap.get(job.Id).Stage__c != 'Close'){
            createHandler(job.OwnerId, 'Rejection');
        }
        if(job.Stage__c == 'Offer' && oldMap.get(job.Id).Stage__c != 'Offer'){
            createHandler(job.OwnerId, 'Offer');
        }
        insertHandler(jobList);     
    }
}
public List<Task> createHandler(Id ownId, String sbj){
    taskList.add(new Task(OwnerId = ownId, Subject = sbj));
    return taskList;
}
}

Trigger:

 trigger JobTrigger on Job__c (after insert, after update) {

    JobHelper helper = new JobHelper();

    if(Trigger.isAfter){
        if(Trigger.isInsert){
            helper.triggerInsertHandler(Trigger.new);
        }
        if(Trigger.isUpdate){
            helper.triggerUpdateHandler(Trigger.new, Trigger.oldMap);
        }
    } 
    }

2 Answers 2

3

You can use shortcircuit boolean expression to your advantage.

public with sharing class JobHelper {

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


    public void triggerInsertUpdateHandler(List<Job__c> jobList, Map<Id, Job__c> oldMap, Boolean isInsert){

        for(Job__c job : jobList){ 
            if(job.Stage__c == 'Close' && (isInsert ||oldMap.get(job.Id).Stage__c != 'Close')){
                createHandler(job.OwnerId, 'Rejection');
            }
            if(job.Stage__c == 'Offer' &&  (isInsert || oldMap.get(job.Id).Stage__c != 'Offer')){
                createHandler(job.OwnerId, 'Offer');
            }

        }
        insertHandler(jobList);
    }
    public List<Task> createHandler(Id ownId, String sbj){
        taskList.add(new Task(OwnerId = ownId, Subject = sbj));
        return taskList;
    }
}

Trigger Code:

trigger JobTrigger on Job__c (after insert, after update) {

    JobHelper helper = new JobHelper();

    if(Trigger.isAfter){
        if(Trigger.isInsert || Trigger.isUpdate){
            helper.triggerInsertUpdateHandler(Trigger.new,Trigger.oldMap,Trigger.isInsert);
        }

    } 
}

As you can see I am passing isInsert into the handler to make it more generic.

3

The insertHandler method should be outside of the for loop entirely. Aside from that, I really don't see any point in trying to optimize your code. If you had a ton of different values, you might use a map to check for appropriate values and the new task to create, but for only two, it would actually be more code than what you've written.

2
  • I would also change it to an if-else statement, instead of if/if. Only needs to evaluate one if then. Use variables for the close strings, so you don't keep repeating them. Less likely to cause run time errors with typos (would cause a compile error if you put the wrong variable name in)
    – Xander
    Commented Sep 12, 2018 at 14:40
  • @Xander Again, for only two values, there's no practical difference between if-else vs if-if. At this point, it's simply a micro-optimization, and premature optimization is the root of all evil.
    – sfdcfox
    Commented Sep 12, 2018 at 14:43

You must log in to answer this question.

Not the answer you're looking for? Browse other questions tagged .