Trigger handler induced design damage

There is a great series of discussions including videos on the subject of the benefits and costs of test driven development. Part of that discussion is the notion of Test-induced design damage where pursuing one principle – testability – to the exclusion of other principles results in damage to the design.

An area in Salesforce where I see risk of damage is the pursuit of this:

Another widely-recognized best practice is to make your Triggers logic-less. That means, the role of the Trigger is just to delegate the logic responsibilities to some other handler class.

quoted from Trigger Frameworks and Apex Trigger Best Practices and often paraphrased. (The term “best practice” is sometimes used to give an idea unmerited gravitas.)

There can be benefits in using separate classes, typically where the logic being implemented has enough complexity to require multiple methods and static constants. Or if a choice has been made to use a carefully selected trigger framework (and not an unproven local invention). And a mixed model – where some code is in the trigger and other parts are moved to classes as needed – also works.

But there are costs:

  • A reduction in cohesion: the logic has moved to a class that is separate from the execution context that includes a number of subtleties as documented in Trigger Context Variables.
  • Over time as more logic is added, the chances of queries being repeated in separate handler class methods or separate handler classes goes up.
  • Some naming challenges: what should the handler class, methods and method parameters be called? And the clutter of more top level classes in an org.
  • Should the test relate to the trigger (the specific context the logic has to run in) or the handler class (with no assumed context)?
  • A risk of failure to bulkify. A handler class that has methods that accept single values rather than collections is an obvious red flag. But with the trigger context not so directly in view when writing the handler class and logic running several method calls down, bulkification can be missed.

Code in a trigger is just like any other code and should be written to Do The Simplest Thing That Could Possibly Work. If the problem being solved can be solved cleanly by keeping the code in the trigger then do that. If not, introduce a class to solve the part of the problem that needs a class. Be prepared to refactor when logic is added later.

The Apex Code Best Practices documentation says “Bulkify your Helper Methods” – if you have any. Those helper methods and classes are not assumed.

Don’t damage your code base by introducing a pattern where the benefits are outweighed by the costs.

Constraining a detail field depending on a master field in a trigger

I’ve implemented this pattern several times with slight variations and each time had to do some head scratching to get it right. So here is the pattern expressed in somewhat abstract terms. The aim is to disallow a change to a Detail object field if its Master object has a field of a particular value. In my case these have been status fields.

Here is the trigger:

trigger DetailBeforeTrigger on Detail__c (before insert, before update) {
    // Create a map of the master object id to the detail objects
    Map<Id, List<Detail__c>> masterToDetails = new Map<Id, List<Detail__c>>();
    for (Detail__c newDetail : {
        Detail__c oldDetail = Trigger.oldMap != null ? Trigger.oldMap.get(newDetail.Id) : null;
        String oldStatus = oldDetail != null ? oldDetail.Status__c : null;
        String newStatus = newDetail.Status__c;
        // Only add where the field has changed and to the specific value
        if (oldStatus != newStatus && newStatus == 'Approved') {
            List<Detail__c> ds = masterToDetails.get(newDetail.Master__c);
            if (ds == null) {
                ds = new List<Detail__c>();
                masterToDetails.put(newDetail.Master__c, ds);
    // Add an error to the detail objects where the master field has a particular value
    if (masterToDetails.keySet().size() > 0) {
        for (Master__c m : [select Id, Status__c from Master__c where Id in :masterToDetails.keySet()]) {
            if (m.Status__c == 'Closed') {
                for (Detail__c d : masterToDetails.get(m.Id)) {
                    d.Status__c.addError('Detail Status cannot be "Approved" when the Master Status is "Closed"');

This code illustrates:

  • Checking if a field value has changed and only doing work if it has
  • Holding the 1:Many information in a Map
  • Ensuring only one SOQL query is done whether there is one Detail object or many
  • Reporting the error on a field so that it is clearly presented in the default UI