potencjalny błąd używając removeAll() wywołane przez zbiór na siebie

Question

Podczas przeglądu kodu przy użyciu sonaru, następujący kod został wykryty jako złego:

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

Sonar narzekają na removeAll na zwołanym przez kolekcja sama w sobie.

Zgadzam się, że to brzydkie, ale czy może to spowodować błędy?

NB: To nie jest mój kod, przeglądam go.

Answer 1

Problem polega na tym, że może dojść do tego, że może dojść do zniekształcenia list, niekończącej się pętli lub nieusunięcia wpisów.

ArrayList, w szczególności w JDK8 Oracle, wydaje się być napisane tak, że te problemy nie wystąpią.

Czy to oznacza, że ​​ten kod jest w porządku, a następnie?

Nie, to nie w porządku.

Ten kod:

• polega na realizacji lista na removeAll być na tyle silny, aby obsłużyć bardzo dziwny przypadków użycia

• niepotrzebnie skomplikowane do odczytania i zrozumienia, tworząc tym samym problemy z konserwacją

• Wykonuje niepotrzebną pracę, tym samym zajmuje więcej czasu, aby wykonać swoją pracę, niż to konieczne (nie, że to może być wielka sprawa)

Powiedzieliście to w kontekście przeglądu kodu. Zgłaszam je i rozmawiam z autorem o tym, dlaczego go użyli, i wyjaśniam, dlaczego ops.clear(); lub ops = new ArrayList<String>(); (zależnie od kontekstu) byłoby prawie na pewno lepszym wyborem, od rzetelności, konserwacji i (bardzo niewielkiej) perspektywy wydajności.

Answer 2

Tak, spowoduje to wprowadzenie błędów. Domyślnie removeAll działa na zasadzie Iterator, jeśli zmodyfikujesz kolekcję bez użycia iteratora, otrzymasz ConcurrentModificationException. Jeśli daje ten wyjątek, zależy od wewnętrznej konstrukcji używanego modelu Collection i nie można na nim polegać.

Mimo że bieżąca wersja nie korzysta z iterator(), nie jest to udokumentowane, a firma Oracle MOŻE zmienić to bez powiadomienia.

Aby wyczyścić kolekcję, można użyć .clear().

Answer 3

To może absolutnie wprowadzić błędy. Konkretnie, w zależności od implementacji kolekcji może to spowodować wyświetlenie ConcurrentModificationException.

Aby zrozumieć, jak to się stało, należy rozważyć tę pseudo-realizacja:

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; 
      } 
     } 
    } 
} 

Moment usuwamy element z tej listy, iterator z collection staje się nieważny. Zewnętrzna pętla for-each nie może kontynuować, powodując wyjątek.

Oczywiście rzeczywista implementacja jest inna, zapobiegając tego rodzaju błędom. Jednak w dokumentacji nie ma gwarancji, że wszystkie wdrożenia muszą być "obronne" w ten sposób; stąd ostrzeżenie.

Answer 4

To więcej niż błędy, to wygląda na nieprawidłowe użycie metody removeAll() na tablicach. Dokumentacja Java mówi "removeAll()" usuwa wszystkie elementy kolekcji, które są również zawarte w określonej kolekcji. Po zwróceniu tego wywołania ta kolekcja nie będzie zawierać elementów wspólnych dla określonej kolekcji.

Tak więc, jeśli celem jest wyczyszczenie wszystkich elementów, clear() to metoda wywoływania zamiast usuwania. Te ostatnie należy stosować, jeśli celem jest usunięcie elementów, które są również zawarte w określonym zbiorze.

Mam nadzieję, że to pomoże.

Answer 5

Właściwie to zależy od wersji jdk, której używasz. W JDK6, większość klasy zbierania dziedziczą metodę removeAll z klasy AbstractCollection:

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; 
} 

Więc w jakimś scenariuszu, będzie to powodować CME (ConcurrentModificationException), na przykład:

ArrayList<Integer> it = new ArrayList<Integer>(); 
it.add(1); 
it.add(2); 
it.add(3); 
List<Integer> sub = it.subList(0,5); 
it.removeAll(sub); 

jednak w jdk8 większość kolekcji ma własną implementację removeAll, taką jak ArrayList. Klasa ArrayList ma swoją własną metodę removeAll, co nazywamy prywatna metoda 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; 
    } 

Ten użyciem pętli iteracyjne nad bazowego tablicy oraz zmianę zmiennej modCount tylko raz po zakończeniu tej metody, podczas iteracja podlisty (wywołanie przez c. zawiera (elementData [r])), modCount nie zmienia się, więc nie zostanie rzucona CME.