Can this Trigger be improved?

Question

I am very new to both programming and Apex. I have written one Trigger on account to update contact SLA fields whenever the SLA is updated as 'Bronze' on account. My code coverage is 100%. Please advise if this code can be improved further and optimised as I have written nested for loops.

trigger MySLA on Account (after Update) {

if(Trigger.isAfter)
{
    for(account a:Trigger.new)
    {
      for(account ao:Trigger.old)
      {
    if(a.SLA__c == 'Bronze' && ao.SLA__c != 'Bronze')
    {
    list<contact> conlist = new list<contact>();
    list<Contact> cons = [select SLA_Serial_Number__c,SLA__c,SLA_Expiration_Date__c from contact where accountid IN :Trigger.new];
    for(contact c:cons)
    {
      c.SLA__c = 'Bronze';
      c.SLA_Expiration_Date__c = a.SLAExpirationDate__c;
      c.SLA_Serial_Number__c = a.SLASerialNumber__c;
      conlist.add(c);
    }
        update conlist;
    }
      }
    }
}

}

@isTest
public class testSLAupdate {

 private static testmethod void testSLAContactUpdate()
 {
     account a = new account(name='abc', SLA__c='Gold');
     insert a;
     contact c = new contact(lastname='Smith',firstname='John',accountid=a.id);
     insert c;
     a.SLA__c = 'Bronze';
     update a;
     system.assertEquals('Bronze', c.SLA__c);
     System.assertEquals(a.SLAExpirationDate__c, c.SLA_Expiration_Date__c);
     System.assertEquals(a.SLASerialNumber__c, c.SLA_Serial_Number__c);
 }
}

Amit Bangad · Answer 1 · 2014-12-12T10:15:08.903

2

Class can be improved a lot :

Issues :
1. Query in For loop
2. Added cyclomatic complexity
3. Bad Indentation
4. Empty check before DMLs

trigger MySLA on Account (after Update) {
    Map<Id,Account> newAccountMap = trigger.NewMap;
    Map<Id,Account> oldAccountMap = trigger.OldMap;
    Set<Id> accIds = new Set<Id>();
    for(Account a : newAccountMap.values()){
        if(a.SLA__c == 'Bronze' && oldAccountMap.get(a.Id).SLA__c != 'Bronze'){
            accIds.add(a.Id);
        }
    }
    if(!accIds.IsEmpty()){
        list<contact> conlist = new list<contact>();
        list<Contact> cons = [select AccountId, SLA_Serial_Number__c,SLA__c,SLA_Expiration_Date__c from contact where accountid IN :accIds];
        for(contact c:cons)
        {
            c.SLA__c = 'Bronze';
            c.SLA_Expiration_Date__c = newAccountMap.get(c.AccountId).SLAExpirationDate__c;
            c.SLA_Serial_Number__c = newAccountMap.get(c.AccountId).SLASerialNumber__c;
            conlist.add(c);
        }
        if(!conlist.IsEmpty()){
            update conlist;
        }
    }
}

edited Dec 12 '14 at 10:15

answered Dec 12 '14 at 09:38

Amit Bangad

3,890
1
15
19

Note that item 4 is not a desirable change - see Any reason to skip DML on empty lists?. – Keith C Dec 12 '14 at 09:51
And your reference a is undefined. – Keith C Dec 12 '14 at 10:00
My Bad. Let me improve. :) – Amit Bangad Dec 12 '14 at 10:12
My Bad. I have been too lazy to test it out. It must be fine now. Thanks for the reference :). – Amit Bangad Dec 12 '14 at 10:18

score 1 · Accepted Answer · answered Dec 12 '14 at 10:21

Slight trim down of Amit's solution:

trigger MySLA on Account (after Update) {

    Set<Id> accountIds = new Set<Id>();
    for (Account a : Trigger.new) {
        if (a.SLA__c == 'Bronze' && a.SLA__c != Trigger.oldMap.get(a.Id).SLA__c) {
            accountIds.add(a.Id);
        }
    }

    if (!accountIds.isEmpty()) {
        List<Contact> contacts = [
                select Id, AccountId
                from Contact
                where AccountId in :accountIds
                ];
        for (Contact c : contacts) {
            Account a = Trigger.newMap.get(c.AccountId);
            c.SLA__c = a.SLA__c;
            c.SLA_Expiration_Date__c = a.SLAExpirationDate__c;
            c.SLA_Serial_Number__c = a.SLASerialNumber__c;
        }
        update contacts;
    }
}

Hi Keith, isn't necessary to add each c as element to contacts list inside the for loop? Then update contacts list outside the for loop? — Jarvis, Dec 12 '14 at 11:30
@Bennie No, the original list returned by the query can have the objects in it modified and that list can be updated. The only time you would need a second list if say only some items were to be updated where you would only copy across references to some of the items. — Keith C, Dec 12 '14 at 11:58

Can this Trigger be improved?

2 Answers2