+ Start a Discussion
shaqib ahmadshaqib ahmad 

when a new contact is created for a existing account then set contact otherphone as account phone.

I am getting an error System.LimitException: Too many SOQL queries: 101 .
my code.
trigger contactaccountrelation on Contact (before insert) {
 
       list<account> acc = [select id, name from account];
    for(account a : acc){
    for(contact con : [select lastname, accountid from contact where accountid =: a.id]){
        con.OtherPhone = a.Phone;
    }
    }
}
Best Answer chosen by shaqib ahmad
Sachin_PuraswaniSachin_Puraswani
Hi Shaqib,
Please try the below code
trigger contactAccountRelation on Contact (before insert) {
    List<Account> acct;
   
    for(Contact c:Trigger.new){
        if(c.OtherPhone!=null)
            break;
        else{
            acct=[Select Id,Phone from Account where Id =: c.AccountId];
            for(Account a1:acct){
                c.OtherPhone=a1.Phone;
            }
        }
    }  
}

Let me know the feedback. Thanks
 

All Answers

Abdul KhatriAbdul Khatri
Hi Shaqib

Please use the below code
 
trigger contactaccountrelation on Contact (before insert) {
    
    Map<Id, Contact> contactMap = new Map<Id, Contact>();
    for(Contact contact : Trigger.new)
    {
        if(!String.isBlank(contact.AccountId))
        {
            contactMap.put(contact.AccountId, contact);
        }
    }
    
    if(contactMap.isEmpty()) return;
    
    Map<Id, Account> accountMap = new Map<Id, Account>([SELECT Id, Phone FROM Account WHERE Id = :contactMap.keySet()]);
    
    for(Id idAccount : contactMap.keySet()){
        
        contactMap.get(idAccount).OtherPhone = accountMap.get(idAccount).Phone;
    }
       
}

 
Sanjay Bhati 95Sanjay Bhati 95
Hi shaqib,

Please use below code for best practice.
 
trigger contactaccountrelation on Contact (before insert) {
      Set<Id> accIdSet = new Set<Id>();
      
      for(Contact con : trigger.new){
           if(String.isNotBlank(con.AccountId)){
                accIdSet.add(con.AccountId);
           }
      }

     if(accIdSet.size() > 0){
          Map<Id,Account> accMap = new Map<Id,Account>{'Select Id,Phone From Account where id In:accIdSet'};
     
          for(Contact con : trigger.new){
               if(con.AccountId != null && accMap.containskey(con.AccountId)){
                      if(accMap.get(con.AccountId).Phone != null){
                              con.OtherPhone = accMap.get(con.AccountId).Phone;
                      }
               }
          }
     


     }
     
}

Please let me know if have any doubt.
Sachin_PuraswaniSachin_Puraswani
Hi Shaqib,
Please try the below code
trigger contactAccountRelation on Contact (before insert) {
    List<Account> acct;
   
    for(Contact c:Trigger.new){
        if(c.OtherPhone!=null)
            break;
        else{
            acct=[Select Id,Phone from Account where Id =: c.AccountId];
            for(Account a1:acct){
                c.OtherPhone=a1.Phone;
            }
        }
    }  
}

Let me know the feedback. Thanks
 
This was selected as the best answer
vishal-negandhivishal-negandhi

Hi Shaqib, 

 

As suggested by everyone else, you need to optimize your code. 

What is wrong in your code? 

- You're doing an SOQL query inside a for-loop, you should never do that. 

 

One more point I'd like to add, since your requirement is just to copy data from Account to Contact, you could do this using one of the out of the box features such as Process builder, flow or workflow and avoid writing code for this.

Hope this helps and gives you better clarity.

 

Thanks,

Vishal

Andrew GAndrew G
and the answer selected as best answer also contains a SELECT inside a FOR loop - such sadness

@sanjay Bhati - your answer is the best imho

regards
Andrew
Abdul KhatriAbdul Khatri
Hi, 

I completely agree with you @Andrew, this is such a sadness.

Nothing wrong with @sanjay Bhatti code but would like to provide some insight between the difference of code of his and mine so we have more clarity. My Code contains the following feature.
  • Less Line of Code
  • Making check for AccountId only once.
  • My second loop only consider eligible contacts eliminating contacts without AccountId (this make it some optimize).
Thanks.
 
Andrew GAndrew G
Hi Abdul.

Yes, your code is also well written.  It shows a good solution that is bulkified.  And I have no real issue with it.   

But sometimes when we say we like some things more, it's like saying "I like Mexican Cola more than I like Coke Cola"

It comes down to personal preference. If we are to be specific, I (personally) don't like to use a return to exit my code in the middle of the run.  I prefer to let the code "read" to completion, and by that I mean I like to read the code to the end without stepping out of the method. 

There is nothing wrong with using a construct like if(contactMap.isEmpty()) return;   I just prefer to use something like if(!contactMap.isEmpty()) //run code;    In that way, if the contactMap is empty, it skips over the code and jumps to the end.
Some people would argue that by using a negative of a negative check is confusing, but it's just my preference.

happy coding.
Andrew