Trigger - Update minimum and maximum salary on contacts using Trigger

Question

Requirement: I want to update minimum and maximum salary of Contacts using trigger.

For that, I have created custom fields on Account object named as MaxSalary and MinSalary.

I am like to know, My approach is right in following code for my requirement.

Suggestions are really helpful,I am new to triggers.

My Trigger:

trigger MaxMinSalaryCal on Contact(after insert,after update,after delete) {
    Set<Id> accids= new Set<id>();
    List<Account> updateList=new List<Account>();
    if(Trigger.isInsert|| Trigger.isUpdate){
        for(Contact c:trigger.new)
        {
            accids.add(c.accountid);
        }    }
    if(Trigger.isDelete){
        for(Contact c1:Trigger.old){
            accids.add(c1.accountid);
        }
    }
    for(AggregateResult result:[select accountid accid,max(salary__C) maxSalary,min(salary__c) minsalary from contact where accountid in: accids group by accountid])               
    {
        Account a= new Account();
        a.MaxSalary__c=(Decimal) result.get('maxSalary');  
        a.MinSalary__c=(Decimal)result.get('minsalary');
        a.id=(Id)result.get('accid');
        updateList.add(a);                                       
    }
    update updateList;   
}

Answer 1

This looks mostly correct.

• Your code is bulkified (no SOQL or DML inside of a loop)
• You operate on collections
• You're taking deletion into account
• You're using GROUP BY in your query to do the heavy lifting

You've missed 3 subtle cases from what I can see.

1. If a Contact has its AccountId updated, it's possible that you would need to recalculate the Account that just lost the Contact (in addition to the Account that just gained the Contact)
2. When you remove the final Contact from an Account, the Account's Min and Max salaries will not be updated
3. If all Contacts for a given Account have a null value in the min salary or max salary fields, you'd get null from your aggregate function (may or may not also be an issue if at least one Contact for a given Account has a null value)

Taking care of the first issue is as simple as checking the AccountId value in the trigger.oldMap context variable (available for update and delete contexts).

if(Trigger.isUpdate){
    Contact oldValues = Trigger.oldMap.get(contactRec.Id);
    if(oldValues.AccountId != contactRec.AccountId){
        accIds.add(oldValues.AccountId);
    }
}

Since you're using a Set to store Account Ids though, you could get away with simply adding the old Account Id into the set (instead of checking to see if there is a difference). I'll usually put a mySet.remove(null); afterwords if I think thhere is a possibility of having a null in the set (which would happen here if your Contact was not previously related to an Account, or is being removed from its current Account without being related to a new one).

Taking care of the second issue is also pretty simple. Instead of relying on the query to return results for every Account, assume that every Account has no Contacts and set a default value. This default value will be overwritten by your existing code if it has at least one Contact.

// Using a Map here instead of a List to make it easy to override the default values
Map<Id, Account> accountsToUpdateMap = new Map<Id, Account>();

// Omitting your code to gather Account Ids

// Generate the default values
for(Id acctId :acctIdSet){
    accountsToUpdateMap.put(acctId, 
        // You can set fields using name-value pairs when using the
        //   constructor for an SObject
        // Can save a little typing (compared to doing rec.field1 = value)
        new Account(
            Id = acctId,
            Min_Salary__c = 0,
            Max_Salary__c = 0
        )
    );
}

for(/* your query to get the max and min salary here */){
    // This will look very similar to setting the default values
    // You don't need to worry about if something exists in the map already
    // If the AccountId _does_ exist in the map, you'll simply overwrite
    //   the previous value
    accountsToUpdateMap.put((Id)result.get('Id'),
        new Account(
            Id = (Id)result.get('Id'),
            Min_Salary__c = (Decimal)result.get('minSalary'),
            // and so on
        )
    );
}

// We cannot perform DML on a Map directly.
// It needs to be a single SObject or a List of SObjects
// The .values() method returns a List of the value type of a map, so
//   <our Map>.values() returns a List<Account>
update accountsToUpdateMap.values();

Fixing issue #3 usually comes down to performing null checks. I use the ternary operator for this.

Instead of
(Decimal)result.get('max');

You could write

// The ternary operator is <condition> ? <value if true> : <value if false>
// Just a shorter way to write simple if/else statements (though both options
//   need to provide a result of the same type)
result.get('max') == null ? 0.0 : (Decimal)result.get('max');