Trigger Best Practices

Question

How would you rewrite the following trigger to follow best practices re: bulkification, etc?

trigger Order_PopulateGallery on RH_Order__c (before insert, before update){

List<Account> accs = new List<Account>();

for(RH_Order__c ord : Trigger.new){
    if(Trigger.isInsert){
        if(ord.Gallery_Number__c != null){
            accs = [Select Id from Account where Gallery__c = :ord.Gallery_Number__c limit 1];
                if(accs.size() != 0){
                    ord.Gallery__c = accs[0].Id;
                }
                else{
                    ord.addError('  Please enter valid Gallery number.');
                }
        }       
    }
    else{
        if(ord.Gallery_Number__c != null){
            RH_Order__c oldOrd = Trigger.oldMap.get(ord.Id);
            if(oldOrd.Gallery_Number__c != ord.Gallery_Number__c){
                accs = [Select Id from Account where Gallery__c = :ord.Gallery_Number__c limit 1];
                if(accs.size() != 0){
                    ord.Gallery__c = accs[0].Id;
                }
                else{
                    ord.addError(' Please enter valid Gallery number.');
                }
            }       
        }
        else{
            ord.Gallery__c = null;
        }                
    }
}
}

Answer 1

First, you should try to follow the DRY pattern; by moving some logic around, we can reduce your code by almost half:

trigger Order_PopulateGallery on RH_Order__c (before insert, before update){

    List<Account> accs = new List<Account>();

    for(RH_Order__c ord : Trigger.new){
        if(ord.Gallery_Number__c == null) {
            ord.Gallery__c = null;
        } else
            if(Trigger.isInsert || Trigger.oldMap.get(ord.Id).Gallery_Number__c != ord.Gallery_Number__c) {
                accs = [Select Id from Account where Gallery__c = :ord.Gallery_Number__c limit 1];
                if(accs.size() != 0){
                    ord.Gallery__c = accs[0].Id;
                }
                else{
                    ord.addError('  Please enter valid Gallery number.');
                }
            }
    }
}

Second, we use the Aggregate-Query-Update pattern to bulkify your trigger:

trigger Order_PopulateGallery on RH_Order__c (before insert, before update){
    Set<String> galleryNumbers = new Set<String>();
    Map<String, Id> galleryIds = new Map<String, Id>();

    // Aggregate
    for(RH_Order__c ord: Trigger.new) {
        if(ord.Gallery_Number__c != null && (Trigger.isInsert || Trigger.oldMap.get(ord.Id).Gallery_Number__c != ord.Gallery_Number__c)) {
            galleryNumbers.add(ord.Gallery_Number__c);
        }
    }

    // Query
    if(!galleryNumbers.isEmpty()) {
        for(Account record: [SELECT Gallery__c FROM Account WHERE Gallery__c IN :galleryNumbers]) {
            galleryIds.put(record.Gallery__c, record.Id);
        }
    }
    // Update
    for(RH_Order__c ord: Trigger.new) {
        if(ord.Gallery_Number__c == null) {
            ord.Gallery__c = null;
        } else
            if(Trigger.isInsert || Trigger.oldMap.get(ord.Id).Gallery_Number__c != ord.Gallery_Number__c) {
                if(galleryIds.get(ord.Gallery_Number__c) == null) {
                    ord.addError('Please enter valid Gallery number.');
                } else {
                    ord.Gallery__c = galleryIds.get(ord.Gallery_Number__c);
                }
            }
    }
}

Note that bulkification almost always results in slightly longer code, simply because of the extra variables that are necessary.