Salesforce Stack Exchange is a question and answer site for Salesforce administrators, implementation experts, developers and anybody in-between. Join them; it only takes a minute:

Sign up
Here's how it works:
  1. Anybody can ask a question
  2. Anybody can answer
  3. The best answers are voted up and rise to the top

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.

share|improve this question
up vote 4 down vote accepted

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?

share|improve this answer
    
thought-provoking .. – cropredy Feb 27 at 2:55

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');
}
share|improve this answer
    
No. My question is more on the should I collect all the accounts which are invalid and add error for those accounts in a separate loop or can I just add error for the invalid accounts in the first loop itself. My fear is that if we are importing 200 accounts using dataloader and out of which first account doesn't satisfy the criteria does the trigger process remaining 199 accounts if I use the first approach?? – javanoob Feb 26 at 21:50
    
@javanoob That depends 100% on the calling context, which defines allOrNone behavior. – Adrian Larson Feb 26 at 21:53
1  
+1 for legibility; I like the concept of the Selector framework myself, except that it's far bulkier than it should be. – sfdcfox Feb 26 at 23:12
2  
@AdrianLarson I love the folk over at Blue Wolf, and it was the Selector package that initially got me in to writing my own. Here's the "select" pattern in one version of my internal library: Account[] records = (Account[])Data.filterRecords(Trigger.new, new Data.FieldEqualsFilter(Account.Some_Flag__c, true)); If you're interested, I might post a polished version on my blog shortly. – sfdcfox Feb 26 at 23:29
1  
@sfdcfox Do want. Just started reading it. :) – Adrian Larson Feb 26 at 23:30

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).

share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.