advantage of creating a temporary buffer when iterating over and modifying a collection in java

Question

I am iterating over, and modifying a map (which is created from an existing group of enum objects) like the following:

public class Dispenser {
   private Map<Ingredient, Integer> availableIngredients = 
         new EnumMap<Ingredient, Integer>(Ingredient.class);
   public void orderSandwich(SandwichType sandwichType) {
      Map<Ingredient, Integer> buffer = 
            new EnumMap<Ingredient, Integer>(availableIngredients);
      for (Map.Entry<Ingredient, Integer> entry : 
            sandwichType.getIngredients().entrySet()) {
         Integer currentUnits = buffer.get(entry.getKey());
         buffer.put(entry.getKey(), currentUnits - entry.getValue());
      }  
      availableIngredients.clear();
      availableIngredients.putAll(buffer);
   }
}

I wanted to ask if the temporary, method-local, buffer collection is necessary in this case. I mean, it works fine as it is but not sure its benefits. I have to clear my original collection and replace it with the contents of the buffer collection, which is basically the actual map that is modified within the loop.

Since, it works fine without the buffer collection (using only my original collection), I was wondering if one approach is recommended over the oter and why.

Many thanks for any advice on best practices on this.

Answer 1

It's like to avoid ConcurrentModificationExceptions.

You cannot modify a collection while iterating over it, or such an exception will be thrown. What you have posted is a not-uncommon idiom for dealing with this - take a copy of the collection in one way or another, then you can iterate over one while modifying the other.

This can happen even in single-threaded code - for example, something like this will throw that exception on a collection with at least two elements:

for (Object o : myCollection)
{
   myCollection.remove(o);
}

An alternative, and likely more performant, way to deal with this is to explicitly declare the Iterator (rather than using the foreach loop), and then use the Iterator's remove method, if appropriate. (This doesn't apply to your case, though, because you're remapping rather than removing elements).

Edit: On reflection though, the availableIngredients map isn't being looped over so can simply be modified directly. You're right to be confused as it turns out. :-) Chances are this is vestigial from former refactoring, but it can be replaced by

public void orderSandwich(SandwichType sandwichType) {
  for (Map.Entry<Ingredient, Integer> entry : 
        sandwichType.getIngredients().entrySet()) {
     Integer currentUnits = availableIngredients.get(entry.getKey());
     availableIngredients.put(entry.getKey(), currentUnits - entry.getValue());
  }         
}

as you no doubt expect.

A thought that's just come to mind is that this might also have been a misguided attempt to make concurrency problems "less likely", by reducing the window of conflicting updates. Misguided, though, because threadsafety is absolute; making something ten times less likely to exhibit data races is not a good investment of time. It will still fail, "randomly", and is thus incorrect.

Answer 2

You don't need buffer map. You can operate on availableIngredients instead. Everything will be fine. With no useless overhead.

Answer 3

You do not have to use a buffer in this case, as you iterate over the collection sandwichType.getIngredients() while modifying availableIngredients these are separate collections.

For the cases where using a buffer is a good idea, replacing the original can be done easier like this:

  availableIngredients = buffer;

there is no need to update the original collection, it suffices to use the new version from now on.