0

I have read so many posts about maps today but I just can't seem to get them to apply to my use case. I have an object called transactions and each transaction is related to an account. I am trying to total the transactions for each account so I am looping through the accounts, adding up the transactions and moving on to the next account. As expected I get the Apex CPU Time Limit Exceeded error. Could someone please explain how I can rewrite my code to avoid the issue.

public class TransactionCleaning {
public static void TransactionCleaning(){
    List<Transactions__c> TransactionsToArchive = new list<Transactions__c>();
    List<Transactions__c> TransactionBal = new list<Transactions__c>();

    List<account> accountsWithTransactions = [select id,name from account 
                                              WHERE id IN(SELECT account__c FROM Transactions__c WHERE 
                                                          Transaction_Date__c < LAST_N_Months:1)];

    List<Transactions__c> trans = [SELECT Id, Aluminium_Items__c, Glass_Items__c, 
                                     PET_Items__c, Steel_Items__c, RVM__c, Account__c FROM Transactions__c 
                                     WHERE Transaction_Date__c < LAST_N_Months:1];

    For(Account acct: accountsWithTransactions){
            // Initialise variables
            decimal AluminiumCount = 0;
            decimal GlassCount = 0;
            decimal PETCount = 0;
            decimal SteelCount = 0;
            Id rvmID;

            For(Transactions__c t : trans){
                if (t.Account__c == acct.Id){
                    if (t.Aluminium_Items__c != null){AluminiumCount += t.Aluminium_Items__c;}
                    if (t.Glass_Items__c != null){GlassCount += t.Glass_Items__c;}
                    if (t.PET_Items__c != null){PETCount += t.PET_Items__c;}
                    if (t.Steel_Items__c != null){SteelCount += t.Steel_Items__c;}
                }
              TransactionsToArchive.add(t);
              rvmID = t.RVM__c;                           
            }
        system.debug(rvmID);
        Transactions__c t1 = new Transactions__c(Name='Bal C/F - ' + System.Now(), RVM__c=rvmID, account__c=acct.Id, Description__c = 'Balance Carried Forward',
                                                 Aluminium_Items__c=AluminiumCount, Transaction_Date__c = System.Today());
        TransactionBal.Add(t1);
     }
     Insert TransactionBal;
}
  • How many records. Did you do any profiling? Is this running in a batch – Eric Jul 28 '17 at 6:50
  • Yes it is running in batch. There are about 12K accounts and 64K transactions. – DanP Jul 31 '17 at 3:21
3

In addition to Santanu Boral's answer, the standard answer to these questions is generally

If your two objects are in a Master-Detail relationship, use a rollup summary field instead of trying to roll your own solution. If you only have a lookup relationship between the two objects, look at Andrew Fawcett's Declarative Lookup Rollup Summary tool.

While that is good advice, and using SOQL Aggregate Functions like COUNT() would be my next preferred approach, I'm more interested in your statement about not being able to get a Map to work for you.

Seeing a nested loop resembling this is a pretty big red flag

for(Obj_A__c a :listOfAs){
    for(Obj_B__c b :listOfBs){
        if(a.something == b.something){
            // do stuff
        }
    }
}

If listOfAs contains 1000 records, and listOfBs also contains 1000 records, your innermost loop will run 1,000,000 times. For traditional for loops (i.e. for(Integer i = 0; i < someNumber ; i++)) a loop structure like that would only take around 250 units of your CPU limit (just to run, not doing anything useful). For loops over collections, just running the snippet above takes over 5000 CPU units (the CPU limit now appears to be 39,000 units for synchronous code, but for a number of years, that limit was 10,000) Yikes!

Using an inner loop to match records wastes a lot of time. We can do better using a Map. More specifically, you would want to use a Map<Id, List<Transactions__c>> where the key of the map is an Account Id, and the values are the list of Transactions__c whose Account__c field matches the key.

Building that map does require a loop, but it's a loop that will run through significantly fewer iterations (and thus be a lot less demanding of CPU time).

The code I would write for that would be this

Map<Id, List<Transactions__c>> accountToTransactionsMap = new Map<Id, List<Transactions__c>>();

for(Transactions__c tran :[SELECT Id, Aluminium_Items__c, Glass_Items__c, 
                                     PET_Items__c, Steel_Items__c, RVM__c, Account__c 
                           FROM Transactions__c 
                           WHERE Transaction_Date__c < LAST_N_Months:1]){
    // This is my preferred method for building maps. 
    // There are other ways to build a map, but I like this one because it separates
    //   initialization from population
    if(!accountToTransactionsMap.containsKey(tran.Account__c)){
        // In here, we know that this is the first time we have encountered this
        //   particular Account Id.
        // Add the new key to the map, and set up an empty List to store data in.
        accountToTransactionsMap.put(tran.Account__c, new List<Transactions__c>());
    }

    // Out here, we are guaranteed to have tran.Account__c in our map (and a list to
    //   add items to).
    // Thus, the following line is completely safe (without needing to do any additional checks)
    accountsToTransactionsMap.get(tran.Account__c).add(tran);
}

I hope that you can see how this will result in Transactions__c records being pooled into a collection of lists based on the Account Id.

From there, your loops to count items becomes

For(Account acct: accountsWithTransactions){
    // Initialise variables

    For(Transactions__c t : accountToTransactionsMap.get(acct.Id)){
        // Since we already have the Transactions__c separated by Account,
        //   there's no need for this if statement.
        // Separating Transactions__c by Account also means that this
        //   inner loop will run significantly fewer times
        //if (t.Account__c == acct.Id){
            if (t.Aluminium_Items__c != null){AluminiumCount += t.Aluminium_Items__c;}
            if (t.Glass_Items__c != null){GlassCount += t.Glass_Items__c;}
            if (t.PET_Items__c != null){PETCount += t.PET_Items__c;}
            if (t.Steel_Items__c != null){SteelCount += t.Steel_Items__c;}
        //}
        // other work omitted                        
    }
    // other work omitted
 }

That should tame your CPU limit issues.

Now that I'm done with that, there is an even easier way to get the Transactions__c records related to each Account. You can use a parent-child subquery (a.k.a. a left outer join)

// The subquery is everything enclosed between parentheses '(', ')'.
// Fields in a subquery are specified the same as if it was a stand-alone query
// In the subquery, instead of using the child SObject name, we use the
//   'relationship name', which is usually the plural of the object name + '__r'
//   for custom objects.
// Since your base object name 'Transactions__c' is already plural, I don't know
//   if 'Transactions__r' is quite right.
// You can check the relationship name by diving into the detail page for the 
//   Account__c lookup field.
List<Account> acctList = [SELECT Id, (SELECT Id, Aluminium_Items__c FROM Transactions__r) FROM Account];

for(Account acct :acctList){
    // To access the child records from the subquery, simply use the relationship name.
    // A Parent-Child subquery always results in a List.
    // The results of the subquery will only contain the Transactions__c
    //   records related to the current Account (in the outer loop)
    for(Transactions__c tran :acct.Transactions__r){
        // other work done in here
    }
}
| improve this answer | |
  • Thanks for expanding my answer, when I started answering this question, it was 2 AM at my end, so didn't get time to put more info – Santanu Boral Jul 28 '17 at 16:34
  • @DanP I'm not sure which loop you're talking about. About the only things I can see being null are your AluminiumCount and related variables (which I had omitted from my example for brevity). – Derek F Jul 31 '17 at 12:43
  • @DerekF, I've done some troubleshooting with system debug and I've discovered that the map key values don't seem to have the correct Id values for the accounts. They have three additional characters 18 instead of 15? – DanP Aug 1 '17 at 12:47
  • @DanP Salesforce uses both 15 and 18 character ID strings. The 15-char version is case-sensitive, while the 18-char version is case-insensitive. If you do an equality comparison between the 15-char Id and the corresponding 18-char Id, you'll see that they are indeed treated as equal. – Derek F Aug 1 '17 at 12:50
  • Thanks @DerekF, yes I see that. I have also placed a system.debug(accountsToTransactionsMap.keySet()) line just after the map is complete and I notice that the first value in the key set is null. – DanP Aug 1 '17 at 13:12
7

There are potential reasons to optimize your coding.

Rather than 2 SOQL queries, you could have single Aggregate query to solve your purpose.

List<AggregateResult> lstResult = [SELECT account__c,account__r.Name, 
                                COUNT(Aluminium_Items__c),  
                                COUNT(Glass_Items__c),
                                COUNT(PET_Items__c),
                                COUNT(Steel_Items__c)
                                FROM Transactions__c
                                WHERE Transaction_Date__c < LAST_N_Months:1
                                GROUP BY account__c,account__r.Name];

for(AggregateResult result:lstResult)
{
     //do your logic    
}

You can also play around with map for rest of the logic.

| improve this answer | |
5

In general, when you have the following pattern in your code:

List<Object> list1 = ...;
List<Object> list2 = ...;
for(Object list1val: list1) {
  for(Object list2val: list2) {
    if(list1val.something == list2val.something) {
     ...

What you have is the perfect candidate for one or more Map types. There's several ways you could have written this. One way your code could have been written would be like this:

public class TransactionCleaning {
    class Totals {
        Decimal glass, steel, aluminum, pet;
        Id rvmId, accountId;
        Totals(Id rmvId, Id accountId) {
            this.rvmId = rvmId;
            this.accountId = accountId;
            glass = steel = aluminum = pet = 0;
        }
    }
    public static void TransactionCleaning(){
        Map<Id, Totals> totalValues = new Map<Id, Totals>();
        Transactions__c[] recordsToCreate = new Transactions__c[0],
            recordsToArchive = [
                SELECT Aluminium_Items__c, Glass_Items__c, PET_Items__c, Steel_Items__c, RVM__c, Account__c 
                FROM Transactions__c 
                WHERE Transaction_Date__c < LAST_N_Months:1];
        for(Transactions__c record: recordsToArchive) {
             Totals t = totalValues.get(record.Account__c);
             if(t == null) {
                 totalValues.put(record.Account__c, t = new Totals(record.RVM__c, record.Account__c));
             }
             if(record.Aluminum_Items__c != null) {
                 t.aluminum += record.Aluminum_Items__c;
             }
             if(record.Glass_Items__c != null) {
                 t.glass += record.Glass_Items__c;
             }
             if(record.Steel_Items__c != null) {
                 t.steel += record.Steel_Items__c;
             }
             if(record.Pet_Items__c != null) {
                 t.pet += record.Pet_Items__c;
             }
         }
         for(Totals value: totalValues.values()) {
             recordsToCreate.add(
                 new Transactions__c(
                     Name = 'Bal C/F - ' + System.Now(),
                     RVM__c = value.rvmId,
                     Account__c = value.accountId,
                     Aluminum_Items__c = value.aluminum,
                     Steel_Items__c = value.steel,
                     Pet_Items__c = value.pet,
                     Glass_Items__c = value.glass,
                     Transaction_Date__c = System.today()));
         }
         insert recordsToInsert;
    }
}

Notes

There's no need to query the parents explicitly if you're not going to use them. This reduces the number of records we need to query. If you had actually needed the accounts, you could have used a relationship query to get, for example, the account name.

There's several ways you could have used a map, but I simply chose one interpretation you could have used. Using a map allows you to look up values efficiently. The intent is to avoid looping over two lists, with one iteration for one list looping over every element in the other. This is potentially very expensive. To see the math applied here, if we assume there's 1,000 accounts with 10 transactions each (a total of 10,000 transctions), the code above has 10,000 iterations, while your code would have a total of 100,000 iterations, or 10 times slower.

The AggregateResult solution presented by Santanu is even better, as it would loop only 1,000 items (once per account). This solution is presented as an alternative only because the AggregateResult solution can only process 2,000 accounts (the AggregateResult limit), while this code can handle up to 10,000 accounts (the DML governor limit) or 50,000 transactions (the SOQL query limit), whichever comes first. Since we don't know how much data we're dealing with, you might prefer one solution over the other.

Either way, you'll want to experiment with either AggregateResult or some sort of Map tabulation. Note that if you have even more data you need to process, you'd eventually need to migrate to a batch process, in which case you could adapt the class to a Batchable class.

| improve this answer | |
  • Thanks for expanding my answer as well as providing optimization, when I started answering this question, it was 2 AM at my end, so didn't get time to put more info. Thanks again. – Santanu Boral Jul 28 '17 at 16:35
  • 1
    @SantanuBoral I like your answer, too, I just thought it'd be worth a bit more exposition. – sfdcfox Jul 28 '17 at 16:44
  • Thanks, your answer is superb. I would like to select your answer – Santanu Boral Jul 28 '17 at 16:45

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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