Primary user Checkbox ticked for Contact with highest Commission

Question

I'm new to Salesforce and Apex. I need help setting a trigger which checks Contacts under a specific Account. The Contact with the highest Total Commission value is marked as Primary user by ticking the checkbox (and the other contacts are unticked).

Currently I have this code and it seems to work but not always, sometimes it doesn't tick the Contact with highest commission and I dont know why.

trigger SetPrimary on Commission__c (after insert, after update, after delete) {
    //This gets all the account ids associated with the Commission records
    Set<Id> accountIds = new Set<Id>();
    for (Commission__c c : Trigger.new) {
        accountIds.add(c.Account__c);
    }

    //This should query the database for all the Contact records related to the accounts with commission records
     Map<Id, Contact> contacts = new Map<Id, Contact>([SELECT Id, AccountId, Total_Commission__c FROM Contact WHERE AccountId IN :accountIds]);

    // Loops through each Account
    for (Id accountId : accountIds) {
        // Finds the Contact with the highest Total Commission
        Contact primaryContact = null;

        for (Contact c : contacts.values()) {
            if (c.AccountId == accountId) {
                if (primaryContact == null || c.Total_Commission__c > primaryContact.Total_Commission__c) {
                    primaryContact = c;
                }
            }
        }
        
        // Sets the Primary field to true for the primary Contact and false for all others
        for (Contact c : contacts.values()) {
            if (c.AccountId == accountId) {
                c.Primary__c = (c.Id == primaryContact.Id);
            }
        }
    }

    update contacts.values();
}

Total_Commission__c is a field in the Contact object (roll-up summary)

Answer 1

At first blush, I can't see why this wouldn't check the Primary Contact either.

What I can see, though, is that you're using the "bad pattern" for nested loops. I.e.

for(object 1){
    for(object 2){
        if(object1 field == object2 field){
            // do work
        }
    }
}

You can improve your code's ability to scale (and make your life easier) by using a parent-child subquery. This will group the child records (Contact) by their parent record (Account) so that you don't need to check if(c.AccountId == accountId)

trigger SetPrimary on Commission__c (after insert, after update, after delete) {
    //This gets all the account ids associated with the Commission records
    Set<Id> accountIds = new Set<Id>();
    for (Commission__c c : Trigger.new) {
        accountIds.add(c.Account__c);
    }

    // The subquery uses the "child relationship name", which is usually just the
    //   plural of the object name (and you change "__c" to "__r" if it's a custom
    //   lookup or master-detail field)
    // You can also make use of ORDER BY in the subquery to make your life even
    //   easier.
    List<Account> accountsWithContacts = [
        SELECT 
            Id, 
            (SELECT Id, Total_Commission__c FROM Contacts ORDER BY Total_Commission__c DESC)
        FROM Account
        WHERE Id IN :accountIds
    ];

    List<Contact> contactsToUpdate = new List<Contact>();

    for (Account acct :accountsWithContacts) {
        Boolean isPrimary = true;
        for(Contact cont :acct.Contacts) {
            // Thanks to the ORDER BY in the query, the first Contact will
            //   _always_ be the one with the highest total commission

            // Updating something that doesn't need it is mostly a waste of
            //   resources, so check if there is a change happening
            if(cont.Primary__c != isPrimary){
                cont.Primary__c = isPrimary;
                contactsToUpdate.add(cont);
            }

            // flipping isPrimary so that all of the other Contacts will not be
            //   marked as primary (in the subsequent iterations)
            isPrimary = false;
        }
    }

    // If this list ends up being empty, Salesforce is smart enough not to
    //   run the DML (so there's never a need to put this inside of an
    //   if(myList.size() > 0) )
    update contactsToUpdate;
}