potenziale bug utilizzando removeAll() chiamato da una raccolta su se stessa

Question

Durante una revisione del codice utilizzando Sonar, il seguente codice è stato rilevato come uno cattivo:

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

Sonar si lamentano della removeAll sulla chiamato dal raccolta su se stessa.

Sono d'accordo che è brutto ma questo può introdurre dei bug?

NB: questo non è il mio codice, lo sto rivedendo.

Answer 1

Il problema è se un ConcurrentModificationException o un danneggiamento dell'elenco o un ciclo infinito o la mancata rimozione di voci o simili potrebbe causare.

ArrayList, in particolare, nel JDK8 di Oracle, sembra essere scritto in modo tale che tali problemi non si verifichino.

Significa che quel codice è ok, allora?

No, non va bene.

Quel codice:

• si basa sulla realizzazione del del removeAll essere abbastanza intelligente per gestire uno strano caso d'uso

• è inutilmente complesso da leggere e capire, creando così la lista problemi di manutenzione

• Sta facendo lavoro non necessario, quindi impiega più tempo a fare il suo lavoro di quello che è necessario (non che questo sia probabilmente un grosso problema)

L'hai detto nel contesto della revisione del codice. Lo contrassegnerei e parlerei con l'autore del motivo per cui l'hanno usato, e spiegheremo perché ops.clear(); o ops = new ArrayList<String>(); (a seconda del contesto) sarebbe quasi certamente una scelta migliore, da un punto di vista dell'affidabilità, della manutenzione e (molto minore) delle prestazioni.

Answer 2

Sì, questo introdurrà bug. L'impostazione predefinita removeAll funziona secondo il principio di Iterator, se si modifica la raccolta senza utilizzare l'iteratore, verrà fornito un valore ConcurrentModificationException. Se fornisce questa eccezione o meno dipende dal design interno dello Collection che si sta utilizzando e non può essere considerato affidabile.

Anche se la versione corrente non utilizza un iterator(), questo non è documentato e Oracle PU change cambiare questo senza preavviso.

Per cancellare una raccolta, è possibile utilizzare .clear().

Answer 3

Questo potrebbe assolutamente introdurre bug. In particolare, a seconda dell'implementazione di una raccolta, questo potrebbe generare ConcurrentModificationException.

Per capire come questo possa accadere, prendere in considerazione questa pseudo-implementazione:

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

Nel momento in cui rimuovere un elemento da questa lista, l'iteratore di collection è più valido. Il ciclo esterno per ogni ciclo non può continuare, causando un'eccezione.

Ovviamente l'implementazione effettiva è diversa, evitando questo tipo di bug. Tuttavia, in nessuna parte della documentazione non è garantito che tutte le implementazioni debbano essere "difensive" in questo modo; da qui l'avvertimento.

Answer 4

Più che bug, sembra l'uso errato di removeAll() sugli array. Java doc dice "removeAll()" rimuove tutti gli elementi di questa collezione che sono anche contenuti nella collezione specificata. Al termine della chiamata, questa raccolta non conterrà elementi in comune con la raccolta specificata.

Quindi, se l'intenzione è cancellare tutti gli elementi, clear() è il metodo da chiamare piuttosto che removeAll. Quest'ultimo dovrebbe essere usato se l'intenzione è rimuovere gli elementi che sono anche contenuti nella collezione specificata.

Spero che questo aiuti.

Answer 5

In realtà, dipende dalla versione di jdk che si sta utilizzando. In JDK6, la maggior parte della classe di raccolta ereditano il metodo removeAll dalla classe 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; 
} 

Quindi, in un certo scenario, questo causerà una CME (ConcurrentModificationException), ad esempio:

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

tuttavia

, in jdk8, la maggior parte delle raccolte ha la propria implementazione removeAll, come ArrayList. La classe ArrayList ha il proprio metodo removeAll, che chiamare un metodo batchRemove privato:

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

Questa utilizzando ciclo for iterate sopra la matrice sottostante, e cambiando la variabile modCount sola volta dopo la fine di tale metodo, durante l'iterazione della sottolista (invocando da c.taintains (elementData [r])), la mod modCount non cambia, quindi non verrà generata alcuna CME.