In addition to performing one query per record in the trigger context, it also performs one
DML Operation per child
Contact record. Really awful from a performance perspective. The main idea behind "bulkification" of any trigger is to move to a collection based approach. In this case, you need to query the
Contact before your loop, and update those that need it afterward.
If you are not a developer, you can indeed move this logic to
Process Builder (as mentioned in the comments) and avoid the need to write unit tests, which are almost certainly lacking in this case. If you want to fix this code, you need to learn about many topics in
Apex development, but most critically: trigger bulkification, proper unit testing, and exception handling. It would also be really beneficial to learn about trigger handler patterns and how to set up one trigger per object.
The main risk is that you will suffer a performance hit, and may experience CPU timeouts by switching to
Process Builder. Another big problem with the approach is that it does not handle errors gracefully. So if you try to update a child
Contact and it fails, it can fail the entire transaction and provide really ugly and unhelpful error messages.
The basic idea on how to perform your updates without hitting governors:
List<Contact> contactsToUpdate = new List<Contact>();
for (Contact contact : [
SELECT ... FROM Contact
WHERE AccountId IN :trigger.newMap.keySet()
if (contact.MailingCity != contact.Account.BillingCity && ...)
Id = contact.Id,
MailingCity = contact.Account.BillingCity,
In the above code, I omitted many fields for brevity, but you should get the gist. In order to properly perform the updates, you must handle any
DmlException that may occur as a result of your cross-object update.
catch (DmlException d)
for (Integer i = 0; i < d.getNumDml(); i++)