diff --git a/its/ruling/src/test/resources/expected/squid-S3047.json b/its/ruling/src/test/resources/expected/squid-S3047.json new file mode 100644 index 00000000000..349dbe3267d --- /dev/null +++ b/its/ruling/src/test/resources/expected/squid-S3047.json @@ -0,0 +1,20 @@ +{ +'project:java/awt/Frame.java':[ +1144, +], +'project:java/awt/Window.java':[ +1614, +], +'project:java/lang/ApplicationShutdownHooks.java':[ +104, +], +'project:java/lang/ProcessEnvironment.java':[ +279, +], +'project:javax/swing/colorchooser/ColorPanel.java':[ +96, +], +'project:javax/swing/plaf/synth/SynthDesktopPaneUI.java':[ +219, +], +} diff --git a/java-checks/src/main/java/org/sonar/java/checks/CheckList.java b/java-checks/src/main/java/org/sonar/java/checks/CheckList.java index 64b97b99904..c818d1e5adf 100644 --- a/java-checks/src/main/java/org/sonar/java/checks/CheckList.java +++ b/java-checks/src/main/java/org/sonar/java/checks/CheckList.java @@ -343,6 +343,7 @@ public static List> getJavaChecks() { .add(ServletMethodsExceptionsThrownCheck.class) .add(DynamicClassLoadCheck.class) .add(MembersDifferOnlyByCapitalizationCheck.class) + .add(LoopsOnSameSetCheck.class) .build(); } diff --git a/java-checks/src/main/java/org/sonar/java/checks/LoopsOnSameSetCheck.java b/java-checks/src/main/java/org/sonar/java/checks/LoopsOnSameSetCheck.java new file mode 100644 index 00000000000..cefa8e7e1ea --- /dev/null +++ b/java-checks/src/main/java/org/sonar/java/checks/LoopsOnSameSetCheck.java @@ -0,0 +1,122 @@ +/* + * SonarQube Java + * Copyright (C) 2012 SonarSource + * sonarqube@googlegroups.com + * + * This program is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser 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 + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with this program; if not, write to the Free Software + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02 + */ +package org.sonar.java.checks; + +import com.google.common.collect.ImmutableList; +import org.sonar.api.server.rule.RulesDefinition; +import org.sonar.check.Priority; +import org.sonar.check.Rule; +import org.sonar.java.checks.helpers.ExpressionsHelper; +import org.sonar.java.model.JavaTree; +import org.sonar.java.model.SyntacticEquivalence; +import org.sonar.java.syntaxtoken.FirstSyntaxTokenFinder; +import org.sonar.plugins.java.api.semantic.Symbol; +import org.sonar.plugins.java.api.tree.BaseTreeVisitor; +import org.sonar.plugins.java.api.tree.BlockTree; +import org.sonar.plugins.java.api.tree.ExpressionTree; +import org.sonar.plugins.java.api.tree.ForEachStatement; +import org.sonar.plugins.java.api.tree.IdentifierTree; +import org.sonar.plugins.java.api.tree.Tree; +import org.sonar.squidbridge.annotations.SqaleConstantRemediation; +import org.sonar.squidbridge.annotations.SqaleSubCharacteristic; + +import java.util.HashMap; +import java.util.HashSet; +import java.util.List; +import java.util.Map; +import java.util.Set; + +@Rule( + key = "S3047", + name = "Multiple loops over the same set should be combined", + tags = {"performance"}, + priority = Priority.MAJOR) +@SqaleSubCharacteristic(RulesDefinition.SubCharacteristics.CPU_EFFICIENCY) +@SqaleConstantRemediation("20min") +public class LoopsOnSameSetCheck extends SubscriptionBaseVisitor { + + @Override + public List nodesToVisit() { + // raise issues only when they appear in the same block + return ImmutableList.of(Tree.Kind.BLOCK); + } + + @Override + public void visitNode(Tree tree) { + Map forEachSymbols = new HashMap<>(); + Set forEachIterables = new HashSet<>(); + for (Tree item : ((BlockTree) tree).body()) { + if (item.is(Tree.Kind.FOR_EACH_STATEMENT)) { + checkForEach(forEachSymbols, forEachIterables, (ForEachStatement) item); + } else { + item.accept(new InvalidatorVisitor(forEachSymbols)); + } + } + } + + private void checkForEach(Map forEachSymbols, Set forEachIterables, ForEachStatement item) { + ExpressionTree expressionTree = ExpressionsHelper.skipParentheses(item.expression()); + if (expressionTree.is(Tree.Kind.IDENTIFIER)) { + checkForEachIdentifier(forEachSymbols, (IdentifierTree) expressionTree); + } else { + checkForEachExpression(forEachIterables, expressionTree); + } + } + + private void checkForEachExpression(Set forEachIterables, ExpressionTree expressionTree) { + for (Tree subTree : forEachIterables) { + if (SyntacticEquivalence.areEquivalent(expressionTree, subTree)) { + addIssue(expressionTree, FirstSyntaxTokenFinder.firstSyntaxToken(subTree).line()); + return; + } + } + forEachIterables.add(expressionTree); + } + + private void checkForEachIdentifier(Map forEachSymbols, IdentifierTree node) { + Symbol symbol = node.symbol(); + if (forEachSymbols.containsKey(symbol)) { + addIssue(node, forEachSymbols.get(symbol)); + } else { + forEachSymbols.put(symbol, ((JavaTree) node).getLine()); + } + } + + private void addIssue(Tree tree, int line) { + addIssue(tree, "Combine this loop with the one that starts on line " + line + "."); + } + + private static class InvalidatorVisitor extends BaseTreeVisitor { + private final Map forEachSymbols; + + InvalidatorVisitor(Map forEachSymbols) { + this.forEachSymbols = forEachSymbols; + } + + @Override + public void visitIdentifier(IdentifierTree tree) { + Symbol symbol = tree.symbol(); + if (forEachSymbols.containsKey(symbol)) { + forEachSymbols.remove(symbol); + } + super.visitIdentifier(tree); + } + } +} diff --git a/java-checks/src/main/resources/org/sonar/l10n/java/rules/squid/S3047.html b/java-checks/src/main/resources/org/sonar/l10n/java/rules/squid/S3047.html new file mode 100644 index 00000000000..3037514e3d7 --- /dev/null +++ b/java-checks/src/main/resources/org/sonar/l10n/java/rules/squid/S3047.html @@ -0,0 +1,28 @@ +

+ When a method loops multiple over the same set of data, + whether it's a list or a set of numbers, + it is highly likely that the method could be made more efficient by combining + the loops into a single set of iterations. +

+ +

Noncompliant Code Example

+
+public void doSomethingToAList(List<String> strings) {
+  for (String str : strings) {
+    doStep1(str);
+  }
+  for (String str : strings) {  // Noncompliant
+    doStep2(str);
+  }
+}
+
+ +

Compliant Solution

+
+public void doSomethingToAList(List<String> strings) {
+  for (String str : strings) {
+    doStep1(str);
+    doStep2(str);
+  }
+}
+
diff --git a/java-checks/src/test/files/checks/LoopsOnSameSetCheck.java b/java-checks/src/test/files/checks/LoopsOnSameSetCheck.java new file mode 100644 index 00000000000..f79cb6dff7a --- /dev/null +++ b/java-checks/src/test/files/checks/LoopsOnSameSetCheck.java @@ -0,0 +1,39 @@ + +import java.lang.Object; +import java.util.List; + +public class A { + private List getList() { + return null; + } + + public void doSomethingToAList(List strings, List strings2) { + for (String str : strings); + for (String str : strings); // Noncompliant {{Combine this loop with the one that starts on line 11.}} + strings = null; + for (String str : strings); // strings get reassigned + for (String str : getList()); + for (String str : getList()); // Noncompliant {{Combine this loop with the one that starts on line 15.}} + for (String str : foo()); + List list = null; + for (String str : list); + for (String str : list); // Noncompliant {{Combine this loop with the one that starts on line 19.}} + if (list.size() > 10) { + list.remove(0); + } + for (String str : list); + if (true) { + for (String str : strings2); + } else { + for (String str : strings2); + } + getList().remove(0); + Object obj = new Object() { + void foo() { + for (String str : strings) { + doStep2(str); + } + } + }; + } +} diff --git a/java-checks/src/test/java/org/sonar/java/checks/LoopsOnSameSetCheckTest.java b/java-checks/src/test/java/org/sonar/java/checks/LoopsOnSameSetCheckTest.java new file mode 100644 index 00000000000..59802161aa0 --- /dev/null +++ b/java-checks/src/test/java/org/sonar/java/checks/LoopsOnSameSetCheckTest.java @@ -0,0 +1,32 @@ +/* + * SonarQube Java + * Copyright (C) 2012 SonarSource + * sonarqube@googlegroups.com + * + * This program is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser 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 + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with this program; if not, write to the Free Software + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02 + */ +package org.sonar.java.checks; + +import org.junit.Test; +import org.sonar.java.checks.verifier.JavaCheckVerifier; + +public class LoopsOnSameSetCheckTest { + + @Test + public void test() { + JavaCheckVerifier.verify("src/test/files/checks/LoopsOnSameSetCheck.java", new LoopsOnSameSetCheck()); + } + +}