diff --git a/CHANGELOG.md b/CHANGELOG.md index 277f0dd..43b38ae 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -16,6 +16,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - upgrade some libraries versions - improve Integration Tests system to be more flexible (add new IT for each rule) +- [#21](https://github.com/green-code-initiative/creedengo-java/issues/21) Improvement: some method calls are legitimate in a for loop expression ### Deleted diff --git a/src/it/java/org/greencodeinitiative/creedengo/java/integration/tests/GCIRulesIT.java b/src/it/java/org/greencodeinitiative/creedengo/java/integration/tests/GCIRulesIT.java index 8b68d55..671b146 100644 --- a/src/it/java/org/greencodeinitiative/creedengo/java/integration/tests/GCIRulesIT.java +++ b/src/it/java/org/greencodeinitiative/creedengo/java/integration/tests/GCIRulesIT.java @@ -29,20 +29,46 @@ void testMeasuresAndIssues() { } + @Test + void testGCI3() { + String projectKey = analyzedProjects.get(0).getProjectKey(); + + List issues = issuesForFile(projectKey, + "src/main/java/org/greencodeinitiative/creedengo/java/checks/AvoidGettingSizeCollectionInForLoopBad.java"); + + assertThat(issues) + .hasSize(2) + .extracting("rule", "message", "line", "textRange.startLine", "textRange.endLine", + "textRange.startOffset", "textRange.endOffset", "severity", "type", "debt", "effort") + .containsExactly( + Tuple.tuple("creedengo-java:GCI3", "Avoid getting the size of the collection in the loop", + 13, 13, 13, 28, 45, MINOR, CODE_SMELL, "5min", "5min"), + Tuple.tuple("creedengo-java:GCI69", "Do not call a function when declaring a for-type loop", + 13, 13, 13, 28, 45, MINOR, CODE_SMELL, "5min", "5min") + ); + + } + @Test void testGCI69() { String projectKey = analyzedProjects.get(0).getProjectKey(); List issues = issuesForFile(projectKey, - "src/main/java/org/greencodeinitiative/creedengo/java/checks/AvoidGettingSizeCollectionInForLoopIgnored.java"); + "src/main/java/org/greencodeinitiative/creedengo/java/checks/NoFunctionCallWhenDeclaringForLoop.java"); assertThat(issues) - .hasSize(1) + .hasSize(4) .extracting("rule", "message", "line", "textRange.startLine", "textRange.endLine", "textRange.startOffset", "textRange.endOffset", "severity", "type", "debt", "effort") .containsExactly( Tuple.tuple("creedengo-java:GCI69", "Do not call a function when declaring a for-type loop", - 18, 18, 18, 15, 27, MINOR, CODE_SMELL, "5min", "5min") + 58, 58, 58, 28, 40, MINOR, CODE_SMELL, "5min", "5min"), + Tuple.tuple("creedengo-java:GCI69", "Do not call a function when declaring a for-type loop", + 66, 66, 66, 34, 46, MINOR, CODE_SMELL, "5min", "5min"), + Tuple.tuple("creedengo-java:GCI69", "Do not call a function when declaring a for-type loop", + 74, 74, 74, 39, 51, MINOR, CODE_SMELL, "5min", "5min"), + Tuple.tuple("creedengo-java:GCI69", "Do not call a function when declaring a for-type loop", + 101, 101, 101, 108, 132, MINOR, CODE_SMELL, "5min", "5min") ); } diff --git a/src/it/test-projects/creedengo-java-plugin-test-project/src/main/java/org/greencodeinitiative/creedengo/java/checks/AvoidGettingSizeCollectionInForLoopBad.java b/src/it/test-projects/creedengo-java-plugin-test-project/src/main/java/org/greencodeinitiative/creedengo/java/checks/AvoidGettingSizeCollectionInForLoopBad.java index d3b8af4..03efe7c 100644 --- a/src/it/test-projects/creedengo-java-plugin-test-project/src/main/java/org/greencodeinitiative/creedengo/java/checks/AvoidGettingSizeCollectionInForLoopBad.java +++ b/src/it/test-projects/creedengo-java-plugin-test-project/src/main/java/org/greencodeinitiative/creedengo/java/checks/AvoidGettingSizeCollectionInForLoopBad.java @@ -4,16 +4,13 @@ import java.util.List; class AvoidGettingSizeCollectionInForLoopBad { - AvoidGettingSizeCollectionInForLoopBad() { - - } public void badForLoop() { - List numberList = new ArrayList(); + final List numberList = new ArrayList(); numberList.add(10); numberList.add(20); - for (int i = 0; i < numberList.size(); i++) { // Noncompliant {{Avoid getting the size of the collection in the loop}} + for (int i = 0; i < numberList.size(); ++i) { // Noncompliant System.out.println("numberList.size()"); } } diff --git a/src/it/test-projects/creedengo-java-plugin-test-project/src/main/java/org/greencodeinitiative/creedengo/java/checks/NoFunctionCallWhenDeclaringForLoop.java b/src/it/test-projects/creedengo-java-plugin-test-project/src/main/java/org/greencodeinitiative/creedengo/java/checks/NoFunctionCallWhenDeclaringForLoop.java index bb5ae32..4232607 100644 --- a/src/it/test-projects/creedengo-java-plugin-test-project/src/main/java/org/greencodeinitiative/creedengo/java/checks/NoFunctionCallWhenDeclaringForLoop.java +++ b/src/it/test-projects/creedengo-java-plugin-test-project/src/main/java/org/greencodeinitiative/creedengo/java/checks/NoFunctionCallWhenDeclaringForLoop.java @@ -1,58 +1,119 @@ -package org.greencodeinitiative.creedengo.java.checks; - +package org.greencodeinitiative.creedengo.java.integration.tests;/* + * creedengo - Java language - Provides rules to reduce the environmental footprint of your Java programs + * Copyright © 2024 Green Code Initiative (https://green-code-initiative.org/) + * + * This program is free software: you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation, either version 3 of the License, or + * (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with this program. If not, see . + */ +import java.util.Iterator; +import java.util.List; +import java.util.ListIterator; +import java.util.Arrays; class NoFunctionCallWhenDeclaringForLoop { - NoFunctionCallWhenDeclaringForLoop(NoFunctionCallWhenDeclaringForLoop mc) { - } public int getMyValue() { return 6; } - public int incrementeMyValue(int i) { + public int incrementeMyValue(final int i) { return i + 100; } public void test1() { - for (int i = 0; i < 20; i++) { + for (int i = 0; i < 20; ++i) { System.out.println(i); - boolean b = getMyValue() > 6; + final boolean b = getMyValue() > 6; + System.out.println(b); } } public void test2() { - String[] cars = {"Volvo", "BMW", "Ford", "Mazda"}; - for (String i : cars) { + final String[] cars = {"Volvo", "BMW", "Ford", "Mazda"}; + for (final String i : cars) { System.out.println(i); } } + // compliant, the function is called only once in the initialization so it's not a performance issue public void test3() { - for (int i = getMyValue(); i < 20; i++) { // Noncompliant {{Do not call a function when declaring a for-type loop}} + for (int i = getMyValue(); i < 20; ++i) { System.out.println(i); - boolean b = getMyValue() > 6; + final boolean b = getMyValue() > 6; + System.out.println(b); } } public void test4() { - for (int i = 0; i < getMyValue(); i++) { // Noncompliant {{Do not call a function when declaring a for-type loop}} + for (int i = 0; i < getMyValue(); ++i) { // Noncompliant {{Do not call a function when declaring a for-type loop}} System.out.println(i); - boolean b = getMyValue() > 6; + final boolean b = getMyValue() > 6; + System.out.println(b); } } public void test5() { - for (int i = 0; i < getMyValue(); incrementeMyValue(i)) { // Noncompliant {{Do not call a function when declaring a for-type loop}} + for (final int i = 0; i < getMyValue(); incrementeMyValue(i)) { // Noncompliant {{Do not call a function when declaring a for-type loop}} System.out.println(i); - boolean b = getMyValue() > 6; + final boolean b = getMyValue() > 6; + System.out.println(b); } } public void test6() { - for (int i = getMyValue(); i < getMyValue(); i++) { // Noncompliant {{Do not call a function when declaring a for-type loop}} + for (int i = getMyValue(); i < getMyValue(); ++i) { // Noncompliant {{Do not call a function when declaring a for-type loop}} System.out.println(i); - boolean b = getMyValue() > 6; + final boolean b = getMyValue() > 6; + System.out.println(b); + } + } + + // compliant, iterators are allowed to be called in a for loop + public void test7() { + final List joursSemaine = Arrays.asList("Lundi", "Mardi", "Mercredi", "Jeudi", "Vendredi", "Samedi", "Dimanche"); + + String jour = null; + // iterator is allowed + for (final Iterator iterator = joursSemaine.iterator(); iterator.hasNext(); jour = iterator.next()) { + System.out.println(jour); + } + + // subclass of iterator is allowed + for (final ListIterator iterator = joursSemaine.listIterator(); iterator.hasNext(); jour = iterator.next()) { + System.out.println(jour); + } + + // iterator called in an indirect way is allowed + for (final OtherClassWithIterator otherClass = new OtherClassWithIterator(joursSemaine.iterator()); otherClass.iterator.hasNext(); jour = otherClass.iterator.next()) { + System.out.println(jour); + } + // but using a method that returns an iterator causes an issue + for (final OtherClassWithIterator otherClass = new OtherClassWithIterator(joursSemaine.iterator()); otherClass.getIterator().hasNext(); jour = otherClass.getIterator().next()) { // Noncompliant {{Do not call a function when declaring a for-type loop}} + System.out.println(jour); } + } } + +class OtherClassWithIterator { + public final Iterator iterator; + + public OtherClassWithIterator(Iterator iterator){ + this.iterator = iterator; + } + + public Iterator getIterator(){ + return iterator; + } +} diff --git a/src/main/java/org/greencodeinitiative/creedengo/java/checks/NoFunctionCallWhenDeclaringForLoop.java b/src/main/java/org/greencodeinitiative/creedengo/java/checks/NoFunctionCallWhenDeclaringForLoop.java index 8ebb8db..b364b55 100644 --- a/src/main/java/org/greencodeinitiative/creedengo/java/checks/NoFunctionCallWhenDeclaringForLoop.java +++ b/src/main/java/org/greencodeinitiative/creedengo/java/checks/NoFunctionCallWhenDeclaringForLoop.java @@ -59,23 +59,30 @@ public void visitNode(Tree tree) { if (null != condition) { method.condition().accept(invocationMethodVisitor); } + // update - // initaliser method.update().accept(invocationMethodVisitor); - method.initializer().accept(invocationMethodVisitor); } private class MethodInvocationInForStatementVisitor extends BaseTreeVisitor { @Override public void visitMethodInvocation(MethodInvocationTree tree) { - if (!lineAlreadyHasThisIssue(tree)) { + if (!lineAlreadyHasThisIssue(tree) && !isIteratorMethod(tree)) { report(tree); return; } super.visitMethodInvocation(tree); } + private boolean isIteratorMethod(MethodInvocationTree tree) { + boolean isIterator = tree.methodSymbol().owner().type().isSubtypeOf("java.util.Iterator"); + String methodName = tree.methodSelect().lastToken().text(); + boolean isMethodNext = methodName.equals("next"); + boolean isMethodHasNext = methodName.equals("hasNext"); + return isIterator && (isMethodNext || isMethodHasNext); + } + private boolean lineAlreadyHasThisIssue(Tree tree) { if (tree.firstToken() != null) { final String classname = getFullyQualifiedNameOfClassOf(tree); diff --git a/src/test/files/AvoidGettingSizeCollectionInForLoopBad.java b/src/test/files/AvoidGettingSizeCollectionInForLoopBad.java index c21f81c..b437655 100644 --- a/src/test/files/AvoidGettingSizeCollectionInForLoopBad.java +++ b/src/test/files/AvoidGettingSizeCollectionInForLoopBad.java @@ -1,37 +1,16 @@ -/* - * creedengo - Java language - Provides rules to reduce the environmental footprint of your Java programs - * Copyright © 2024 Green Code Initiative (https://green-code-initiative.org/) - * - * This program is free software: you can redistribute it and/or modify - * it under the terms of the GNU General Public License as published by - * the Free Software Foundation, either version 3 of the License, or - * (at your option) any later version. - * - * This program is distributed in the hope that it will be useful, - * but WITHOUT ANY WARRANTY; without even the implied warranty of - * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the - * GNU General Public License for more details. - * - * You should have received a copy of the GNU General Public License - * along with this program. If not, see . - */ package org.greencodeinitiative.creedengo.java.checks; -import java.util.Collection; import java.util.ArrayList; import java.util.List; class AvoidGettingSizeCollectionInForLoopBad { - AvoidGettingSizeCollectionInForLoopBad() { - - } public void badForLoop() { - List numberList = new ArrayList(); + final List numberList = new ArrayList(); numberList.add(10); numberList.add(20); - for (int i = 0; i < numberList.size(); i++) { // Noncompliant {{Avoid getting the size of the collection in the loop}} + for (int i = 0; i < numberList.size(); ++i) { // Noncompliant {{Avoid getting the size of the collection in the loop}} System.out.println("numberList.size()"); } } diff --git a/src/test/files/NoFunctionCallWhenDeclaringForLoop.java b/src/test/files/NoFunctionCallWhenDeclaringForLoop.java index 1021462..4232607 100644 --- a/src/test/files/NoFunctionCallWhenDeclaringForLoop.java +++ b/src/test/files/NoFunctionCallWhenDeclaringForLoop.java @@ -1,4 +1,4 @@ -/* +package org.greencodeinitiative.creedengo.java.integration.tests;/* * creedengo - Java language - Provides rules to reduce the environmental footprint of your Java programs * Copyright © 2024 Green Code Initiative (https://green-code-initiative.org/) * @@ -15,59 +15,105 @@ * You should have received a copy of the GNU General Public License * along with this program. If not, see . */ +import java.util.Iterator; +import java.util.List; +import java.util.ListIterator; +import java.util.Arrays; class NoFunctionCallWhenDeclaringForLoop { - NoFunctionCallWhenDeclaringForLoop(NoFunctionCallWhenDeclaringForLoop mc) { - } public int getMyValue() { return 6; } - public int incrementeMyValue(int i) { + public int incrementeMyValue(final int i) { return i + 100; } public void test1() { - for (int i = 0; i < 20; i++) { + for (int i = 0; i < 20; ++i) { System.out.println(i); - boolean b = getMyValue() > 6; + final boolean b = getMyValue() > 6; + System.out.println(b); } } public void test2() { - String[] cars = {"Volvo", "BMW", "Ford", "Mazda"}; - for (String i : cars) { + final String[] cars = {"Volvo", "BMW", "Ford", "Mazda"}; + for (final String i : cars) { System.out.println(i); } } + // compliant, the function is called only once in the initialization so it's not a performance issue public void test3() { - for (int i = getMyValue(); i < 20; i++) { // Noncompliant {{Do not call a function when declaring a for-type loop}} + for (int i = getMyValue(); i < 20; ++i) { System.out.println(i); - boolean b = getMyValue() > 6; + final boolean b = getMyValue() > 6; + System.out.println(b); } } public void test4() { - for (int i = 0; i < getMyValue(); i++) { // Noncompliant {{Do not call a function when declaring a for-type loop}} + for (int i = 0; i < getMyValue(); ++i) { // Noncompliant {{Do not call a function when declaring a for-type loop}} System.out.println(i); - boolean b = getMyValue() > 6; + final boolean b = getMyValue() > 6; + System.out.println(b); } } public void test5() { - for (int i = 0; i < getMyValue(); incrementeMyValue(i)) { // Noncompliant {{Do not call a function when declaring a for-type loop}} + for (final int i = 0; i < getMyValue(); incrementeMyValue(i)) { // Noncompliant {{Do not call a function when declaring a for-type loop}} System.out.println(i); - boolean b = getMyValue() > 6; + final boolean b = getMyValue() > 6; + System.out.println(b); } } public void test6() { - for (int i = getMyValue(); i < getMyValue(); i++) { // Noncompliant {{Do not call a function when declaring a for-type loop}} + for (int i = getMyValue(); i < getMyValue(); ++i) { // Noncompliant {{Do not call a function when declaring a for-type loop}} System.out.println(i); - boolean b = getMyValue() > 6; + final boolean b = getMyValue() > 6; + System.out.println(b); } } + // compliant, iterators are allowed to be called in a for loop + public void test7() { + final List joursSemaine = Arrays.asList("Lundi", "Mardi", "Mercredi", "Jeudi", "Vendredi", "Samedi", "Dimanche"); + + String jour = null; + // iterator is allowed + for (final Iterator iterator = joursSemaine.iterator(); iterator.hasNext(); jour = iterator.next()) { + System.out.println(jour); + } + + // subclass of iterator is allowed + for (final ListIterator iterator = joursSemaine.listIterator(); iterator.hasNext(); jour = iterator.next()) { + System.out.println(jour); + } + + // iterator called in an indirect way is allowed + for (final OtherClassWithIterator otherClass = new OtherClassWithIterator(joursSemaine.iterator()); otherClass.iterator.hasNext(); jour = otherClass.iterator.next()) { + System.out.println(jour); + } + // but using a method that returns an iterator causes an issue + for (final OtherClassWithIterator otherClass = new OtherClassWithIterator(joursSemaine.iterator()); otherClass.getIterator().hasNext(); jour = otherClass.getIterator().next()) { // Noncompliant {{Do not call a function when declaring a for-type loop}} + System.out.println(jour); + } + + } + +} + +class OtherClassWithIterator { + public final Iterator iterator; + + public OtherClassWithIterator(Iterator iterator){ + this.iterator = iterator; + } + + public Iterator getIterator(){ + return iterator; + } }