Modify rule S5413: reworked rule description for LaYC format (#2905)

This commit is contained in:
Marco Kaufmann 2023-08-18 14:16:29 +02:00 committed by GitHub
parent 289a124d78
commit 92b3f17c07
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23

View File

@ -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<String> 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<String> 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<String> 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<String> 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<String> 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<String> 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<String> 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<String> 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[]