Skip to content

Commit

Permalink
Merge pull request #238 from green-code-initiative/ISSUE_112_recup
Browse files Browse the repository at this point in the history
[ISSUE 112] [CRJVM208] get stale code of PRs 151 and 157 to merge them into this…
  • Loading branch information
dedece35 authored Dec 4, 2023
2 parents 3470db6 + ad8d17b commit 20d1826
Show file tree
Hide file tree
Showing 12 changed files with 334 additions and 75 deletions.
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
- [#128](https://github.com/green-code-initiative/ecoCode/pull/128) Adding EC35 rule for Python and PHP : EC35 rule replaces EC34 with a specific use case ("file not found" sepcific)
- [#132](https://github.com/green-code-initiative/ecoCode/issues/132) Upgrade RULES.md: set proposed Python rule "Use numpy array instead of standard list" as refused with link to the justification
- [#103](https://github.com/green-code-initiative/ecoCode/issues/103) Upgrade RULES.md: set proposed HTML rule "HTML page must contain a doctype tag" as refused with link to the justification
- [#112](https://github.com/green-code-initiative/ecoCode/issues/112) Updating EC1 rule to add controls on streams
- [#247](https://github.com/green-code-initiative/ecoCode/issues/247) Disable EC2 rule for float and double comparison for java language

### Deleted
Expand Down
2 changes: 1 addition & 1 deletion RULES.md
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@ Some are applicable for different technologies.
| EC69 | Calling a function in the declaration of a for loop | Avoid calling the function each time the loop is iterated. | [cnumr best practices (3rd edition) BP_069 (no longer exists in edition 4)](https://www.greenit.fr/2019/05/07/ecoconception-web-les-115-bonnes-pratiques-3eme-edition/) ||| 🚀 || 🚀 |
| EC72 | Perform an SQL query inside a loop | Servers are optimized to process multiple selections, insertions, or changes in a single query or transaction. consume CPU cycles, RAM, and bandwidth unnecessarily. | [cnumr best practices (3rd edition) BP_072](https://github.com/cnumr/best-practices/blob/main/chapters/BP_072_fr.md) ||| 🚀 || 🚀 |
| EC74 | Write SELECT * FROM | The database server must resolve the fields based on the schema. If you are familiar with the diagram, it is strongly recommended to name the fields. | [cnumr best practices (3rd edition) BP_074 (no longer exists in edition 4)](https://www.greenit.fr/2019/05/07/ecoconception-web-les-115-bonnes-pratiques-3eme-edition/) ||| 🚀 || 🚀 |
| EC1 | Calling a Spring repository inside a loop | The use of Spring repository in a loop induces unnecessary calculations by the CPU and therefore superfluous energy consumption. | || 🚫 | 🚫 | 🚫 | 🚫 |
| EC1 | Calling a Spring repository inside a loop or a stream | The use of Spring repository in a loop or a stream induces unnecessary calculations by the CPU and therefore superfluous energy consumption. | || 🚫 | 🚫 | 🚫 | 🚫 |
| EC3 | Getting the size of the collection in the loop | When iterating over any collection, fetch the size of the collection in advance to avoid fetching it on each iteration, this saves CPU cycles, and therefore consumes less power. | ||| 🚀 | 🚫 | 🚀 |
| EC2 | Multiple if-else statement | Using too many conditional if-else statements will impact performance since JVM will have to compare the conditions. Prefer using a switch statement instead of multiple if-else if possible, or refactor your code to reduce conditonnal statements on the same variable. Switch statement has a performance advantage over if – else. | ||| 🚀 | 🚧 | 🚀 |
| EC76 | Usage of static collections | Avoid usage of static collections. If you want to use static collections make them final and create for example a singleton if needed containing the collections. The static fields are more complicated for the Garbage Collector to manage and can lead to memory leaks. | || 🚫 | 🚫 | 🚫 | 🚫 |
Expand Down
2 changes: 1 addition & 1 deletion ecocode-rules-specifications/src/main/rules/EC1/EC1.json
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
{
"title": "Avoid Spring repository call in loop",
"title": "Avoid Spring repository call in loop or stream operations",
"type": "CODE_SMELL",
"status": "ready",
"remediation": {
Expand Down
17 changes: 17 additions & 0 deletions ecocode-rules-specifications/src/main/rules/EC1/java/EC1.asciidoc
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
The use of Spring repository in a loop induces unnecessary calculations by the CPU and therefore superfluous energy consumption.
Also, the use of Spring repository in a stream operation like "peek, forEach, forEachOrdered, map" induces unnecessary multiple access to the database instead of single batch call.

## Noncompliant Code Example

Expand All @@ -15,9 +16,25 @@ for (Integer id: ids) {
}
```

```java
List<Employee> employees = new ArrayList<>();
Stream stream = Stream.of(1, 2, 3, 4, 5, 6, 7, 8, 9, 10);
stream.forEach(id -> {
Optional<Employee> employee = employeeRepository.findById(id); // Noncompliant
if (employee.isPresent()) {
employees.add(employee.get());
}
});
```

## Compliant Solution

```java
private final List<Integer> ids = Arrays.asList(1, 2, 3, 4, 5, 6, 7, 8, 9, 10);
List<Employee> employees = employeeRepository.findAllById(ids);
```

```java
private final List<Integer> ids = Stream.of(1, 2, 3, 4, 5, 6, 7, 8, 9, 10).toList();
List<Employee> employees = employeeRepository.findAllById(ids);
```
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@
import fr.greencodeinitiative.java.checks.AvoidRegexPatternNotStatic;
import fr.greencodeinitiative.java.checks.AvoidSQLRequestInLoop;
import fr.greencodeinitiative.java.checks.AvoidSetConstantInBatchUpdate;
import fr.greencodeinitiative.java.checks.AvoidSpringRepositoryCallInLoopCheck;
import fr.greencodeinitiative.java.checks.AvoidSpringRepositoryCallInLoopOrStreamCheck;
import fr.greencodeinitiative.java.checks.AvoidStatementForDMLQueries;
import fr.greencodeinitiative.java.checks.AvoidUsageOfStaticCollections;
import fr.greencodeinitiative.java.checks.AvoidUsingGlobalVariablesCheck;
Expand Down Expand Up @@ -60,7 +60,7 @@ public class JavaCheckRegistrar implements CheckRegistrar {
AvoidRegexPatternNotStatic.class,
NoFunctionCallWhenDeclaringForLoop.class,
AvoidStatementForDMLQueries.class,
AvoidSpringRepositoryCallInLoopCheck.class,
AvoidSpringRepositoryCallInLoopOrStreamCheck.class,
AvoidSQLRequestInLoop.class,
AvoidFullSQLRequest.class,
UseCorrectForLoop.class,
Expand Down

This file was deleted.

Original file line number Diff line number Diff line change
@@ -0,0 +1,123 @@
/*
* ecoCode - Java language - Provides rules to reduce the environmental footprint of your Java programs
* Copyright © 2023 Green Code Initiative (https://www.ecocode.io)
*
* 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 <http://www.gnu.org/licenses/>.
*/
package fr.greencodeinitiative.java.checks;

import java.util.Arrays;
import java.util.List;

import org.sonar.check.Rule;
import org.sonar.plugins.java.api.IssuableSubscriptionVisitor;
import org.sonar.plugins.java.api.semantic.MethodMatchers;
import org.sonar.plugins.java.api.tree.*;
import org.sonarsource.analyzer.commons.annotations.DeprecatedRuleKey;

@Rule(key = "EC1")
@DeprecatedRuleKey(repositoryKey = "greencodeinitiative-java", ruleKey = "GRC1")
public class AvoidSpringRepositoryCallInLoopOrStreamCheck extends IssuableSubscriptionVisitor {

protected static final String RULE_MESSAGE = "Avoid Spring repository call in loop or stream";

private static final String BASE_STREAM = "java.util.stream.BaseStream";
private static final String SPRING_REPOSITORY = "org.springframework.data.repository.Repository";

private static final MethodMatchers SPRING_REPOSITORY_METHOD =
MethodMatchers
.create()
.ofSubTypes(SPRING_REPOSITORY)
.anyName()
.withAnyParameters()
.build();

private static final MethodMatchers STREAM_FOREACH_METHOD =
MethodMatchers
.create()
.ofSubTypes(BASE_STREAM)
.names("forEach", "forEachOrdered", "map", "peek")
.withAnyParameters()
.build();

private final AvoidSpringRepositoryCallInLoopCheckVisitor visitorInFile = new AvoidSpringRepositoryCallInLoopCheckVisitor();
private final StreamVisitor streamVisitor = new StreamVisitor();

private final AncestorMethodVisitor ancestorMethodVisitor = new AncestorMethodVisitor();

@Override
public List<Tree.Kind> nodesToVisit() {
return Arrays.asList(
Tree.Kind.FOR_EACH_STATEMENT, // loop
Tree.Kind.FOR_STATEMENT, // loop
Tree.Kind.WHILE_STATEMENT, // loop
Tree.Kind.DO_STATEMENT, // loop
Tree.Kind.METHOD_INVOCATION // stream
);
}

@Override
public void visitNode(Tree tree) {
if (tree.is(Tree.Kind.METHOD_INVOCATION)) { // stream process
MethodInvocationTree methodInvocationTree = (MethodInvocationTree) tree;
if (STREAM_FOREACH_METHOD.matches(methodInvocationTree)) {
tree.accept(streamVisitor);
}
} else { // loop process
tree.accept(visitorInFile);
}
}

private class AvoidSpringRepositoryCallInLoopCheckVisitor extends BaseTreeVisitor {
@Override
public void visitMethodInvocation(MethodInvocationTree tree) {
if (SPRING_REPOSITORY_METHOD.matches(tree)) {
reportIssue(tree, RULE_MESSAGE);
} else {
super.visitMethodInvocation(tree);
}
}

}

private class StreamVisitor extends BaseTreeVisitor {

@Override
public void visitLambdaExpression(LambdaExpressionTree tree) {
tree.accept(ancestorMethodVisitor);
}

}

private class AncestorMethodVisitor extends BaseTreeVisitor {

@Override
public void visitMethodInvocation(MethodInvocationTree tree) {
// if the method is a spring repository method, report an issue
if (SPRING_REPOSITORY_METHOD.matches(tree)) {
reportIssue(tree, RULE_MESSAGE);
} else { // else, check if the method is a method invocation and check recursively
if (tree.methodSelect().is(Tree.Kind.MEMBER_SELECT)) {
MemberSelectExpressionTree memberSelectTree = (MemberSelectExpressionTree) tree.methodSelect();
if ( memberSelectTree.expression().is(Tree.Kind.METHOD_INVOCATION)) {
MethodInvocationTree methodInvocationTree = (MethodInvocationTree) memberSelectTree.expression();
methodInvocationTree.accept(ancestorMethodVisitor);
}
}
}
}

}

}
Original file line number Diff line number Diff line change
Expand Up @@ -21,16 +21,15 @@
import org.springframework.data.jpa.repository.JpaRepository;

import java.util.*;
import java.util.stream.Collectors;

public class AvoidRepositoryCallInLoopCheck {
public class AvoidSpringRepositoryCallInLoopCheck {
@Autowired
private EmployeeRepository employeeRepository;

public List<Employee> smellGetAllEmployeesByIds(List<Integer> ids) {
List<Employee> employees = new ArrayList<>();
for (Integer id : ids) {
Optional<Employee> employee = employeeRepository.findById(id); // Noncompliant {{Avoid Spring repository call in loop}}
Optional<Employee> employee = employeeRepository.findById(id); // Noncompliant {{Avoid Spring repository call in loop or stream}}
if (employee.isPresent()) {
employees.add(employee.get());
}
Expand All @@ -39,10 +38,20 @@ public List<Employee> smellGetAllEmployeesByIds(List<Integer> ids) {
}

public class Employee {
private Integer id;
private String name;

public Employee(Integer id, String name) {
this.id = id;
this.name = name;
}

public Integer getId() { return id; }
public String getName() { return name; }
}

public interface EmployeeRepository extends JpaRepository<Employee, Integer> {

}

}
Loading

0 comments on commit 20d1826

Please sign in to comment.