1

Recently I created an after insert trigger to update a field in my sponsorship object. it works so far in sandbox and has passed the unit test that I have created and has 100% code coverage. here is the code for the trigger:

trigger Donation_Updater on Opportunity (after insert) {
    For(Opportunity Don : Trigger.New)
    {
        if(Don.Sponsorship_Contract__c != null)
        {
            sponsorships__c spon =  [SELECT  Id FROM sponsorships__c WHERE Id = :Don.Sponsorship_Contract__c LIMIT 1];
    spon.Donation__c = Don.Id;
            update spon;
        }
        }
}

The question that I have is do you guys think this trigger is ready to be ported to production? The only thing i can think of is possibly replacing the SQOL statement but I'd rather keep it in if at all possible,any other suggestions?

  • 1
    You absolutely need to move that query outside of the for loop and make this bulk safe. Presumably your unit test also doesn't operate on 200 Opportunities and their related sponsorships or you would have seen the exception that gets thrown by this pattern. – Mark Pond Mar 13 at 17:25
6

No, you have not properly bulkified your code. Bulkification 101's most important lesson is to aggregate filter values, query before your loop and put records into a Map, and update values after your loop.

Set<Id> sponsorshipIds = new Set<Id>();
for (Opportunity record : trigger.new)
{
    sponsorshipIds .add(record.Sponsorship_Contract__c);
}
Map<Id, Sponsorships__c> sponsorships = new Map<Id, Sponsorships__c>([
    SELECT ... FROM ... WHERE Id IN :sponsorshipIds
]);

However, in this case, you do not need any query whatsoever. You can update a record as long as you know its Id. Therefore, your code can be simplified to:

List<Sponsorships__c> sponsorships = new List<Sponsorships__c>();
for (Opportunity record : trigger.new)
{
    sponsorships.add(new Sponsorships__c(
        Id = record.Sponsorship_Contract__c,
        Donation__c = record.Id
    ));
}
update sponsorships;

Other things you should strongly consider are error handling and a handler pattern. Another thing to note is you should always use singular names for your objects. If it's not too late, change Sponsorships__c to Sponsorship__c.

| improve this answer | |
  • +1 for Another thing to note is you should always use singular names for your objects - this drives me nuts – cropredy Mar 14 at 0:16
3

I wouldn't say this trigger is production ready. Couple of things I can recommend are.

  • Move the SOQL and DML outside for loop.

  • Add error handling for SOQL and update.

  • Do not write logic inside triggers.

  • Read this document and Force.com enterprise architecture book if you have some time.

  • Add comments about the functionality.(Personal choice)

| improve this answer | |
  • sorry for the late response but I have a question. why should i not include logic within my trigger? without my if statement an error is thrown if i try to make a donation that doesn't have a sponsor. – Apex_rookie Mar 18 at 15:46
  • Well, simply put code should be well organized. Firstly, The logic in the trigger is not reusable. Secondly, you cannot implement record sharing in triggers. As you organisation becomes code heavy, the code should be well maintained and usually to do that we introduce layering. Think of a scenario where you have you logic in trigger and different developers want to modify the same trigger. They can either take turns and wait for each other or they can work on separate classes which are called from logic less trigger without worrying what changes trigger is going to make to their input. – manjit5190 Mar 18 at 16:37

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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