Is there any better version in which I can write this code?

Question

I am getting the expected output, but I would like to get optimized and cleaned code. I am using for inside for in accounttriggerhandler. Since it is not a good practice, I always avoid it.

I tried using the map which would avoid a limit exception, but due to duplicate keys, I am facing some issue.

Here is my objective. I will get the domain name which I should check in metadata and if the domain is found, get the regular expression.

Here is the code I tried using MAP:

for(Website_URL_Redirect_Exception__mdt mdtRecrod:[SELECT Domain__c,Matching_Rule__c,Website_URL_Expression__c
                                                        FROM Website_URL_Redirect_Exception__mdt])
{
   domainNameByMetadata.put(mdtRecrod.Domain__c,mdtRecrod); //storing metadata to map
}

for( Account a : newAccounts )
{
    if( oldAccountMap == NULL || ( oldAccountMap != NULL && a.Website !=  oldAccountMap.get( a.Id ).Website ) )
    {
        if( String.isNotBlank( a.Website ) )
        {
            //link.com/fgf
            System.Url websiteURL = new System.Url( ( a.Website.startsWithIgnoreCase( 'http://' ) || a.Website.startsWithIgnoreCase( 'https://' ) ? '' : 'http://' ) + a.Website );//SFDC-2751
            String hostURL = websiteURL.getHost();
            //Strip out www.
            String websiteDomain = hostURL.toLowerCase().remove( 'www.' );//getting here test.com
            if(domainNameByMetadata.containsKey(websiteDomain)){//checking if mdt has domain
                String matchingPattern = domainNameByMetadata.get(websiteDomain).Matching_Rule__c;
                String completeUrl = websiteURL.toExternalForm();
                completeUrl =  completeUrl.toLowerCase().remove( 'www.' );
                Pattern p = Pattern.compile(matchingPattern);
                Matcher pm = p.matcher(completeUrl);
                if (pm.matches()){
                    String webSiteUrlPattern = domainNameByMetadata.get(websiteDomain).Website_URL_Expression__c;
                    Pattern p1 = Pattern.compile(webSiteUrlPattern);
                    Matcher pm1 = p1.matcher(completeUrl);
                    if(pm1.matches()){
                        a.website = pm1.group();
                        a.Website_Domain__c = websiteDomain;
                    }else{
                        a.website=websiteDomain;
                    }
                }else{
                    a.website=websiteDomain;
                }
            }
            else{
                a.Website_Domain__c = websiteDomain;
            }
        }
        else
            a.Website_Domain__c = '';
    }
}

Here is how the metadata looks like:

I was using the map where the key is domain, and the value is metadatatype. Later the change in data (duplication of domain started my problem). So I changed the code using for inside for:

Map<String,Website_URL_Redirect_Exception__mdt> domainNameByMetadata = new Map<String,Website_URL_Redirect_Exception__mdt>();
    List<String> domainNameList = new List<String>();
    for(Website_URL_Redirect_Exception__mdt mdtRecrod:[SELECT Domain__c,Matching_Rule__c,Website_URL_Expression__c
                                                        FROM Website_URL_Redirect_Exception__mdt])
    {
       domainNameByMetadata.put(mdtRecrod.Matching_Rule__c,mdtRecrod);
       domainNameList.add(mdtRecrod.Domain__c);
    }

    for( Account a : newAccounts )
    {
        if( oldAccountMap == NULL || ( oldAccountMap != NULL && a.Website !=  oldAccountMap.get( a.Id ).Website ) )
        {
            if( String.isNotBlank( a.Website ) )
            {
                //link.com/fgf
                System.Url websiteURL = new System.Url( ( a.Website.startsWithIgnoreCase( 'http://' ) || a.Website.startsWithIgnoreCase( 'https://' ) ? '' : 'http://' ) + a.Website );//SFDC-2751
                String hostURL = websiteURL.getHost();
                //Strip out www.
                String websiteDomain = hostURL.toLowerCase().remove( 'www.' );
                if(domainNameList.contains(websiteDomain)){
                    String completeUrl = websiteURL.toExternalForm();
                    completeUrl =  completeUrl.toLowerCase().remove( 'www.' );
                    Boolean isPatternMatched = false;
                    for(String matchingPattern:domainNameByMetadata.keySet()){
                        Pattern p = Pattern.compile(matchingPattern);
                        Matcher pm = p.matcher(completeUrl);
                        if (pm.matches()){
                            String webSiteUrlPattern = domainNameByMetadata.get(matchingPattern).Website_URL_Expression__c;
                            Pattern p1 = Pattern.compile(webSiteUrlPattern);
                            Matcher pm1 = p1.matcher(completeUrl);
                            if(pm1.matches()){
                                a.website = pm1.group();
                                a.Website_Domain__c = websiteDomain;
                                isPatternMatched=true;
                                break;
                            }else{
                                isPatternMatched=false;
                            }
                        } else{
                            isPatternMatched=false;
                        }
                    }
                    if(!isPatternMatched)
                    a.Website_Domain__c = websiteDomain;
                }
                else{
                    a.Website_Domain__c = websiteDomain;
                }
            }
            else
                a.Website_Domain__c = '';
        }

It looks like it is working as expected, but I would like to know if the code looks good or needs any improvement. I tried all the way to avoid for inside for, but neither works. Please guide me.

Answer 1

Spelling

First, fix mdtRecrod, typos can make it a bit more challenging to understand the code's intent, especially when it's this size. Our brains expect letters to be in certain orders, and doing something like that results in additional cognitive load for no good reason.

Safe Navigation Operator (SNO)

You can shorten some of your code with the SNO. You do this by using ?. whenever there may be a null value:

 if(oldAccountMap?.get( a.Id ).Website != a.Website) {

Use Meaningful Variable Names

Names like a are tempting, but they convey no meaning. accountRecord or even record are much more descriptive, and also reduce cognitive load. This is always a good thing.

Don't Do Meaningless Logic

You have a number of situations where you do:

Object someVariable = someValue;
if(someCondition) {
  someVariable = someOtherValue;
} else {
  someVariable = someValue;
}

Those are just cluttering up your code. You've already set a default value, you don't need to reset it every time you reach an else block. This simply increases cognitive load.

Do Not Remove www.

You can cause all kinds of hilarious problems with debugging when someone decides to name their website thebestwww.com or something like that. If you absolutely want to ignore www., do so only when it's at the beginning:

String websiteDomain = hostURL.toLowerCase().removeStart( 'www.' );

Overall, I'd avoid doing this. The www., if present, should be considered part of the host name. There are ways to intentionally confuse your algorithm. It's even technically possible for www.mysafesite.com and mysafesite.com to point to two completely different servers. There's just too much potential for abuse here.

Put Short Logic First

Code tends to be more easily legible when the short path is on top, because you remember the condition better.

        // Don't do this
        if( String.isNotBlank( a.Website ) ) {
          // Many lines of code
        } else {
          // One line of code
        }

Could be better written as:

        if( String.isBlank( a.Website ) ) {
          // One line of code
        } else {
          // Many lines of code
        }

Use Functions

Watch how the main idea is made easy when you use a function or two:

Map<String,Website_URL_Redirect_Exception__mdt> domainNameByMetadata = new Map<String,Website_URL_Redirect_Exception__mdt>();
List<String> domainNameList = new List<String>();
for(Website_URL_Redirect_Exception__mdt mdtRecord:[SELECT Domain__c,Matching_Rule__c,Website_URL_Expression__c 
                                                    FROM Website_URL_Redirect_Exception__mdt]) {
    domainNameByMetadata.put(mdtRecord.Matching_Rule__c,mdtRecord);
    domainNameList.add(mdtRecord.Domain__c);
}
Account[] accountsToProcess = getChangedAccounts(oldAccountMap, newAccounts);
for( Account record: accountsToProcess) {
    if(record.Website == null) {
        record.Website_Domain__c = null;
    } else {
        updateWebsiteDomainField(domainNameList, domainNameByMetadata, record);
    }
}

You don't need to go overboard, but you could:

Map<String, Website_URL_Redirect_Exception__mdt> domainNameByMetadata = getDomainNameByMetadata();
List<String> domainNames = getDomainsFromMetadata(domainNameByMetadata);
Account[] changedAccounts = getChangedAccounts(oldAccountMap, newAccounts);
updateWebsiteDomainField(changedAccounts);

Your function now just tells a story of what it does. The other functions do the heavy lifting for you. It's a few more lines of code, but much more readable. You can do this further down as well to keep the functions small enough and descriptive enough to barely need any comments at all.

Avoid Nested If

If you don't mind writing slightly more complicated unit tests, you can choose to avoid that deeply nested if pattern:

if(someCondition) {
  doLogic();
}

Becomes:

if(!someCondition) {
  continue; // or return, whatever
}
doLogic();

This makes it slightly harder to test, as you have to test each branch to get to 100% coverage, but makes the code a ton easier to read.

---

The refactored code ends up being quite a bit longer than the original code, but the new functions make it really apparent as to what's going on.

// Main logic
Map<String, Website_URL_Redirect_Exception__mdt> domainNameByMetadata = getDomainNameByMetadata();
List<String> domainNames = getDomainsFromMetadata(domainNameByMetadata.values());
Account[] changedAccounts = getChangedAccounts(oldAccountMap, newAccounts);
updateWebsiteDomainFields(changedAccounts);

// Helper functions
Account[] getChangedAccounts(Map<Id, Account>oldAccountMap, Account[] newAccounts) {
    Account[] results = new Account[0];
    for(Account record: newAccounts) {
        if(oldAccountMap?.get(record.Id).Website != record.Website) {
            results.add(record);
        }
    }
    return results;
}
Map<String, Website_URL_Redirect_Exception__mdt> getDomainNameByMetadata() {
    Map<String, Website_URL_Redirect_Exception__mdt> results = new Map<String, Website_URL_Redirect_Exception__mdt>();
    for(Website_URL_Redirect_Exception__mdt mdtRecord:[SELECT Domain__c,Matching_Rule__c,Website_URL_Expression__c 
                                                        FROM Website_URL_Redirect_Exception__mdt]) {
        domainNameByMetadata.put(mdtRecord.Matching_Rule__c,mdtRecord);
        domainNameList.add(mdtRecord.Domain__c);
    }
    return results;
}
String[] getDomainsFromMetadata(Website_URL_Redirect_Exception__mdt[] records) {
    String[] results = new String[0];
    for(Website_URL_Redirect_Exception__mdt record: records) {
        results.add(record.Domain__c);
    }
    return results;
}
Pattern compileAndCachePattern(Map<String, Pattern> cache, String matchingString) {
    Pattern thePattern = cache.get(matchingString);
    if(thePattern == null) {
        cache.put(thePattern, thePattern = Pattern.compile(matchingString));
    }
    return thePattern;
}
void updateWebsiteDomainForAccount(String[] domainNameList, Map<String, Website_URL_Redirect_Exception__mdt> domainNameByMetadata, Account record, Map<String, Pattern> domainCache, Map<String, Pattern> websiteCache) {
    String websiteUrlString = record.Website.toLowerCase();
    if(!websiteUrlString.startsWith('http://') && !websiteUrlString.startsWith('https://')) {
        websiteUrlString = 'http://' + websiteUrlString;
    }
    System.Url websiteAsUrl = new System.Url(websiteUrlString);
    String hostName = websiteAsUrl.getHost();
    record.Website_Domain__c = hostName;
    if(domainNameList.contains(hostName)) {
        String completeUrl = webAsUrl.toExternalForm();
        for(String matchingPattern: domainNameByMetadata.keySet()) {
            Website_URL_Redirect_Exception__mdt thisMetadata = domainNameByMetadata.get(matchingPattern);
            Pattern domainMatchPattern = compileAndCachePattern(domainCache, matchingPattern);
            Pattern websiteMatchPattern = compileAndCachePattern(websiteCache, matchingPattern);
            Matcher domainMatch = domainMatchPattern.matcher(completeUrl);
            Matcher websiteMatch = websiteMatchPattern.matcher(completeUrl);
            if(domainMatch.matches() && websiteMatch.matches()) {
                record.Website = websiteMatch.group();
                return;
            }
        }
    }

}
void updateWebsiteDomainFields(String[] domainNameList, Map<String, Website_URL_Redirect_Exception__mdt> domainNameByMetadata, Account[] records) {
    Map<String, Pattern> domainPatternCache = new Map<String, Pattern>();
    Map<String, Pattern> websitePatternCache = new Map<String, Pattern>();

    for(Account record: records) {
        if(record.Website == null) {
            record.Website_Domain__c = null;
        } else {
            updateWebsiteDomainForAccount(domainNameList, domainNameByMetadata, record, domainPatternCache, websitePatternCache);
        }
    }
}

The main point here is to make the code as easy to read as possible, by using descriptive names and functions, and reduce the number of pathways in every function as much as possible. As you can see, this massive refactor should be a good deal more legible to read than the original by removing deeply nested for/if loops, etc. Notice how every function fits comfortably in a single screen, this helps you scan back and forth over the code to reduce cognitive complexity at the cost of a few extra lines of code.

Finally, notice that there's still a "for loop inside a for loop" (a nested loop). This is acceptable when you're being as selective as possible with the data (see my answer here). The "avoiding nested for loops" Best Practice specifically focuses on situations where it's appropriate to use a Map.

Here, you are using a Map, but that Map happens to contain a list of data, so you're still going to have to loop over every record for every element in the map. This is necessarily unavoidable, and a very correct use of nested for loops, as you can't optimize it any further than it is.