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); } }
Improve this question

2 Answers 2

Reset to default
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; } }
Improve this answer
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? Commented 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. Commented 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; } } }
Improve this answer
4
  • Note that item 4 is not a desirable change - see Any reason to skip DML on empty lists?. Commented Dec 12, 2014 at 9:51
  • And your reference a is undefined. Commented Dec 12, 2014 at 10:00
  • My Bad. Let me improve. :) Commented 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 :). Commented Dec 12, 2014 at 10:18

You must log in to answer this question.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.