isoman isoman - 3 months ago 9
Java Question

potential bug using removeAll() called by a Collection on itself

During a code review using Sonar , the following code has been detected as a bad one:

ArrayList<String> ops = new ArrayList<String>();
ops.add("test");
ops.removeAll(ops);


Sonar is complaining about the removeAll on called by the collection on itself.

I agree that it's ugly but can this introduce bugs?

NB: This is not my code , I'm reviewing it .

Answer

The issue is whether a ConcurrentModificationException, or list corruption, or endless looping, or failure to remove entries, or similar may result.

ArrayList, specifically, in Oracle's JDK8, seems to be written such that those issues won't occur.

Does that mean that that code is okay, then?

No, it's not okay.

That code:

  • Relies on the implementation of the list's removeAll to be smart enough to handle a very strange use-case

  • Is unnecessarily complex to read and understand, thus creating maintenance issues

  • Is doing unnecessary work, thus taking longer to do its job than it needs to (not that this is likely to be a big deal)

You said this in in the context of code review. I'd flag it and talk with the author about why they used it, and explain why ops.clear(); or ops = new ArrayList<String>(); (depending on context) would almost certainly be a better choice, from a reliability, maintenance, and (very minor) performance perspective.

Comments