How to Write This Trigger Without Nested Loop Using Map

Question

Trigger..................

trigger AccountTriggerZip on Account (before update) {
    if(Trigger.isBefore){
        if(Trigger.isUpdate){
            AccountTriggerZipHelper.checkZip(Trigger.new);
        }
    }
}

Helper Class..............

public class AccountTriggerZipHelper {
    public static void checkZip(List<Account> updatedAccountList){
        Account accountToUpdate;
    List&lt;Account&gt; accountToUpdateList = new List&lt;Account&gt;(); // contains accounts

    List&lt;Account&gt; acc = [SELECT id, Out_of_Zip__c, Billingpostalcode,
                         (SELECT id,Mailingpostalcode 
                                    FROM Contacts)
                                FROM Account WHERE id IN :updatedAccountList];

    for(Account a : acc){
            System.debug('Iterate Over Accounts');
            for(Contact c : a.Contacts){
                System.debug('Iterating over contacts');
                if(c.MailingPostalCode == a.BillingPostalCode) {

                }else{
                   accountToUpdate = new  Account(Id = a.Id, Out_of_Zip__c=True);
                }
            }
        }

    if(accountToUpdateList.size()&gt;0){

        update accountToUpdateList;
    }
}

}

Answer 1

First thing you should do is add a filter to check if your BillingPostalCode has actually changed. Don't consume governors if you have nothing to process. This pattern is very common.

public static List<Account> hasBillingPostalCodeChanged
    (List<Account> records, Map<Id, Account> oldMap)
{
    List<Account> output = new List<Account>();
    for (Account record : records)
        if (record.BillingPostalCode != oldMap.get(record.Id).BillingPostalCode)
            output.add(record);
    return output;
}

Next thing you should do is add a Formula Field on your Contact object to detect if the postal code differs from the Account, which will simplify your code.

MailingPostalCode != Account.BillingPostalCode

Next, you can query child contacts only and use this new formula as a filter in your WHERE clause. You can use a bit of aggregate query magic to make this code extremely simple and fast.

public static void checkZip(List<Account> records)
{
    Set<Id> accountIdsWithMismatch = new Map<Id, AggregateResult([
        SELECT AccountId Id FROM Contact
        WHERE AccountId IN :records
        Postal_Code_Mismatch__c = true
    ]).keySet();
    for (Account record : records)
        if (accountIdsWithMismatch.contains(record.Id))
            record.Out_Of_Zip__c = true;
}

A few other important things to note about the above:

• Proper architecture for field updates is to put them in a before context, where they do not require additional DML.
• When you do need to perform DML, checking size is unnecessary.

Now, you just need to make sure your trigger actually applies the filter:

AccountTriggerZipHelper.checkZip(
    AccountTriggerZipHelper.hasPostalCodeChanged(
        trigger.new, trigger.oldMap
    )
);

Note that I renamed AccoounTriggerZipHelper to AccountTriggerZipHelper. I strongly suggest you fix the typo before deploying to production. You should also strongly consider renaming trigger AccoounTriggerZip to simply trigger Account as you should only have one trigger per object, and you may call other services in the future.

Answer 2

Nested loops are not always evil. In cases like this where you're working with nested data, they're effectively unavoidable. Specifically for working with parent-child subqueries, accessing child records via a nested loop is the safest way to do so.

As long as you keep the inner loop short, and keep dml and queries outside of all loops, there's really no pressing issue.

The thing you're trying to avoid are nested loops that look like this:

for(Account acct :myAccounts){
    for(Contact cont :myContacts){
        if(cont.AccountId == acct.Id){
            // do work
        }
    }
}

Your case here somewhat resembles that anti-pattern, but I'd say it's ok (not great, but not terrible) because you're only iterating over the Contacts related to the Account in the outer loop (as opposed to iterating over all contacts every time).

If you want to make an improvement, you could consider creating a formula field on Contact to do this comparison. You would then be able to filter your parent-child subquery using it (so that you only return related contacts if the postal code does not match).

One last note: wrapping dml statements inside of an if(collection.size() > 0) is useless and should be removed. Salesforce is smart/kind enough to not perform DML (and not increment your governor limit counter for DML statements) if a collection is empty (i.e. size 0)