0

I'm new to the Salesforce development, I want to write a trigger for my custom object(Test__C).

In my custom object I have 2 fields, one is email__C and other id contact_id__C.

So I want to check if contact_id__C is null and contact is available with email_C field than I have to map contact id with my object Test__C(contact_id__C) and if any contact is not available than I have to create a contact first with email__C than have to map with contact_id__C field of Test__C.

I have written the below trigger and it's working as expected,but want to know this is best practice or not :

trigger MapContact on Test__C (before insert) 
{
    List<String> EmailAddresses = new List<String>();
    //First exclude any records where the contact is set
    for (Test__C c:Trigger.new) 
    {
        if (c.Contact_Id__C==null && c.Email__C!='')
        {
            EmailAddresses.add(c.Email__C);
        }
    }
    //query for how many contacts already exist.
    List<Contact> listofallContacts = [Select Id,Email From Contact Where Email in:EmailAddresses];
    Map<String, Contact> ExstingEmails = new Map<String, Contact>();
    for (Contact c:listofallContacts) 
    {
        ExstingEmails.put(c.Email,c);
    }   

    Map<String,Contact> emailToContactMap = new Map<String,Contact>();
    List<Test__C> recordsToUpdate = new List<Test__C>();
    List<Contact> CreateContact = new List<Contact>();
    for (Test__C c:Trigger.new) {
        if (c.Contact_Id__C==null && c.Email__C!='' && ExstingEmails.containsKey(c.Email__C))
        {

emailToContactMap.put(c.Email__C,ExstingEmails.get(c.Email__C)); recordsToUpdate.add(c); } else if(c.Contact_Id__C==null && c.Email__C!='' && !ExstingEmails.containsKey(c.Email__C)) { String[] CName = c.Email__C.split('@',2); if (CName.size() == 2) { Contact conts = new Contact(FirstName=CName[0], LastName=CName[1], Email=c.Email__C ); emailToContactMap.put(c.Email__C,conts); recordsToUpdate.add(c); CreateContact.add(conts); } } } insert CreateContact;

    for (Test__C c:recordsToUpdate) {
        Contact newContact = emailToContactMap.get(c.Email__C);
        c.Contact_Id__C= newContact.Id;
    }
}
1
  • Have you completed the Apex modules on Salesforce Trailhead? There are also many many good tutorials on the web to guide you on best practices. Sep 18, 2018 at 19:46

1 Answer 1

1

This strikes me a something that would be more appropriate for codereview.stackexchange.com (which I do keep an eye on, as I hope other people here do as well)

That said, it looks like you've avoided the 3 major pitfalls that new devs tend to run into (only working with the first record in trigger.new, querying inside of loops, dml inside of loops)

Current "best practice" for triggers is to keep the trigger "logicless", instead preferring to keep the logic used in the trigger in a separate Apex class, and to only have one trigger per object. If you're feeling up to it, you can try using a trigger framework like the one developed by Kevin O'Hara to accomplish this.

Other than that, there are several improvements that could be made to your code

  • I'm not entirely sure that emailToContactMap is required here. You might be able to simply use existingEmails
  • I'd suggest writing a unit test to verify what happens when you have multiple Test__c records with the same email, and that email is not used by an existing contact. Looks to me like you'll create duplicate contacts with your current code. That may be fine by you, but it's still something that should be tested.
  • String.isNotBlank() is preferred above myString != '' because String.isNotBlank() additionally checks for null strings
  • Your main if/else if contains some repeated terms that could be factored out to reduce typing/duplication (I like the DRY principle, Don't Repeat Yourself).

    // Instead of this
    if (c.Contact_Id__C==null && c.Email__C!='' && ExstingEmails.containsKey(c.Email__C)){
        // some code
    }else if (c.Contact_Id__C==null && c.Email__C!='' && !ExstingEmails.containsKey(c.Email__C)){
        // other code
    }
    
    // Use this (more DRY)
    // Common terms become an outer "if" statement
    if (c.Contact_Id__C==null && c.Email__C != ''){
        // Unique terms become inner if/else if/else statements
        if(ExstingEmails.containsKey(c.Email__c)){
            // some code
        }else{
            // other code
        }
    }
    
1
  • Thanks for your review,I will change my code according to points mentioned by you and check it. Sep 19, 2018 at 8:22

You must log in to answer this question.

Not the answer you're looking for? Browse other questions tagged .