Take the 2-minute tour ×
Salesforce Stack Exchange is a question and answer site for Salesforce administrators, implementation experts, developers and anybody in-between. It's 100% free, no registration required.

I'm trying to run the following piece of code using anonymous apex. I'm looking to update owners of historical records. Basically, if account number in a historical matches account id of an account, update owner of the historical to match account owner. I'm new in Apex and would appreciate some guidance on best practices.

List<Account> accounts = [SELECT Account_ID__c, OwnerId FROM Account WHERE Account_ID__c != Null];
List<Historical__c> toBeUpdated = [SELECT Account_Number__c, OwnerId FROM Historical__c WHERE Account_Number__c != Null];
List<Historical__c> historicalUpdates = new List<Historical__c>();
for(Account a : accounts ){
    for(Historical__c h : toBeUpdated ){
        if(a.Account_ID__c == h.Account_Number__c){
            h.OwnerId = a.OwnerId;

            historicalUpdates.add(h);
        }
    }
}
update historicalUpdates;
share|improve this question
    
Is there some limit to how many Accounts and Historicals you might have? Aside from the logic inefficiency, these unlimited queries could easily cause governor limit trouble if there are ever too many records in the database. –  Jeremy Nottingham Jul 3 at 19:44

2 Answers 2

You appear to have a loop inside a loop so Big O of n^2. This should typically be avoided if possible. You main goal should be to reduce this.

How exactly are your objects related? Is that field a lookup to Account or just a shared field essentially that links them?

First step, you want to get all of the Account Ids in one place.

List<Account> accounts = [SELECT Account_ID__c, OwnerId FROM Account WHERE Account_ID__c != Null];
Set<String> accIds = new Set<String>();
for (Account acc: accounts)
{
  accIds.add(acc.Account_ID__c);
}

You then want to use populate a Map of your Historical that are associated by AccountIds. (I'll assume that they can share the same AccountIds.)

Map<String, List<Historical__c>> histMap = new Map<String, List<Historical__c>>();
List<Historical__c> toBeUpdated = [SELECT Account_Number__c, OwnerId from Historical__c WHERE Account_Number__c IN :accIds];
for (Historical__c hist: toBeUpdated)
{
  List<Historical__c> hists = histMap.get(hist.Account_Number__c);
  if (hists == null)
  {
    hists = new Map<Historical__c>();
    histMap.put(hist.Account_Number__c, hists);
  }
  hists.add(hist);
}

Now, you have everything you need to eliminate that Big O of n^2. Your final loop to bring it all together:

for (Account acc: accounts)
{
  List<Historical__c> hists = histMap.get(acc.Account_Id__c);
  if (hists != null)
  {
    for (Historical__c hist: hists)
    {
      hist.OwnerId = acc.OwnerId;
    }
  }
}

Remember that everything is pass by reference and your query only grabbed Historical__c that had at least 1 Account associated to it already. So you don't need to do anything special.

update toBeUpdated;

Now your code has been reduced to essentially Big O of 2n which is essentially just Big O of n.

share|improve this answer

The CPU limit will be the result of the nested for loop when you take into account how many Accounts and Historical__c records there are.

Ideally this sort of processing would be done in a batch. That way you could limit the number of Accounts in each transaction.

One option with anonymous apex is to reduce the number of records being processed at any one time. Use an AccountId and ordering clause to manually batch through the records.

// Update this with the last log message
string lastProcessedAccountId = '001000000000000';

// Limit the total number of Accounts being processed at any one time.
List<Account> accounts = [SELECT Account_ID__c, OwnerId 
                          FROM Account 
                          WHERE Account_ID__c != Null 
                            and ID > :lastProcessedAccountId 
                          ORDER BY Id asc 
                          LIMIT 100];

// TODO: Set the correct type for your custom Account_Number__c field
Set<integer> accountIds = new Set<integer>();
for(Account a : accounts ){
    if(!accountIds.contains(a.Account_ID__c)) {
        accountIds.add(a.Account_ID__c);
    }
    lastProcessedAccountId = s.Id;
}    

// Get only those Historical records that are applicable to the Accounts being processed
List<Historical__c> toBeUpdated = [SELECT Account_Number__c, OwnerId 
                                   FROM Historical__c
                                   WHERE Account_Number__c in :accountIds];

Map<integer, List<Historical__c>> accountIdToHistoricalMap = 
    new Map<integer, List<Historical__c>>();

for(Historical__c h : toBeUpdated ) {
   List<Historical__c> hl = null;
   if(accountIdToHistoricalMap.containsKey(h.Account_Number__c)) {
       hl = accountIdToHistoricalMap.get(h.Account_Number__c);
   } else {
       hl = new List<>(Historical__c);
       accountIdToHistoricalMap.put(h.Account_Number__c, hl);
   }
   hl.add(h);
}

List<Historical__c> historicalUpdates = new List<Historical__c>();
for(Account a : accounts ) {
    if(accountIdToHistoricalMap.containsKey(a.Account_ID__c)) {
        List<Historical__c> hl = accountIdToHistoricalMap.get(h.Account_Number__c);
        for(Historical__c h : hl ){
            h.OwnerId = a.OwnerId;
            historicalUpdates.add(h);
        }
    }      
}
update historicalUpdates;

// Use this logged ID to set the lastProcessedAccountId for the next run
System.debug(LoggingLevel.Error, 'Last Account Id: ' + lastProcessedAccountId);
share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.