Csdrhrt

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');

                    }

                }

            }

}

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');

        }

    }

}

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.

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.