3

I am working in some old Apex classes and found the following code:

// Get all plans closed 90+ days ago
List<Plan__c> plans;
plans = [select Id , EndDate__c  from Plan__c where RecordTypeId in :recordTypeIds
        and LineItemStatus__c = 'Closed' and EndDate__c < :checkDate and EndDate__c > :limitDate                 
        order by EndDate__c desc ];
        
// Get all PlanRenown__c pointers to those plans
List<id> planIds = new List<id>();
for (Plan__c p : plans)
{
    planIds.add(p.Id);
}  

After doing some searching I found that this whole section, from my understanding, an be optimized to:

// EndDate__c has been removed since it was never actually used in the original code
Set<Id> idsOfPlansClosedAtLeast90DaysAgo = new Map<Id, Plan__c>([SELECT Id FROM Plan__c
  WHERE RecordTypeId IN :recordTypeIds AND LineItemStatus__c = 'Closed' 
  AND EndDate__c < :checkDate AND EndDate__c > :limitDate ORDER BY EndDate__c DESC
]).keyset();

However, in my searching the mentions of using this method to get a set of IDs from a query are from ~10 years ago. I was curious if this is still, or ever was, considered "good practice?"

2
  • 1
    That's not coercion.
    – Adrian Larson
    Jan 18 at 17:52
  • 1
    I use the second pattern (idiom) all the time; I might rename the variable as PlanIdsClosedAtLeast90DaysAgo
    – cropredy
    Jan 18 at 23:16

2 Answers 2

4

This approach is completely fine in my eyes because:

  • Putting the result of a query into the Map constructor is a well-known thing
  • Getting the keyset of a map is a simple, well-known method
  • This doesn't try to do too much in a single line

It may take a junior dev a little time (or a brief explanation) to understand what's going on, but I don't think this violates the principle of least surprise. If you only need the Ids, then this map-keyset approach saves a bit of typing.

There are surely still loops happening in the map-keyset approach, they're just implicit/done behind the scenes. So I wouldn't expect there to be much measurable impact (if any) on cpu/heap metrics. There is actually a measurable impact (thanks to sfdcfox for benchmarking it). Still, if you're bumping up against those governor limits, there are probably better things in your codebase to try to tweak to rein things in.

The biggest benefit to me is the reduction in typing without sacrificing much in the way of readability/maintainability.

3
  • 4
    I tested this out, actually, because I was curious. Given 1,000 iterations of extracting the ID value from 200 records, the new map/keyset method took about 1.9 seconds (1.96ms per 200 records), while the for loop took 12.6 seconds (12.6ms per 200), nearly an order of magnitude slower. I do agree it's probably not the thing that'd be throwing CPU timeouts, but this is an optimization worth considering.
    – sfdcfox
    Jan 18 at 16:37
  • @sfdcfox Yikes! I didn't think it would be measurable, certainly not under pretty normal conditions.
    – Derek F
    Jan 18 at 16:40
  • I chose 1,000 because, like you, I felt I wouldn't see a significant difference on a smaller benchmark. Even 10 iterations on 200 records shows nearly a 100ms difference (18 ms vs. 111ms). Even a mildly complicated algorithm would definitely see massive improvements with "One Weird Trick", lol.
    – sfdcfox
    Jan 18 at 16:45
4

It depends on your definition of "good practice." The goal for most programming is to be efficient as possible, because that provides a good user experience. The main benefit of using the Map/keySet solution is that you use less memory and less CPU time. The tradeoff is you don't get to keep the details from the query (e.g. if you needed the EndDate__c field for some reason).

There's another alternative that works well if you want the details and want better performance. You can bind a record list directly to a lookup field in the filters, avoiding the need to extract the Id values. To use your first example:

// Get all plans closed 90+ days ago
List<Plan__c> plans = [select Id , EndDate__c  from Plan__c where RecordTypeId in :recordTypeIds
        and LineItemStatus__c = 'Closed' and EndDate__c < :checkDate and EndDate__c > :limitDate                 
        order by EndDate__c desc ];
        
PlanRenown__c[] renowns = [SELECT Id FROM PlanRenown__c WHERE Plan__c IN :plans];

This allows you to keep the details you query, and not have to extract the ID values.

4
  • Interesting! I forgot to mention but one other thing I found is the EndDate__c was never actually used and is getting queried for no reason. But good to know the map keyset solution is efficient, I was worried this was the type of thing that is more concise but in reality much slower than other methods
    – Shades
    Jan 18 at 16:05
  • 2
    Fox, do you think there may have been a difference in the two approaches back in the era where we had the "script statement" governor limit instead of the cpu limit?
    – Derek F
    Jan 18 at 16:16
  • 4
    @DerekF Yes, the 200k "statement" governor limit meant that a query of, say, 200 records would use 400 lines of statements to grab those ID values, but the constructor method used just 1. Even today, however, there is a significant CPU time savings using the constructor method over a for loop, because even though the statement checks are gone, governor limits are still checked between statements, so fewer statements means fewer checks, so more speed. The constructor also allocates heap far more effectively if you read the logs.
    – sfdcfox
    Jan 18 at 16:20
  • 2
    @Shades No, it's definitely faster. The more stuff you can do in "library" code, rather than direct Apex code, the faster your overall code will run. Notably, for loops in Apex have to construct an iterator, and inefficiently allocate heap memory. The constructor gets to do this more efficiently, so it will save both heap and CPU time.
    – sfdcfox
    Jan 18 at 16:21

You must log in to answer this question.

Not the answer you're looking for? Browse other questions tagged .