0

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);
 }
}

2 Answers 2

1

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;
    }
}
2
  • 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, 2014 at 11:30
  • 1
    @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, 2014 at 11:58
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;
        }
    }
}
4
  • Note that item 4 is not a desirable change - see Any reason to skip DML on empty lists?.
    – Keith C
    Dec 12, 2014 at 9:51
  • And your reference a is undefined.
    – Keith C
    Dec 12, 2014 at 10:00
  • My Bad. Let me improve. :) Dec 12, 2014 at 10:12
  • My Bad. I have been too lazy to test it out. It must be fine now. Thanks for the reference :). Dec 12, 2014 at 10:18

You must log in to answer this question.

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