3

Background

I'm working on a bugfix for some code that I've written. The code itself generally works fine (bugfix is for an edge case), and the exact code isn't terribly important for this question.

The gist of my code is

Map<Id, SObject> recordsToWorkOn = trigger.newMap.deepClone();
Set<Id> firstRoundOfExclusions = findExclusionsMethod();

recordsToWorkOn.keySet().removeAll(firstRoundOfExclusions);

for(SObject obj :recordsToWorkOn.values()){
    if(secondExclusionTest()){
        // Throw an exception if called async
        // Otherwise, add an error to the record and continue
    }
}

// Other processing logic that shouldn't operate on records excluded in either the 
//   first or second rounds of exclusion.

Question

One of the ways I can think of to perform my second round of exclusions is to remove the key from the map while I'm still iterating over the map.

The following anonymous apex appears to work as intended

Map<Id, Opportunity> testOppMap = new Map<Id, Opportunity>([SELECT Id FROM Opportunity LIMIT 20]);

Integer i = 0;

// Size should be 20, matching the LIMIT statement in the query above
system.debug('size: ' + testOppMap.size());
system.debug(testOppMap);

for(Opportunity opp :testOppMap.values()){
    // Remove every other Opportunity from the Map
    if(Math.mod(i,2) == 0){
        testOppMap.keySet().remove(opp.Id);
    }
    i++;

    system.debug('Opp ' + i + ' in loop: ' + opp.Id);
}

// Size should now be 10
system.debug('size: ' + testOppMap.size());
system.debug(testOppMap);

Size before the loop shows 20, each Opportunity is still available in the Opp loop variable for the entire iteration of the loop after being removed from the Map, size after the loop shows 10.

My question is this:

Is it a good idea to add/remove items from a Map as we iterate over it?

My gut feeling says that this is probably a bad idea (though now that I think about it, map.keySet().removeAll() must do something similar).

It's easy enough to gather Ids of records that match my second exclusion criteria, and removeAll() outside of my loop. I'm just curious to know whether or not there is anything to look out for (edge cases, un-intuitive behavior, etc...).

  • Related: salesforce.stackexchange.com/questions/45453 – sfdcfox Apr 10 '17 at 21:25
  • 1
    You don't need to reference keySet() to remove map element: just use testOppMap.remove(opp.Id); – o-lexi Apr 11 '17 at 0:04
  • @Oleksiy yep, I completely missed that. Referencing the keyset is required, however, if you want to remove more than one key at a time from a map. – Derek F Apr 11 '17 at 12:39
3

In addition to my previous answer here, the documentation says that you should not remove elements of a current collection:

To remove elements while iterating a list, create a new list, then copy the elements you wish to keep. Alternatively, add the elements you wish to remove to a temporary list and remove them after you finish iterating the collection.

Modifying a collection during iteration is not explicitly supported, so you should always create a new map/list or remove them all afterwards (e.g. store keys to remove in a set, then use removeAll after the loop).

  • Interesting find, though their advice seems very targeted toward List for some reason. – Adrian Larson Apr 10 '17 at 21:32
2

I wouldn't worry about removing map keys as you iterate. Here's a simple test to demonstrate that the behavior works as you'd expect:

Map<Id, User> users = new Map<Id, User>([SELECT IsActive FROM User]);
for (Id userId : users.keySet())
    if (!users.get(userId).IsActive)
        users.remove(userId);

Integer expected = [SELECT count() FROM User WHERE IsActive = true];
Integer actual = users.size();
system.assertEquals(expected, actual);

If you try to call get after you call remove, you might cause yourself a NullPointerException, but those should be easy to avoid, or catch before they go live.

  • This is not guaranteed to work, as in the documentation. Use at your own risk. – sfdcfox Apr 10 '17 at 21:30
1

In other languages which allow you to iterate directly over a Map or equivalent thereof, yes it is a very bad idea to remove from the Map while iterating over it. And definitely do not remove from a List/Array while iterating over it.

You'll notice though that your loop starts with: for(Opportunity opp :testOppMap.values()). The .values() function of Map returns a List that is no longer tied to the Map - it's not "live". So be cautious but you should be fine.

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.