Using Integers in SOQL For Loops - Apex CPU time limit exceeded

Question

I need to insert Monthly Revenue Forecast records for each Opportunity that's been inserted / updated, matches my criteria and caused the trigger to fire.

When I execute the below, in bulk via a test class, it causes an error

System.LimitException: Apex CPU time limit exceeded

I've already changed the lines after if(*list* size() > 0){ from for(Opportunity o : insOpps){ to use the SOQL query's instead.

And my trigger's not updating any fields on the Opportunity - and therefore causing the trigger to fire again.

So I'm guessing that either I've got my syntax wrong (but I am closing each For loop and have the DLM statements outside the loops) or my use of integers with the SOQL for loop is causing the issue, could you please suggest potential causes?

trigger MRFv2 on Opportunity (after insert, after update) {

    List<WEID__c> weids = WEID__c.getall().values();
    Set<Id> validRecordTypeIds = new Set<Id>();

    // add Opportunity Record Type Ids from Custom Setting to list of valid Ids      
    for(WEID__c weid: weids) {
        try {
            validRecordTypeIds.add(weid.WEOppId__c);
        } catch (System.StringException e) {
            System.debug(
                System.LoggingLevel.ERROR,
                'Invalid Record Type Id ' + weid.WEOppId__c);
        }
    }

    List<Opportunity> insOpps = new List<Opportunity>();
    List<Opportunity> upOpps = new List<Opportunity>();
    Map<Id,Integer> forecastDuration = new Map<Id,Integer>();
    Map<Integer,String> monthName = new Map<Integer,String>{1 => 'January', 2 => 'February', 3 => 'March', 4 => 'April', 5 => 'May', 6 => 'June', 7 => 'July', 8 => 'August', 9 => 'September', 10 => 'October', 11 => 'November', 12 => 'December'};
    List<Monthly_Revenue_Forecast__c> existMRFs = new List<Monthly_Revenue_Forecast__c>();
    List<Monthly_Revenue_Forecast__c> newMRFs = new List<Monthly_Revenue_Forecast__c>();
    Map<Id,Decimal> rampSett = new Map<Id,Decimal>();
    Map<Id,Decimal> rampTxnRev = new Map<Id,Decimal>();

    for(Opportunity o : Trigger.New){

        if(validRecordTypeIds.contains(o.RecordTypeId))
        {
            try{
                Opportunity oldO = Trigger.OldMap.get(o.Id);

                if(oldO.CloseDate != o.CloseDate
                   ||
                   oldO.Annual_Settlement_Value__c != o.Annual_Settlement_Value__c
                   ||
                   oldO.Implementation_Revenue__c != o.Implementation_Revenue__c
                   ||
                   oldO.Implementation_Revenue_as_a_of_FSR__c != oldO.Implementation_Revenue_as_a_of_FSR__c
                   ||
                   oldO.FSR_End_Date__c != o.FSR_End_Date__c
                   ||
                   oldO.Monthly_Txn_Revenue_POS__c != o.Monthly_Txn_Revenue_POS__c
                   ||
                   oldO.Annual_transaction_volume__c != o.Annual_transaction_volume__c)
                {
                    upOpps.add(o);
                    forecastDuration.put(o.Id, o.Implementation_Revenue__c.monthsBetween(o.FSR_End_Date__c));
                    system.debug('upOpps contains ' + upOpps);
                    system.debug('forecastDuration contains ' + forecastDuration);
                }
                //if old.Map is null, record has been inserted & MRF records should be added
            }catch(system.NullPointerException e){
                insOpps.add(o);
                forecastDuration.put(o.Id, o.Implementation_Revenue__c.monthsBetween(o.FSR_End_Date__c));
            }
        }
    }

    if(insOpps.size() > 0){

        for(Opportunity o : [SELECT Id,Name,Implementation_Revenue__c,Implementation_Revenue_as_a_of_FSR__c,
                                    Annual_Settlement_Value__c,Annual_transaction_volume__c,Monthly_Txn_Revenue_POS__c,
                                    CurrencyIsoCode 
                               FROM Opportunity
                              WHERE Id IN :insOpps])
        {

            Date ic = o.Implementation_Revenue__c;
            Integer icm = o.Implementation_Revenue__c.month();//DELETE?

            //Ramp calculations
            Decimal increment = o.Implementation_Revenue_as_a_of_FSR__c / 100;
            Integer rDur = (1 / increment).setscale(0,roundingMode.DOWN).intValue();
            Integer fcstDuration = forecastDuration.get(o.Id);
            Decimal monthlySett = o.Annual_Settlement_Value__c / fcstDuration;
            Decimal annualTxnRev = o.Annual_transaction_volume__c*o.Monthly_Txn_Revenue_POS__c;
            Decimal monthlyTxnRev = annualTxnRev/fcstDuration;

            //FSR calculations
            Integer fsrDur = forecastDuration.get(o.Id) - rDur;

            Decimal settLoopTotal = 0.00;
            Decimal txnrevLoopTotal = 0.00;

            //add Monthly Forecast records for 'ramp' months
            for(Integer m = 0; m < rDur; m++){

                Decimal ramp = increment*(m + 1);
                Decimal monthSett = monthlySett*ramp;
                Decimal monthTxnRev = monthlyTxnRev*ramp;

                Monthly_Revenue_Forecast__c fm = new Monthly_Revenue_Forecast__c();
                fm.Opportunity__c = o.Id;
                fm.CurrencyIsoCode = o.CurrencyIsoCode;
                fm.Name = monthName.get(ic.addMonths(m).month()) +' '+ ic.addMonths(m).year();
                fm.Revenue_Date__c = ic.addMonths(m + 1).toStartOfMonth() - 1;
                fm.Monthly_Settlement1__c = monthSett;
                fm.Transaction_Revenue__c = monthTxnRev;

                settLoopTotal = settLoopTotal + monthSett;
                txnrevLoopTotal = txnrevLoopTotal + monthTxnRev;

                newMRFs.add(fm);

                if(m == rDur - 1){
                    rampSett.put(o.Id,settLoopTotal);
                    rampTxnRev.put(o.Id,txnrevLoopTotal);
                    settLoopTotal = 0;
                    txnrevLoopTotal = 0;
                }
            }

            Decimal mnthlyFsrSett = (o.Annual_Settlement_Value__c - rampSett.get(o.Id)) / fsrDur;
            Decimal mnthlyFsrTxnRev = (annualTxnRev - rampTxnRev.get(o.Id)) / fsrDur;

            //add Monthly Forecast Records for rFSR Months
            for(Integer m = rDur; m < rDur + fsrDur; m++){

                Monthly_Revenue_Forecast__c fm = new Monthly_Revenue_Forecast__c();
                fm.Opportunity__c = o.Id;
                fm.CurrencyIsoCode = o.CurrencyIsoCode;
                fm.Name = monthName.get(ic.addMonths(m).month()) +' '+ ic.addMonths(m).year();
                fm.Revenue_Date__c = ic.addMonths(m + 1).toStartOfMonth() - 1;
                fm.Monthly_Settlement1__c = mnthlyFsrSett;
                fm.Transaction_Revenue__c = mnthlyFsrTxnRev;

                newMRFs.add(fm);
            }
        }
    }

    if(upOpps.size() > 0){

        for(Opportunity o : [SELECT Id,
                            (SELECT Id FROM Monthly_Revenue_Forecasts__r) FROM Opportunity
                              WHERE Id IN :upOpps])
        {               

            if(o.Monthly_Revenue_Forecasts__r.size()> 0){
                existMRFs.addAll(o.Monthly_Revenue_Forecasts__r);
            }
        }
        delete existMRFs;

        for(Opportunity o : [SELECT Id,Name,Implementation_Revenue__c,Implementation_Revenue_as_a_of_FSR__c,
                                    Annual_Settlement_Value__c,Annual_transaction_volume__c,Monthly_Txn_Revenue_POS__c,
                                    CurrencyIsoCode 
                               FROM Opportunity
                              WHERE Id IN :upOpps])
        {

            Date ic = o.Implementation_Revenue__c;
            Integer icm = o.Implementation_Revenue__c.month();//DELETE?

            //Ramp calculations
            Decimal increment = o.Implementation_Revenue_as_a_of_FSR__c / 100;
            Integer rDur = (1 / increment).setscale(0,roundingMode.DOWN).intValue();
            Integer fcstDuration = forecastDuration.get(o.Id);
            Decimal monthlySett = o.Annual_Settlement_Value__c / fcstDuration;
            Decimal annualTxnRev = o.Annual_transaction_volume__c*o.Monthly_Txn_Revenue_POS__c;
            Decimal monthlyTxnRev = annualTxnRev / fcstDuration;

            //FSR calculations
            Integer fsrDur = forecastDuration.get(o.Id) - rDur;

            Decimal settLoopTotal = 0.00;
            Decimal txnrevLoopTotal = 0.00;

            //add Monthly Forecast records for 'ramp' months
            for(Integer m = 0; m < rDur; m++){

                Decimal ramp = increment*(m + 1);
                Decimal monthSett = monthlySett*ramp;
                Decimal monthTxnRev = monthlyTxnRev*ramp;

                Monthly_Revenue_Forecast__c fm = new Monthly_Revenue_Forecast__c();
                fm.Opportunity__c = o.Id;
                fm.CurrencyIsoCode = o.CurrencyIsoCode;
                fm.Name = monthName.get(ic.addMonths(m).month()) +' '+ ic.addMonths(m).year();
                fm.Revenue_Date__c = ic.addMonths(m + 1).toStartOfMonth() - 1;
                fm.Monthly_Settlement1__c = monthSett;
                fm.Transaction_Revenue__c = monthTxnRev;

                settLoopTotal = settLoopTotal + monthSett;
                txnrevLoopTotal = txnrevLoopTotal + monthTxnRev;

                newMRFs.add(fm);

                if(m == rDur - 1){
                    rampSett.put(o.Id,settLoopTotal);
                    rampTxnRev.put(o.Id,txnrevLoopTotal);
                    settLoopTotal = 0;
                    txnrevLoopTotal = 0;
                }
            }

            Decimal mnthlyFsrSett = (o.Annual_Settlement_Value__c - rampSett.get(o.Id)) / fsrDur;
            Decimal mnthlyFsrTxnRev = (annualTxnRev - rampTxnRev.get(o.Id)) / fsrDur;

            //add Monthly Forecast Records for rFSR Months
            for(Integer m = rDur; m < rDur + fsrDur; m++){

                Monthly_Revenue_Forecast__c fm = new Monthly_Revenue_Forecast__c();
                fm.Opportunity__c = o.Id;
                fm.CurrencyIsoCode = o.CurrencyIsoCode;
                fm.Name = monthName.get(ic.addMonths(m).month()) +' '+ ic.addMonths(m).year();
                fm.Revenue_Date__c = ic.addMonths(m + 1).toStartOfMonth() - 1;
                fm.Monthly_Settlement1__c = mnthlyFsrSett;
                fm.Transaction_Revenue__c = mnthlyFsrTxnRev;

                //if test for empty list?
                newMRFs.add(fm);
            }
        }
    }
    insert newMRFS;
}

UPDATE

When I reduce the number of records which are inserted / updated in my test Class, a different error message is displayed

Too many DML rows: 10001

UPDATE

I've now isolated the cause of the DML row issue to the second inner loop for the section of my code that starts if(upOpps.size() > 0){. If I comment out the section, see below, the trigger passes my test successfully.

            Decimal mnthlyFsrSett = (o.Annual_Settlement_Value__c - rampSett.get(o.Id)) / fsrDur;
            Decimal mnthlyFsrTxnRev = (annualTxnRev - rampTxnRev.get(o.Id)) / fsrDur;

            //add Monthly Forecast Records for rFSR Months
          /*  For(Integer m = rDur; m < rDur + fsrDur; m++){

                Monthly_Revenue_Forecast__c fm = new Monthly_Revenue_Forecast__c();
                fm.Opportunity__c = o.Id;
                fm.CurrencyIsoCode = o.CurrencyIsoCode;
                fm.Name = monthName.get(ic.addMonths(m).month()) +' '+ ic.addMonths(m).year();
                fm.Revenue_Date__c = ic.addMonths(m + 1).toStartOfMonth() - 1;
                fm.Monthly_Settlement1__c = mnthlyFsrSett;
                fm.Transaction_Revenue__c = mnthlyFsrTxnRev;

                //if test for empty list?
                newMRFs.add(fm);
            }*/
        }
    }
    insert newMRFS;
}

This is pretty odd as the inner loop is a copy from the if(insOpps.size() > 0){ section, which does have code coverage.

The size of the upOpps list is 0, if I add the debug line just before or just after if(upOpps.size() > 0){ whereas insOpps size is 200.

But my test class inserts the 200 Opportunities and then does update them - I have code coverage for the lines after the upOpps size if test.

Test Class

        //create Opportunitys to update, in bulk
    List<Opportunity> opportunitys = new List<Opportunity>();

    for (Integer i = 0; i < 200; i++) {

        Opportunity o = new Opportunity();
        o.name = 'Test Opp' + i;
        o.RecordTypeId = '012700000005qif';
        o.OwnerId = u.Id;

        o.StageName = '1) Suspect';
        o.CloseDate = date.newInstance(2015,06,15);
        o.Implementation_Revenue__c = date.newInstance(2015, 08, 15);
        o.FSR_End_Date__c = date.newInstance(2016, 08, 15);
        o.Implementation_Revenue_as_a_of_FSR__c = 40;

        //cover Forecast validation rules
        o.Monthly_Txn_Revenue_POS__c = 0;
        o.Annual_transaction_volume__c = 0;

        //cover Credit Agreement Details Required validation rule
        o.Credit_Limit__c = 100.00;
        o.Date_Credit_Limit_Approved__c = date.newInstance(1901, 01, 01);
        o.Payment_Terms_Days__c = 5;
        o.Billing_Cycle__c = 'Daily';

        //cover Revenue Values Required validation rule
        o.Annual_Settlement_Value__c = 100.00;
        o.Average_transaction_Value_POS__c = 1.00;

        opportunitys.add(o);
    }
    insert opportunitys;
    system.debug('opportunitys contains ' + opportunitys);

    for(Opportunity upOppsDate : opportunitys){
        upOppsDate.CloseDate = date.newInstance(2015,07,15);
    }
    update opportunitys;

Answer 1

There are a number of issues with your code. So many that I don't want to go into detail with them all. First is that it would be very helpful to you to separate your initial test for data into an

If(Trigger.isInsert)
{
   for(Opportunity o : Trigger.New)
   {
      forecastDuration.put(o.Id, o.Implementation_Revenue__c.monthsBetween(o.FSR_End_Date__c));

   {

    upOpps.addAll(trigger.new);
{ 

// there's nothing in trigger.old to test against, so no need in AfterInsert

If(Trigger.isUpdate)
{
   for(Opportunity o : Trigger.New)
   {

       // record selection code here
   {

{

Opportunity oldO = Trigger.OldMap.get(o.Id);

For you selection criteria in AfterUpdate:

// try{
//              Opportunity oldO = Trigger.OldMap.get(o.Id);

You don't want to use a try-catch block in your trigger. This is cpu intensive. With separate blocks, this is not necessary. Test it and see. If fields aren't required or validated, do a null test instead.

// record selection code 

for(Opportunity o : Trigger.New)
{
   if(oldO.CloseDate != o.CloseDate
      ||
      oldO.Annual_Settlement_Value__c != o.Annual_Settlement_Value__c
      ||
      oldO.Implementation_Revenue__c != o.Implementation_Revenue__c
      ||
      oldO.Implementation_Revenue_as_a_of_FSR__c != oldO.Implementation_Revenue_as_a_of_FSR__c
      ||
      oldO.FSR_End_Date__c != o.FSR_End_Date__c
      ||
      oldO.Monthly_Txn_Revenue_POS__c != o.Monthly_Txn_Revenue_POS__c
      ||
      oldO.Annual_transaction_volume__c != o.Annual_transaction_volume__c)
      {
      upOpps.add(o);
      forecastDuration.put(o.Id, o.Implementation_Revenue__c.monthsBetween(o.FSR_End_Date__c));

{

This is SIX OR conditions to test at the same time. It might behoove you to break these up into 6 If conditions where you add the ID's to a set. Once you have them, at the end, add all of them to a single set. Let's call it OupIds for our purposes.

You can convert OupIds to a list from trigger.newmap to get your list of opportunities and create your map for the oppIds that are in the set should you need to. I suspect that a test will show that doing it this way will prove faster, but don't have comparable data at the moment I can test with to prove one way or another. I can only guarantee that it will be faster than repeating the try-catch that you're using.

for(Opportunity o : [SELECT Id,Name,Implementation_Revenue__c,Implementation_Revenue_as_a_of_FSR__c,
                                    Annual_Settlement_Value__c,Annual_transaction_volume__c,Monthly_Txn_Revenue_POS__c,
                                    CurrencyIsoCode 
                               FROM Opportunity
                              WHERE Id IN :insOpps])
        {

You don't need to run this query. You will already have all of this info. It should be contained in trigger.new or trigger.newmap for the Ids that you've determined need to be processed.

I would run the following loop on OupIds, not on the object and pull the data from trigger.newmap.

For(Id oid:OupIds)
{

   Date ic = trigger.newmap.get(oid).Implementation_Revenue__c;
   Integer icm = trigger.newmap.get(oid).Implementation_Revenue__c.month();//DELETE?

   //Ramp calculations
   Decimal increment = trigger.newmap.get(oid).Implementation_Revenue_as_a_of_FSR__c / 100;

   // You're inside of a loop, don't define each of these variables each time through!!! 
   // Initialize them once and only once outside the loop instead

   Integer rDur = (1 / increment).setscale(0,roundingMode.DOWN).intValue();
   Integer fcstDuration = forecastDuration.get(oid);
   Decimal monthlySett = trigger.newmap.get(oid).Annual_Settlement_Value__c / fcstDuration;
   Decimal annualTxnRev = trigger.newmap.get(oid)..Annual_transaction_volume__c*trigger.newmap.get(oid).Monthly_Txn_Revenue_POS__c;
   Decimal monthlyTxnRev = annualTxnRev/fcstDuration;

   //FSR calculations
   Integer fsrDur = forecastDuration.get(oId) - rDur;

   Decimal settLoopTotal = 0.00;
   Decimal txnrevLoopTotal = 0.00;

   //add Monthly Forecast records for 'ramp' months
   for(Integer m = 0; m < rDur; m++){

   Decimal ramp = increment*(m + 1);
   Decimal monthSett = monthlySett*ramp;
   Decimal monthTxnRev = monthlyTxnRev*ramp;

   // ### same comments here on these variables, you're recreating them within an inner loop!!!

   Monthly_Revenue_Forecast__c fm = new Monthly_Revenue_Forecast__c();
   fm.Opportunity__c = oId;
   fm.CurrencyIsoCode = trigger.newmap.get(oid).CurrencyIsoCode;
   fm.Name = monthName.get(ic.addMonths(m).month()) +' '+ ic.addMonths(m).year();
   fm.Revenue_Date__c = ic.addMonths(m + 1).toStartOfMonth() - 1;
   fm.Monthly_Settlement1__c = monthSett;
   fm.Transaction_Revenue__c = monthTxnRev;

   settLoopTotal = settLoopTotal + monthSett;
   txnrevLoopTotal = txnrevLoopTotal + monthTxnRev;

   newMRFs.add(fm);

   if(m == rDur - 1){
      rampSett.put(oid,settLoopTotal);
      rampTxnRev.put(oid,txnrevLoopTotal);
      settLoopTotal = 0;
      txnrevLoopTotal = 0;
   }
}

I'm certain you can see where these comments would apply to the remaining portions of your code.