Take the 2-minute tour ×
Salesforce Stack Exchange is a question and answer site for Salesforce administrators, implementation experts, developers and anybody in-between. It's 100% free, no registration required.

This is my first production trigger and I'm just wondering if there is a more efficient way to write this code. Basically, if a campaign meets the criteria in the IF statement, I want it to create a new campaign for a year in the future - additionally, I want it to check off the campaign the trigger is run on so it only runs once.

I'm just wondering if I'm doing this right - or if there's a better way to write this?

trigger CloneCollectionCampaign on Campaign (before insert, before update) {
    List <Campaign> toClone = new List <Campaign>();
    List <Campaign> toUpdate = new List <Campaign>();
        for (Campaign c : Trigger.new){
            if (c.Cloned_Campaign__c == False && c.Status == 'Complete' && 
                c.StartDate != null && c.Total_Rollup__c > 0 || c.Baby_Items_Rollup__c > 0){
                Campaign campaignNew = new Campaign();
                campaignNew.Name = c.Name + 'Cloned';
                campaignNew.Status = 'Planned';
                campaignNew.StartDate = c.StartDate + 365;
                campaignNew.OwnerId = c.OwnerId;
                campaignNew.Type = c.Type;
                campaignNew.Subtype__c = c.Subtype__c;
                campaignNew.Start_Time__c = c.Start_Time__c;
                c.Cloned_Campaign__c = True;
                toClone.add(campaignNew);
                toUpdate.add(c);
            }
        }
    for (Campaign i : Trigger.new){

    }
    try {
        insert toClone;
    } catch (system.DmlException e){
        system.debug(e);
    }

Thanks in advance!

Update:

So I've taken all the feedback and made some changes and would love to get some more feedback. Here's the trigger:

trigger CloneCollectionCampaign on Campaign (before insert, before update) {

CampaignHandler handler = new CampaignHandler();

if(Trigger.isInsert && Trigger.isBefore){
    handler.handleBeforeInsert(Trigger.new);
}

else if(Trigger.isUpdate && Trigger.isBefore){
    handler.handleBeforeUpdate(Trigger.new);
}
}

And here's the handler class - note that I've yet to write test classes for either of these but in just navigating around the UI, it all appears to work:

public class CampaignHandler {
private boolean m_isExecuting = false;

public void cloneCampaign(Campaign[] newCampaigns){

    List <Campaign> toClone = new List <Campaign>();
    List <Campaign> toUpdate = new List <Campaign>();

    String recordTypeName = 'Collection Campaign';
    Map<String,Schema.RecordTypeInfo> rtMapByName = Schema.SObjectType.Campaign.getRecordTypeInfosByName();
    Schema.RecordTypeInfo rtInfo =  rtMapByName.get(recordTypeName);
    id recordTypeId = rtInfo.getRecordTypeId();

        for (Campaign c : newCampaigns){
            if (c.Cloned_Campaign__c == False && c.Status == 'Complete' && 
                c.StartDate != null && (c.Total_Rollup__c > 0 || c.Baby_Items_Rollup__c > 0)){
                Campaign campaignNew = new Campaign();
                campaignNew.Name = c.Name + ' Cloned';
                campaignNew.Status = 'Planned';
                campaignNew.RecordTypeId = recordTypeId;
                campaignNew.StartDate = c.StartDate.addYears(1);
                campaignNew.OwnerId = c.OwnerId;
                campaignNew.Type = c.Type;
                campaignNew.Subtype__c = c.Subtype__c;
                campaignNew.Start_Time__c = c.Start_Time__c;
                campaignNew.Primary_Contact__c = c.Primary_Contact__c;
                campaignNew.Primary_Organization__c = c.Primary_Organization__c;
                campaignNew.Public_Drive2__c = 'Yes';
                campaignNew.Toys_Goal__c = c.Toys_Rollup__c;
                campaignNew.Clothes_Goal__c = c.Clothing_Rollup__c; 
                campaignNew.Books_Goal__c = c.Books_Rollup__c;
                campaignNew.School_Art_Supplies_Goal__c = c.School_Art_Supplies_Rollup__c;
                campaignNew.Baby_Items_Goal__c = c.Nursery_Items_Rollup__c;
                campaignNew.Baby_Equipment_Goal__c = c.Baby_Gear_Rollup__c;
                campaignNew.Socks_Underwear_Goal__c = c.Socks_Underwear_Rollup__c;   
                campaignNew.Coats_Goal__c = c.Coats_Rollup__c;
                campaignNew.Shoes_Goal__c = c.Footwear_Rollup__c;
                campaignNew.PJ_s_Goal__c = c.PJ_s_Rollup__c;
                campaignNew.Unit_Arts_Crafts_Goal__c = '55 Gal Bag';
                campaignNew.Unit_Baby_Equipment_Goal__c = 'Each';
                campaignNew.Unit_Baby_Item_Goal__c = '55 Gal Bag';
                campaignNew.Unit_Books_Goal__c = '55 Gal Bag';
                campaignNew.Unit_Clothes_Goal__c = '55 Gal Bag';
                campaignNew.Unit_Coats_Goal__c = '55 Gal Bag';
                campaignNew.Unit_Hats_Gloves_Mittens_and_Goal__c = '55 Gal Bag';
                campaignNew.Unit_Mixed_Goal__c = '55 Gal Bag';
                campaignNew.Unit_PJs_Goal__c = '55 Gal Bag';
                campaignNew.Unit_Toys_Goal__c = '55 Gal Bag';
                campaignNew.Unit_Shoes_Goal__c = '55 Gal Bag';
                campaignNew.Unit_Socks_Underwear_Goal__c = '55 Gal Bag';
                c.Cloned_Campaign__c = True;
                toClone.add(campaignNew);
                toUpdate.add(c);
            }
        }
    try {
        insert toClone;
    } catch (system.DmlException e){
        system.debug(e);
        for (Campaign campaign : toClone) {
            campaign.adderror('There was a problem cloning this campaign. Please copy and paste the following error and send it to your administrator' + e);
        }
    }
}

public void handleBeforeInsert(Campaign[] newCampaigns) {
    cloneCampaign(newCampaigns);
}

public void handleBeforeUpdate(Campaign[] newCampaigns) {
    cloneCampaign(newCampaigns);
}
}

Again, thanks in advance!

share|improve this question
1  
I guess 'better' is subjective, but you could use the .clone() method to make a copy (rather than instantiate a new) and modify only the fields you wish to...also, you don't need to use the 'toUpdate' list as this is a before trigger so simply updating the field values will work. –  Phil Hawthorn Aug 19 at 12:05
    
The clone would be a good idea here - only thing I'm thinking is that I'm going to need to modify a bunch of fields either way (just found out about 10ish more fields that need to get changed in the clone) - but I didn't even know this method existed, so thanks! –  Zachery Tapp Aug 19 at 15:43

2 Answers 2

up vote 4 down vote accepted

You may want to consider using more of the Trigger Best Practices. Specifically, consider:

One Trigger Per Object

A single Apex Trigger is all you need for one particular object. If you develop multiple Triggers for a single object, you have no way of controlling the order of execution if those Triggers can run in the same contexts. Many times, the order of execution doesn’t matter but when it does matter, it’s nearly impossible to maintain proper flow control. A single Trigger can handle all possible combinations of Trigger contexts.

Logic-less Triggers

Another widely-recognized best practice is to make your Triggers logic-less. That means, the role of the Trigger is just to delegate the logic responsibilities to some other handler class. There are many reasons to do this. For one, testing a Trigger is difficult if all of the application logic is in the trigger itself. If you write methods in your Triggers, those can’t be exposed for test purposes. You also can’t expose logic to be re-used anywhere else in your org. Good old OO principles tell us that this is a bad practice. And to top it all off, cramming all of your logic into a Trigger is going to make for a mess one day. To remedy this scenario, just create a handler class and let your Trigger delegate to it.

Context-Specific Handler Methods

One best-practice that I have picked up is to create context-specific handler methods in my Trigger handlers. In the above example, you’ll see that I’ve created a specific handler method just for after insert. If I were to implement new logic that ran on after update, I’d simply add a new handler method for it. Again, this handler method would be in the handler class, and not the Trigger.

share|improve this answer
    
One question I have around using handler classes (and this concept is new to me). Let's say there's already a bunch of legacy code not written this way - because there is. In order to make this one trigger work, would I need to bring all that legacy code into the Handler class? It's definitely something I want to do as I think this is a much cleaner way to do all this - but I'm wondering if that all needs to happen right now in order to get this to work. –  Zachery Tapp Aug 30 at 15:03
1  
@ZacheryTapp When refactoring old Trigger logic, it isn't a requirement to refactor the entire set of Triggers at once. If you have the time, it is definitely worth it. However, if nothing else, it would be better to implement this practice going forward and refactor only when you need to modify code from the other triggers. This pushes the code base in the right direction without hindering progress now. –  Jesse Altman Aug 30 at 20:28

This looks like it should work. There are a few small things I might do differently:

  • You are swallowing the exception if the insert doesn't work. If the new Campaign doesn't get created, the user will get no message, and the Clone Campaign field will be set to TRUE, so it won't try again. Depending on how you want this to be handled, you might want to do an addError() on the triggering Campaigns that caused problems, or you may just want to unset the Clone Campaign field in that case. The simplest handling would be to just throw an exception on the whole transaction.
  • You are adding 365 days to the date field. Instead, I suggest using .addYears(1), which will account for leap years.
  • Your IF statement doesn't use any parentheses, so it's hard to say whether this is the logic you want to happen. (A && B && (C || D)) makes it clear where the OR goes.
  • You didn't include a space in the name of the new Campaign, so the new name will look like "Old CampaignCloned"
share|improve this answer
1  
Operator precedence is indeed important. && is always evaluated before ||, which is rarely the intended use. I've never not had to use parenthesis when combining these two operators. –  sfdcfox Aug 19 at 15:32

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.