使用由自己调用的removeAll()的潜在错误
在使用Sonar进行代码审查期间,以下代码被检测为错误代码:
ArrayList ops = new ArrayList(); ops.add("test"); ops.removeAll(ops);
Sonar正在抱怨集合本身所调用的removeAll。
我同意这很丑,但这会引入错误吗?
注意:这不是我的代码,我正在审核它。
问题是是否可能导致ConcurrentModificationException
或列表损坏,无限循环,或无法删除条目或类似情况。
ArrayList
,特别是在Oracle的JDK8中,似乎是这样写的,不会发生这些问题。
这是否意味着那个代码没问题呢?
不,这不好。
那段代码:
-
依赖于列表的
removeAll
的实现,足以聪明地处理一个非常奇怪的用例 -
阅读和理解不必要地复杂,从而产生维护问题
-
正在做不必要的工作,因此需要更长的时间来完成它所需要的工作(不是这可能是一个大问题)
您在代码审查的上下文中说过这一点。 我会标记它,并与作者讨论他们为什么使用它,并解释为什么ops.clear();
或ops = new ArrayList
(从具体情况来看)几乎可以肯定是一个更好的选择,从可靠性,维护和(非常小的)性能角度来看。
是的,这将引入错误。 默认的removeAll
基于Iterator
的原理,如果在不使用迭代器的情况下修改集合,它将给出ConcurrentModificationException
。 如果它提供此exception取决于您正在使用的Collection
的内部设计,并且不能依赖。
即使当前版本不使用iterator()
,也没有记录,Oracle可能会在不事先通知的情况下对其进行更改。
要清除集合,可以使用.clear()
。
这绝对可以引入错误。 具体来说,根据集合的实现,这可能会抛出ConcurrentModificationException
。
要了解这是如何发生的,请考虑以下伪实现:
void removeAll(Collection> collection) { for (Object o : collection) { for (int i = 0 ; i != this.length() ; i++) { Object item = this.get(i); if (item.equals(o)) { this.remove(i); break; } } } }
我们从此列表中删除项目的那一刻, collection
的迭代器变为无效。 外部for-each循环无法继续,导致exception。
当然实际的实现是不同的,防止这种错误。 但是,文档中的任何地方都不能保证所有实现都必须以这种方式“防御”; 因此警告。
不仅仅是bug,它看起来像是对数组的removeAll()的错误用法。 Java doc说“removeAll()”删除了同样包含在指定集合中的所有集合的元素。 此调用返回后,此集合将不包含与指定集合相同的元素。
因此,如果打算清除所有元素,则clear()是调用而不是removeAll的方法。 如果打算删除同样包含在指定集合中的元素,则应使用后者。
希望这可以帮助。
实际上,它取决于您使用的jdk的版本。 在jdk6中,大多数集合类从AbstractCollection类inheritanceremoveAll方法:
public boolean removeAll(Collection> c) { boolean modified = false; Iterator> e = iterator(); while (e.hasNext()) { if (c.contains(e.next())) { e.remove(); modified = true; } } return modified; }
因此,在某些情况下,这将导致CME( ConcurrentModificationException
),例如:
ArrayList it = new ArrayList (); it.add(1); it.add(2); it.add(3); List sub = it.subList(0,5); it.removeAll(sub);
但是,在jdk8中,大多数集合都有自己的removeAll实现,例如ArrayList。 ArrayList类有自己的removeAll方法,它调用私有方法batchRemove:
private boolean batchRemove(Collection> c, boolean complement) { final Object[] elementData = this.elementData; int r = 0, w = 0; boolean modified = false; try { for (; r < size; r++) if (c.contains(elementData[r]) == complement) elementData[w++] = elementData[r]; } finally { // Preserve behavioral compatibility with AbstractCollection, // even if c.contains() throws. if (r != size) { System.arraycopy(elementData, r, elementData, w, size - r); w += size - r; } if (w != size) { // clear to let GC do its work for (int i = w; i < size; i++) elementData[i] = null; modCount += size - w; size = w; modified = true; } } return modified; }
这使用for循环遍历底层数组,并且在该方法结束后仅在子列表的迭代期间更改变量modCount (由c.contains(elementData [r])调用), modCount不会改变,所以不会抛出CME。