Lors d'une revue de code à l'aide de sonar, le code suivant a été détecté comme un mauvais:
ArrayList<String> ops = new ArrayList<String>();
ops.add("test");
ops.removeAll(ops);
5 Réponses :
Oui, cela introduira des bugs. La valeur par défaut Même si la version actuelle n'utilise pas un Pour effacer une collection, vous pouvez utiliser removeall fonctionne sur le principe d'un itérateur , si vous modifiez la collection sans utiliser l'itérateur, il donnera un ConcurrentModificationException . Si cela donne à cette exception ou non dépend de la conception interne de la collection que vous utilisez et ne peut pas être invoqué.
itérateur () , ce n'est pas documenté et Oracle peut modifier ceci sans préavis.
.clear () .
Que ce soit ou non une exception dépend de la mise en œuvre de la reloveAll de la collection et de déterminer si son itérateur détecte correctement la modification simultanée. Peu importe, ce n'est jamais une bonne idée de le faire lorsque .clear () est garanti de toujours fonctionner
Et vient d'envisager ArrayList 'S removeall . Aucun itérateur.
@ T.J.Crowder Cela dépend de la mise en œuvre, de la flambée et de l'abstraction n'a pas ce bogue par exemple. Mais ce n'est pas un comportement documenté
@Ferrybig: De même, ConcurrentModificationException n'est pas répertorié comme une possibilité pour removeall . Mais il est une exception d'exécution, il n'est donc pas nécessaire que ce soit. Pour être clair: je ne dis pas que le code dans la question est autre chose qu'une très mauvaise idée.
Cela pourrait introduire absolument des bugs. Spécifiquement, en fonction de la mise en œuvre d'une collection, cela pourrait lancer < Code> ConcurrentModificationException .
Pour comprendre comment cela pourrait se produire, considérez cette pseudo-implémentation: Le moment où nous supprimons un élément de Cette liste, l'itérateur de la collection devient invalide. L'extérieur pour chaque boucle est incapable de continuer, provoquant une exception. Bien sûr, la mise en œuvre réelle est différente, empêchant ce type de bugs. Cependant, il n'ya aucune garantie dans la documentation de dire que toutes les mises en œuvre doivent être "défensives" de cette manière; d'où l'avertissement.
mais l'OP n'a pas une seule boucle
@ cy3er Cela ne signifie pas que la mise en œuvre de .reMoveAll () ne contient pas une telle boucle
@ CY3ER J'ai mentionné que ce n'est pas une réelle implémentation de removeall , seule une possibilité que serait un bug. Le problème n'est pas que le bogue est là, mais que le comportement sans bogue n'est pas garanti par la documentation.
Le problème est de savoir si un Cela signifie-t-il que ce code va bien, alors?
Ce code:
repose sur la mise en œuvre de la liste est inutilement complexe de lire et de comprendre, créant ainsi des problèmes de maintenance
fait un travail inutile, prenant ainsi plus de temps à faire son travail qu'il n'a besoin (pas que cela soit probablement une grosse affaire)
Vous avez dit cela dans le contexte de l'examen du code. Je le fermais et parlerais avec l'auteur de la raison pour laquelle ils l'ont utilisé et expliquent pourquoi ConcurrentModificationException ou la corruption de liste, ou une boucle sans fin, ou une défaillance de la suppression des entrées, ou similaire peut résulter.
ArrayList , plus précisément, dans la JDK8 d'Oracle, semble être écrit de sorte que ces problèmes ne se produisent pas.
removeall pour être suffisamment intelligente pour gérer un cas d'utilisation très étrange
ops.clear (); ou ops = nouvelle arrayliste
et une autre raison de coller avec clair est la complexité du temps - Stackoverflow.com/Questtions/7032070/...
De plus, cette utilisation étrange pourrait indiquer que vous passez la mauvaise liste ou appelez removeall sur la mauvaise liste.
Plus que des bugs, il ressemble à la mauvaise utilisation de RemoveAll () sur les tableaux. Java Doc dit "RemoveAll ()" supprime toutes les éléments de cette collection qui sont également contenus dans la collection spécifiée. Une fois que cet appel revient, cette collection ne contiendra aucun élément en commun avec la collection spécifiée.
Donc, si l'intention est d'effacer tous les éléments, Clear () est la méthode d'appel plutôt que de la removeal. Ce dernier doit être utilisé si l'intention est de supprimer les éléments qui sont également contenus dans la collection spécifiée.
J'espère que cela aide.
En réalité, cela dépend de la version de JDK que vous utilisez. Dans JDK6, la plupart de la classe de collecte héritent de la méthode RemoveAll à partir de la classe AbstractCollection: Donc, dans certains scénarios, cela provoquera une CME ( ConcurrentModificationException code>), Par exemple: P> 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;
}
Quelle raison possible auriez-vous pour cela plutôt que
ops.clear () ?Oui il peut; Vous ithétiez sur la même collection que vous supprimez, cela peut conduire à un
ConcurrentModificationException .S'il vous plaît voir ma modification.
La question s'applique toujours: Qu'elles causent un problème au moment de l'exécution, c'est une chose stupide à faire. Il obscurcise son intention, doit travailler plus fort pour atteindre le même résultat, etc.
Il n'est pas clair et peut conduire à des bugs. Il suffit d'utiliser différentes collections. Si vous l'examinez, rejetez-le.
Sonar stupide. la vaincre par
ops2 = ops; ops = removeall (ops2); ha!@ bayou.io C'est juste un travail stupide autour de cela simplement caché un mauvais code. À mon avis, la raison pour laquelle cela devrait être changé est simple: le résultat est équivalent si
Effacer () méthode, mais c'est beaucoup plus complexe et processus de consommation de temps. Un exemple inefficace est la concaténation de nombreuses chaînes INT l'une à l'aide destringbuilder . Bad Way fonctionne, mais cela devrait être fait mieux.@ T.G - Vous avez raison. Peut-être que nous pouvons cloner des ops comme ops2, puis
removeall (ops2) sera sécurisé --- Je ne faisais que plaisanter :)