Nested Loops Apex not working as expected

Question

When I pass a list of accounts to my method I want it to compare the incoming list to my accounts in the system and extract duplicates from the list. A duplicate account is same name,company,amount

I have used Maps but I had to compare multiple fields so it wont work.

public static Map<String,List<Account>> AccCheck(List<Account> newAccounts){
    Map<String,List<A>> newMap = new Map<String,List<Account>>();
    List<Account> myList = newAccounts;

    List<Account> dup = new List<Account>();
    List<Account> nondup= new List<Account>();

    if(myList.size()>0){
        List<Account> allAccounts = [SELECT Name,EMAIL,Company from Account];
        for(Account nl: myList){
            for(Account al: allAccounts){
                if(nl.Email.equals(al.Email) && nl.Company.equals(al.Company) && nl.Name.equals(al.Name)){
                    dup.add(nl);
                }
                else{
                    nondup.add(nl); //else loop is being entered too many times 
                }
            }
        }
    }
   newMap.put('Dont Exist:',nondup);
    newMap.put('Exist:',dup);
    System.debug(newMap);
   return newMap; 
} 

}

I gave six accounts with 1 duplicate and 5 non dups. Results is

Exist: 1 dup Dont Exist: >40 results

Answer 1

Here, you are expecting this Result: Exist: 1 dup Don't Exist: 5 results right?

in the existing code, your outer loop consists of all new account which you are passing to your method and in the inner loop your itrating the existing accounts. Based on the inner loop you are creating dup & nondup list. in the nonduplist should contain multiple duplicate records because of the below criteria

if(nl.Email.equals(al.Email) && nl.Company.equals(al.Company) &&nl.Name.equals(al.Name)){
                        dup.add(nl);
              }
               else{
                     nondup.add(nl); //else loop is being entered too many times 
                 }

here the problem is with the nondup list you are comparing all existing record against and if there is no match found you are adding that record to nondup list this will leads to add the same records multiple times to the list.

You can update your code like this

public static Map<String,List<Account>> AccCheck(List<Account> newAccounts){
    Map<String,List<A>> newMap = new Map<String,List<Account>>();
    List<Account> myList = newAccounts;

    List<Account> dup = new List<Account>();
    List<Account> nondup= new List<Account>();

    if(myList.size()>0){
        List<Account> allAccounts = [SELECT Name,EMAIL,Company from Account];
        for(Account nl: myList){
            boolean isdup;
            isdup =false;
            for(Account al: allAccounts){

                if(nl.Email.equals(al.Email) && nl.Company.equals(al.Company) && nl.Name.equals(al.Name)){
                    dup.add(nl);
                    isdup =true;
                }

            }
            if(!isdup){
                    nondup.add(nl); //else loop is being entered too many times 
                }
        }
    }
   newMap.put('Dont Exist:',nondup);
    newMap.put('Exist:',dup);
    System.debug(newMap);
   return newMap; 
}
}

Hope this will help you..

Answer 2

Where you want to use a map to avoid nested loops but want to use multiple values, this is easily done using an array of values, in the correct order, as your key. This works because arrays provide equality and hash code based on their content.

For example, you might have a map like:

Map<Object[], Id> objectIdsByNameAndCompany;

You might check and add like:

Object[] key = new Object[] {
    account.Name,
    account.CompanyId
};

If (objectIdsByNameAndCompany.containsKey(key)) {
    // Duplicate
} else {
    objectIdsByNameAndCompany.put(key, account.Id);
}

While I avoid it pretty much wherever possible, due to mutability problems (some code making an update to the key object after the key is used to insert a value into a map) it is possible to use an SObject as a key in a map. This will go wrong as soon as a change is made to any field within that SObject as this changes the hash code and equality for the object.

Answer 3

You can use Map in such kind of scenarios as well.

You can create a Map with sObject as key with only the attributes which needs to be checked for duplication and value as original sObject. And when doing a check, create a new instance of sObject with the same attributes and check if this instance is present in Map, if it returns true, that means its a duplicate else its not.

Map<Account, Account> mapAccounts = new Map<Account, Account>();
if(myList.size()>0){
    List<Account> allAccounts = [SELECT Name,EMAIL,Company from Account];
    for(Account acc : allAcounts){
        //creating a map with key as sObject with fields Name, Company, Email and value as original Account sObject
        Account keyAcc = new Account(Name = acc.Name, Company = acc.Company, Email = acc.Email);
        mapAccounts.put(keyAcc,acc);
    }
    for(Account nl: myList){
        Account valueToCheck = new Account(Name = nl.Name, Company = nl.Company, Email = nl.Email);
        if(mapAccounts.containsKey(valueToCheck)){
            dup.add(nl);
        } else {
            nondup.add(nl); 
        }
    }
}

And a more traditional approach where you can create a unique key by concatenating the values of the fields which needs to be used for duplication check i.e. in your case, it can be a combination of Name Company and Email, so your key of the Map can be Name-Company-Email and then you can check if the values with same combination exist in the map, if they exist, then they are duplicates else they are not duplicates.

So your code using this approach should be something like below

Map<string, Account> mapAccounts = new Map<string, Account>();
if(myList.size()>0){
    List<Account> allAccounts = [SELECT Name,EMAIL,Company from Account];
    string originalKey='';
    string verificationKey='';
    for(Account acc : allAcounts){
        //creating a map with key as 'Name-Company-Email' and value as Account sObject
        tempKey = string.valueOf(acc.Name!=null ? acc.Name : '' ) + '-' + 
                        string.valueOf(acc.Company!=null ? acc.Company : '' ) + '-' + 
                        string.valueOf(acc.EMAIL!=null ? acc.EMAIL : '' );
        mapAccounts.put(tempKey, acc);
    }
    for(Account nl: myList){
        verificationKey = string.valueOf(nl.Name!=null ? nl.Name : '' ) + '-' + 
                        string.valueOf(nl.Company!=null ? nl.Company : '' ) + '-' + 
                        string.valueOf(nl.EMAIL!=null ? nl.EMAIL : '' );
        if(mapAccounts.containsKey(verificationKey)){
            dup.add(nl);
        } else {
            nondup.add(nl); 
        }
    }
}

The problem with your code is that you are looping the same records multiple times for various combination and due to this every time the condition doesn't match, it will go into the else condition and add it as a non duplicate value. Although that can be avoided by using other techniques, but still I won't prefer that approach as it impacts the performance.