For Loop within For Loop.How to avoid such situation?

Question

For Loop within For Loop.How to avoid such situation:

I have a requirement where I am updating Parent Object(NCR__c) Status Value based on the Status of Child object(NCR_Task__c) Value.

Scenario 1: If a Parent has multiple child records and if any of the Child Record Status is Open, Parent Value should Set to Open.

Scenario 2: If a Parent has multiple child records and if any of the Child Record Status is Closed & None of the Child Record Status is Open,Parent Value should set to Closed.

Scenario 3: If a Parent has multiple child records and if all Child Records Status is Cancelled,Parent Value should set to Cancelled.

To Achive the functionality,I have written below code After Inser,After Update,After Delete of Child Records which is working fine:

public void ncrStatusAutoUpdate(List<SObject> newItems,Map<Id,SObject> oldItems){
    try{
        NCR__c ncrObj;
        List<NCR__c> nrcListToBeUpdated = new List<NCR__c>();
        Set<String> ncrListId = new Set<String>();
        Set<String> ncrTaskListId = new Set<String>();
        List<String> allNcrStatus= new List<String>();

        for(SObject newObj :newItems){
            NCR_Task__c newNCRTaskObj = (NCR_Task__c)newObj;
            NCR_Task__c oldNCRObj = null;
            if(newNCRTaskObj.Id!=null && oldItems!=null) {
                oldNCRObj = (NCR_Task__c)oldItems.get(newNCRTaskObj.Id);
            }
            if(newNCRTaskObj.NCR__c!=null){
                ncrListId.add(newNCRTaskObj.NCR__c);
            }
        }

        List<NCR__c> nrcList = [SELECT Id,(SELECT Id, status__c FROM NCR_Tasks__r) FROM NCR__c WHERE ID IN :ncrListId];

        for (NCR__c ncr : NrcList){
            for(NCR_Task__c tasks: ncr.NCR_Tasks__r) {

                allNcrStatus.add(tasks.Status__c);  

            }   

            if((allNcrStatus.contains('Open')) || (allNcrStatus.contains('In Progress'))){
                ncr.NCR_Status__c='Open';
            }
            else if(allNcrStatus.contains('Closed')){
                ncr.NCR_Status__c='Closed';  
            }
            else {
                ncr.NCR_Status__c='Cancelled';       
            }

            nrcListToBeUpdated.add(ncr);

        }

        if(nrcListToBeUpdated.size()>0){
            update nrcListToBeUpdated;
        }

    }

    catch(exception ex){
        system.debug('Exception in ncrStatusAutoUpdate and error is--->'+ex);
    }

}

If You can see in the above code, there is a situation where I have to Use For Loop within For Loop and it is necessary otherwise my functionlity does not work as expected.

I want to know if it is valid to Use For Loop with For Loop?

As per my knowledge if I use For Loop within For Loop,it unnecessary increases CPU time Limit which might result in CPU Time Limit exception.

I have tried to written a logic under one for loop but the functionlity breaks.

Please Do Suggest if I can modify the same code by avoiding for loop within for loop...

Answer 1

There's no problem with this for loop configuration because it doesn't cause multiplicative execution. The reason you may have heard about avoiding nested for loops is that some people try a naive implementation to match up records. Here's a typical example of what not to do:

Account[] accounts = [SELECT Name FROM Account];
Contact[] contacts = [SELECT AccountId, Name FROM Contact];
for(Contact contactRecord: contacts) {
  for(Account accountRecord: accounts) {
    if(contactRecord.AccountId == accountRecord.Id) {
      // Do something here
    }
  }
}

This causes a multiplicative effect; if there are 100 accounts with just 10 contacts each, you end up with a total of 100,000 loop executions, while if you used a Map for the accounts, you'd end up with just the 1,000 loop executions for the 1,000 contacts in the list, resulting in 99% less CPU usage. Here's the way you'd do this properly:

Map<Id, Account> accounts = new Map<Id, Account>([SELECT Name FROM Account]);
Contact[] contacts = [SELECT AccountId, Name FROM Contact];
for(Contact contactRecord: contacts) {
  Account accountRecord = accounts.get(contactRecord.AccountId);
  // Do something here
}

However, in your case, you're simply iterating over the child of each parent. This is a linear effect, not a multiplicative effect. This pattern, generally speaking, is about as efficient as it can be.

You could use an aggregate query to get slightly better performance, but unless there's a significant number of tasks, it's probably not worth it. Here's the design you might use for that:

AggregateResult[] results = [SELECT Status__c Status, NCR__c Id FROM NCR_Task__c WHERE NCR__c IN :ncrListId GROUP BY Status__c, NCR__c];
Map<Id, NCR__c> ncrRecords = new Map<Id, NCR__c>();
for(AggregateResult result: results) {
  Id ncrId = (Id)result.get('Id');
  String ncrStatus = (String)result.get('Status');
  NCR__c ncrRecord = ncrRecords.get(ncrId);
  // Nothing recorded yet, set to default value
  if(ncrRecord == null) {
    ncrRecords.put(ncrId, ncrRecord = new NCR__c(Id=ncrId, NCR_Status__c='Cancelled'));
  }
  if(ncrStatus == 'Open' || ncrStatus == 'In Progress') {
    ncrRecord.NCR_Status__c = 'Open';
  } else if(ncrStatus == 'Closed' && ncrRecord.NCR_Status__c != 'Open') {
    ncrRecord.NCR_Status__c = 'Closed';
  }
}
update ncrRecords.values();

This might be slightly more efficient if there's a large number of tasks for the record.

---

Two notes:

First, do not use a try-catch here. If you're worried about DML errors, you should handle those explicitly and report errors on the NCR_Task__c record, if necessary. This try-catch block will cause unexplainable behavior when something goes wrong.

Second, you don't need to check if a list is empty before performing a DML operation. The system does this for you automatically.

Do Not Use

    if(nrcListToBeUpdated.size()>0){
        update nrcListToBeUpdated;
    }

Preferred Method

    update nrcListToBeUpdated;

The former is slower than the latter, and uses more code.

Answer 2

I would suggest to use Boolean variable and store all the child records in a list and run a for loop on that .set condition if status == open while running the for loop .if it found any record with status = open set the boolean equal true and exit the loop .

silimilary for updating the parent if the boolean is true set the parent as open else closed