Contact trigger to limit an account to not have more than 2 contacts

Question

Yesterday I got rejected from a job interview because the interviewer asked me to write a trigger to limit the number of contacts per account and I wrote exactly the below but he said this won't work and it is not the correct way to write the same. I ran the same and it is working fine I am not able to see any problem with the below code can someone please help me understand from the interview's perspective why he said this is not the right way.

    trigger ContactTrigger on Contact (after insert, before Insert, before update, after update) {

                if(Trigger.operationType == triggerOperation.AFTER_INSERT || Trigger.operationType == triggerOperation.AFTER_UPDATE) {
                    set<Id> IdSet =new set<Id>();
                    for(Contact cot : trigger.new) {
                        if(cot.accountID != null) {
                            IdSet.add(cot.accountId);
                        }
                    }

                    Integer contactListCount = [Select count() from contact where accountID IN: IdSet];

                if(contactListCount > 2) {
                    for(contact cop : trigger.new) {
                        cop.addError('cannot have more than 2 contacts per account');
                    }
                }
            }
}

Answer 1

This trigger is not bulkified. It can't handle more than one account at a time. To fix the problem, an aggregate query would have been the most efficient solution. Here's my version of this trigger:

trigger BlockMoreThan2ContactOnAccount on Contact (after insert, after update, after undelete) {
  Set<Id> accountIds = new Set<Id>();
  for(Contact record: Trigger.new) {
    accountIds.add(record.AccountId);
  }
  accountIds.remove(null);
  Set<Id> morethan2Contacts = new Map<Id, AggregateResult>([
    SELECT AccountId Id
    FROM Contact
    WHERE AccountId = :accountIds
    GROUP BY AccountId
    HAVING COUNT(Id) > 2]).keySet();
  for(Contact record: Trigger.new) {
    if(moreThan2Contacts.contains(record.AccountId)) {
      record.AccountId.addError('You may not have more than 2 contacts per account.');
    }
  }
}

You should always look for opportunities to use aggregate results (such as summing, counting, and finding averages) for performance reasons.

Answer 2

This is the best example to use child soql to handle bulky records.. Please see the Below logic..

trigger ContactTrigger on Contact (after insert, before Insert, before update, after update) {
    if ((Trigger.isInsert || Trigger.isUpdate) && Trigger.isAfter) {
        Set<Id> setofAccountId = new Set<Id>();
        for(Contact objCon : trigger.new)
            setofAccountId.add(objCon.accountid)
        //Get all account contact related values
        Map<Id,Account> mapofAccIdToContacts = new Map<Id,Account>([Select Id,(Select Id From Contacts) From Account Where Id IN : setofAccountId]);
        for(Contact objCon : trigger.new){
            if(mapofAccIdToContacts.containskey(objCon.accountID) && mapofAccIdToContacts.get(objCon.accountID).Contacts.size() > 2)
                objCon.addError('cannot have more than 2 contacts per account');
        }
    }
}

Answer 3

I would recommend creating a developer account to test said code with some debug statements to see what the output is.

Your soql query returns the total number of contacts in each account. This will work if there is only one account in the org, But it won't if there're more.Example: if there are 3 accounts with 4 contacts each, the output of contactListCount will be 12.

Also, in your 2nd loop you are looping again over Trigger.new, even if you did manage to filter out the accounts with 2+ contacts and added them to a new list, you ignored it by running over Trigger.new again and not over the new list.

Answer 4

public class Contactlimit {
    public static void checkInsert(list<Contact> ConAllist){
        list<contact> Conlist = New list<contact>();
         for(Contact c: conAllist){
              if(c.AccountId != Null)
              {Conlist.add(c);}
         }
        if(Conlist.size()>0)
        {checkforConLimit(Conlist);}
    }
    public static void checkUpdate(list<Contact> ConAllist, map<id,Contact> Conoldmap){
        list<contact> Conlist = New list<contact>();
         for(Contact c: conAllist){
              if(c.AccountId != Null && c.AccountId != Conoldmap.get(c.Id).AccountId)
              {Conlist.add(c);}
         }
        if(Conlist.size()>0)
        {checkforConLimit(Conlist);}
    }
     public static void checkforConLimit(list<Contact> Conlist){
        //set of ids of all contacts
        set<id> accids = New set<id>(); 
        for(Contact c: conlist) { accids.add(c.AccountId);}
        if(accids.size()>0){
            //map containing acc id from accid
            Map<id,Account> mapofacc = New Map<Id,Account>([SELECT Id,(SELECT ID from Contacts) From Account where Id IN : accids]);
            for(Contact c: conlist){
                //match trigger contact with its acc id and check size
            if(mapofacc.get(c.AccountId).Contacts.size()>=3)
            {c.addError(System.label.Contact_limit_exceed);}
                                  }     
    }   
    }
}



trigger Abc on Contact (before insert, before update) {
    if(Trigger.isbefore && Trigger.isInsert){
        Contactlimit.checkInsert(Trigger.new);
    }
    if(Trigger.isBefore && Trigger.isUpdate){
        Contactlimit.checkUpdate(Trigger.new,Trigger.oldMap);
    }
    
}