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);
 }
}
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 '14 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 '14 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 '14 at 9:51
  • And your reference a is undefined.
    – Keith C
    Dec 12 '14 at 10:00
  • My Bad. Let me improve. :) 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 :). Dec 12 '14 at 10:18

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.