Trigger bulkification when adding errors

Question

Snippet 1:

trigger AccountValidationTrigger on Account(before update){
    for(Account accObj: Trigger.new){
      if(accObj.Some_Flag__c != TRUE){
        accObj.addError('Some flag should be true');  
      }
    }
}

Snippet 2:

trigger AccountValidationTrigger on Account(before update){
    List<Account> errorAcctsList = new List<Account>();
    for(Account accObj: Trigger.new){
      if(accObj.Some_Flag__c != TRUE){
        errorAcctsList.add(accObj);
      }
    }
    for(Account accObj: errorAcctsList){
        accObj.addError('Some flag should be true');
    }
}

Given the above two code snippets, which of the above is a best practice to use in trigger when adding validation logic.

Most of the best practices for trigger bulkification are about reducing number of DML statements but I couldn't find any example on what is the best practice to follow in above situations.

Answer 1

The point of bulkification is to perform an operation (such as a SOQL query) so that a set of records are returned together to help performance. This avoids running into governor limits which in turn are (often) a guard against repeating expensive operations. In the query example, returning 101 records in one request is: one request from the app server to the the database, one query plan generation, one query execution and one response from the database back to the app server. Making 101 separate queries would require all those steps to be repeated 101 times.

There is no need to arrange code executed locally in the app server using similar patterns. Generally simple code that is easily understood is best and Premature Optimization is the root of all evil. So the simpler pattern "snippet 1" is the way to go.

(By the way I'm saying "generally" here because in reality there are rarely "best practices" in the sense that a practice is universally "best". There are often several factors at play and what is best in one situation might not be best in another.)

PS

Before introducing design patterns into your code have a think about some of the pros and cons - see e.g. When should I use—and not use—design patterns?

Answer 2

Fun fact, if you adopt the latter pattern, you can implement a library like Selector to do the filtering for you.

public with sharing class AccountService
{
    public static Select.Filter isFlagged()
    {
        return Select.Field.isEqual(Account.Some_Flag__c, true);
    }
    public static void validateFlaggedAccounts(List<Account> flaggedAccounts)
    {
        for (Account flaggedAccount : flaggedAccounts)
            flaggedAccount.addError('message');
    }
}

Then in your trigger you can just do:

AccountServices.validateFlaggedAccounts(
    AccountServices.isFlagged().filter(trigger.new, trigger.oldMap)
);

One huge advantage of this pattern is it's way easier to test. You can test the positive/negative filter cases very easily, then test your action completely independently. Separation of concerns, ftw!

static final Integer RECORD_COUNT = Limits.getLimitQueries() + 1;
static testMethod void testIsFlagged_Positive()
{
    List<Account> accounts = new List<Account>();
    for (Integer i = 0; i < RECORD_COUNT; i++)
        accounts.add(new Account(Some_Flag__c=true));

    Test.startTest();
        List<Account> results = AccountService.isFlagged().filter(accounts);
    Test.stopTest();

    system.assertEquals(RECORD_COUNT, results.size(), 'message');
}
static testMethod void testIsFlagged_Negative()
{
    List<Account> accounts = new List<Account>();
    for (Integer i = 0; i < RECORD_COUNT; i++)
        accounts.add(new Account(Some_Flag__c=false));

    Test.startTest();
        List<Account> results = AccountService.isFlagged().filter(accounts);
    Test.stopTest();

    system.assertEquals(0, results.size(), 'message');
}

Answer 3

Both will accomplish what you are after, but I tend to prefer Snippet 1 because it simply reduces the number of script statements that need to be evaluated. In Snippet 2 you are inefficiently iterating twice over the records you want to add the error to (as well as creating more memory overhead by creating another list of records to store temporarily).