diff --git a/rules/S5413/java/rule.adoc b/rules/S5413/java/rule.adoc index 816e7f5cd2..c5edd2b527 100644 --- a/rules/S5413/java/rule.adoc +++ b/rules/S5413/java/rule.adoc @@ -1,67 +1,116 @@ == Why is this an issue? -When ``++List.remove()++`` is called it will shrink the list. If this is done inside the ascending loop iterating through all elements it will skip the element after the removed index. +When `List.remove()` is called, the list shrinks, and the indices of all elements following the removed element are decremented by one. +If this operation is performed within a loop that iterates through the elements in ascending order, +it will cause the loop to skip the element immediately following the removed element. +== How to fix it -=== Noncompliant code example +There are three ways how to fix this issue: -[source,java] +1. Replace the loop with a call to `Collection.removeIf()`. This is the preferred solution. +2. Replace the ascending loop with a descending loop. Use this approach if the preferred solution is not possible due to side effects of the loop. +3. Adjust the loop counter within the loop body after the call to `Collection.remove()`. **This approach is not recommended**, because it will raise an issue with rule _S127 - "for" loop stop conditions should be invariant_ + +=== Code examples + +==== Noncompliant code example + +If the loop can be replaced with Java 8's `Collection.removeIf` method, depending on the side effects of the loop and your Java target version, +then this is the preferred solution for this issue. + +[source,java,diff-id=1,diff-type=noncompliant] ---- void removeFrom(List list) { - // expected: iterate over all the elements of the list + // expected: iterate over all list elements for (int i = 0; i < list.size(); i++) { if (list.get(i).isEmpty()) { - // actual: remaining elements are shifted, so the one immediately following will be skipped - list.remove(i); // Noncompliant + list.remove(i); // Noncompliant, next element is skipped } } } ---- +==== Compliant solution -=== Compliant solution - -You can either adjust the loop index to account for the change in the size of the list - -[source,java] ----- -static void removeFrom(List list) { - // expected: iterate over all the elements of the list - for (int i = 0; i < list.size(); i++) { - if (list.get(i).isEmpty()) { - // actual: remaining elements are shifted, so the one immediately following will be skipped - list.remove(i); - i--; - } - } - } ----- -Or preferably it's probably better to rely on Java 8's ``++removeIf++`` method - -[source,java] ----- - static void removeFrom(List list) { - list.removeIf(String::isEmpty); - } ----- -  - - -=== Exceptions - -The descending loop doesn't have this issue, because the index will be correct when we loop in descending order - -[source,java] +[source,java,diff-id=1,diff-type=compliant] ---- void removeFrom(List list) { - for (int i = list.size() - 1; i >= 0; i--) { + list.removeIf(String::isEmpty); // Compliant +} +---- + +==== Noncompliant code example + +If this is not possible due to side effects of the loop, replace the ascending loop with a descending loop. +Descending loops are not affected by decrementing the element indices after the removed element, because they have already been iterated. + +[source,java,diff-id=2,diff-type=noncompliant] +---- +void removeFrom(List list) { + // expected: iterate over all list elements + for (int i = 0; i < list.size(); i++) { if (list.get(i).isEmpty()) { - list.remove(i); + list.remove(i); // Noncompliant, next element is skipped } } } ---- -  + +==== Compliant solution + +[source,java,diff-id=2,diff-type=compliant] +---- +void removeFrom(List list) { + // expected: iterate over all list elements + for (int i = list.size() - 1; i >= 0; i--) { + if (list.get(i).isEmpty()) { + list.remove(i); // Compliant, elements after removed one have already been iterated + } + } +} +---- + +==== Noncompliant code example + +Another way to solve this issue is to adjust the loop counter after the call to `Collection.remove` to account for the index decrement. + +[source,java,diff-id=3,diff-type=noncompliant] +---- +void removeFrom(List list) { + // expected: iterate over all list elements + for (int i = 0; i < list.size(); i++) { + if (list.get(i).isEmpty()) { + list.remove(i); // Noncompliant, next element is skipped + } + } +} +---- + +==== Compliant solution + +**This is not recommanded** because it raises an issue with rule S127. + +[source,java,diff-id=3,diff-type=compliant] +---- +void removeFrom(List list) { + // expected: iterate over all list elements + for (int i = 0; i < list.size(); i++) { + if (list.get(i).isEmpty()) { + list.remove(i); // Compliant due to counter adjust in next line + i--; // Noncompliant with S127! + } + } +} +---- + +== Resources + +=== Documentation + +* https://docs.oracle.com/javase/7/docs/api/java/util/Collection.html#remove(java.lang.Object)[Java SE 7 API Specification: Collection.remove] +* https://docs.oracle.com/javase/8/docs/api/java/util/Collection.html#removeIf-java.util.function.Predicate-[Java SE 8 API Specification: Collection.removeIf] +* https://sonarsource.github.io/rspec/#/rspec/S6068/java[S127 - "for" loop stop conditions should be invariant] ifdef::env-github,rspecator-view[]