Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Clean code attributes and better descriptions complying with Sonar Coding rule guidelines #349

Open
wants to merge 27 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
27 commits
Select commit Hold shift + click to select a range
c3ca195
Rule specifications normalization in progress
pbaumard Aug 9, 2024
bd8a4f1
Tests are now OK
pbaumard Sep 4, 2024
706c181
Add tests for title length and prefix
pbaumard Sep 4, 2024
06e0173
Add java version
pbaumard Sep 4, 2024
dabb897
Fix test unsing unsorted list
pbaumard Sep 4, 2024
a0a2c2e
Add license-maven-plugin and spotless-maven-plugin
pbaumard Sep 25, 2024
f23f81d
Revert section change for EC31
pbaumard Sep 25, 2024
613a418
Better section titles in EC36
pbaumard Sep 25, 2024
3975cd0
Revert tag changes
pbaumard Sep 25, 2024
7d53fc2
Consistent descriptions for EC2
pbaumard Sep 25, 2024
0fa8fc7
Move pom changes to ecocode-rules-specifications
pbaumard Sep 25, 2024
a3add9e
Move link to resources
pbaumard Sep 25, 2024
52c0d78
Typography
pbaumard Sep 25, 2024
abfcfea
Better EC69 title
pbaumard Sep 25, 2024
94c42dc
Style
pbaumard Sep 26, 2024
8470f60
Test refactoring
pbaumard Sep 26, 2024
f714c48
Merge remote-tracking branch 'origin/main' into feature/rule-descript…
pbaumard Oct 24, 2024
7b43fe8
Merge remote-tracking branch 'origin/main' into feature/rule-descript…
pbaumard Oct 24, 2024
ffa58e8
Merge branch 'main' into feature/rule-description-normalization
pbaumard Nov 14, 2024
ee7f507
Use standard sustainability tag instead of eco-design
pbaumard Nov 20, 2024
f2f8eff
Merge remote-tracking branch 'origin/main' into feature/rule-descript…
pbaumard Dec 19, 2024
3d2e6c8
Merge remote-tracking branch 'origin/main' into feature/rule-descript…
pbaumard Jan 2, 2025
4973db6
Merge branch 'main' into feature/rule-description-normalization
pbaumard Jan 3, 2025
d6dbc50
Merge branch 'main' into feature/rule-description-normalization
dedece35 Jan 3, 2025
14b4072
Merge branch 'main' into feature/rule-description-normalization
pbaumard Jan 6, 2025
7fc9af0
Fix GCI82 description
pbaumard Jan 6, 2025
3d041ce
Revert pom formatting changes
pbaumard Jan 13, 2025
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
112 changes: 111 additions & 1 deletion pom.xml
Original file line number Diff line number Diff line change
Expand Up @@ -129,13 +129,54 @@
<java.version>11</java.version>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

all these pom.xml modifications must be in the ecocode-rules-specifications pom.x instead and the current parent pom.xml

all other plugins work with java 17. why do you put java 11 ?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

POM changes have been moved to ecocode-rules-specifications.

Some of the configuration, including java 11 is coming from
https://github.com/green-code-initiative/ecoCode-java/blob/main/pom.xml

Java version could be changed.

<maven.compiler.source>${java.version}</maven.compiler.source>
<maven.compiler.target>${java.version}</maven.compiler.target>
<!-- to prevent message: system modules path not set in conjunction with -source 11 -->
<maven.compiler.release>${java.version}</maven.compiler.release>

<encoding>UTF-8</encoding>
<project.build.sourceEncoding>${encoding}</project.build.sourceEncoding>
<project.reporting.outputEncoding>${encoding}</project.reporting.outputEncoding>

</properties>
<sonar.organization>green-code-initiative</sonar.organization>
<sonar.host.url>https://sonarcloud.io</sonar.host.url>

<junit.jupiter.version>5.9.1</junit.jupiter.version>
<assertJ.version>3.23.1</assertJ.version>
<mockito.version>5.3.1</mockito.version>
<sonarqube.version>10.10.0.2391</sonarqube.version>
<sonar-analyzer-commons.version>2.12.0.2964</sonar-analyzer-commons.version>
</properties>
<dependencies>
<dependency>
<groupId>org.junit.jupiter</groupId>
<artifactId>junit-jupiter</artifactId>
<version>${junit.jupiter.version}</version>
<scope>test</scope>
</dependency>
<dependency>
<groupId>org.sonarsource.api.plugin</groupId>
<artifactId>sonar-plugin-api</artifactId>
<version>${sonarqube.version}</version>
<scope>test</scope>
</dependency>
<dependency>
<groupId>org.sonarsource.analyzer-commons</groupId>
<artifactId>sonar-analyzer-commons</artifactId>
<version>${sonar-analyzer-commons.version}</version>
<scope>test</scope>
</dependency>
<dependency>
<groupId>org.assertj</groupId>
<artifactId>assertj-core</artifactId>
<version>${assertJ.version}</version>
<scope>test</scope>
</dependency>
<dependency>
<groupId>org.mockito</groupId>
<artifactId>mockito-junit-jupiter</artifactId>
<version>${mockito.version}</version>
<scope>test</scope>
</dependency>
</dependencies>
<build>
<pluginManagement>
<plugins>
Expand All @@ -148,6 +189,75 @@
</plugins>
</pluginManagement>
<plugins>
<plugin>
<groupId>org.apache.maven.plugins</groupId>
<artifactId>maven-surefire-plugin</artifactId>
<version>3.1.2</version>
</plugin>
<plugin>
<groupId>com.mycila</groupId>
<artifactId>license-maven-plugin</artifactId>
<version>4.5</version>
<configuration>
<properties>
<owner>Green Code Initiative</owner>
<email>https://green-code-initiative.org</email>
</properties>
<licenseSets>
<licenseSet>
<header>
com/mycila/maven/plugin/license/templates/GPL-3.txt</header>
<includes>
<include>**/*.java</include>
</includes>
</licenseSet>
</licenseSets>
</configuration>
<executions>
<execution>
<id>validate</id>
<goals>
<goal>check</goal>
</goals>
<phase>validate</phase>
</execution>
</executions>
</plugin>
<plugin>
<groupId>com.diffplug.spotless</groupId>
<artifactId>spotless-maven-plugin</artifactId>
<version>2.43.0</version>
<configuration>
<java>
<googleJavaFormat>
<version>1.23.0</version>
</googleJavaFormat>
</java>
<formats>
<format>
<includes>
<include>src/**/*.json</include>
</includes>
<eclipseWtp>
<type>JSON</type>
</eclipseWtp>
<endWithNewline></endWithNewline>
<indent>
<spaces>true</spaces>
<spacesPerTab>2</spacesPerTab>
</indent>
</format>
</formats>
</configuration>
<executions>
<execution>
<goals>
<goal>check</goal>
</goals>
<phase>process-sources</phase>
</execution>
</executions>
</plugin>
<plugin>
<groupId>org.apache.maven.plugins</groupId>
<artifactId>maven-enforcer-plugin</artifactId>
Expand Down
12 changes: 9 additions & 3 deletions src/main/rules/GCI1/GCI1.json
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
{
"title": "Avoid Spring repository call in loop or stream operations",
"title": "Spring repository should not be called in loop or stream operations",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

in my point of view, I prefer a turn of phrase without negation, thus, I prefer "avoid" than "should not be" ... but I can ask for the point of view of core team

Copy link
Author

@pbaumard pbaumard Jan 6, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From Sonar Coding rule guidelines:

The title of the rule should match the pattern "X should [ not ] Y" for most rules. Note that the "should [ not ]" pattern is too strong for Finding rules, which are about observations on the code. Finding titles should be neutral, such as "Track x".

And as a developer I like the "X should [ not ] Y" pattern, because the expected fix is clear.
With "Avoid X" pattern sometimes it is not clear whether the code is avoiding X and it should not or the opposite, that is the code is using X and it should avoid it.

"type": "CODE_SMELL",
"status": "ready",
"remediation": {
Expand All @@ -9,8 +9,14 @@
"tags": [
"performance",
"spring",
"eco-design",
"sustainability",
"creedengo"
],
"defaultSeverity": "Minor"
"defaultSeverity": "Minor",
"code": {
"impacts": {
"RELIABILITY": "LOW"
},
"attribute": "EFFICIENT"
}
}
15 changes: 13 additions & 2 deletions src/main/rules/GCI1/java/GCI1.asciidoc
Original file line number Diff line number Diff line change
@@ -1,10 +1,16 @@
:!sectids:

== Why is this an issue?

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.

== Non compliant Code Example
== How to fix it
=== Noncompliant code example

[source,java]
----
// Noncompliant
private final List<Integer> ids = Arrays.asList(1, 2, 3, 4, 5, 6, 7, 8, 9, 10);

List<Employee> employees = new ArrayList<>();
Expand All @@ -19,6 +25,7 @@ for (Integer id: ids) {

[source,java]
----
// Noncompliant
List<Employee> employees = new ArrayList<>();
Stream stream = Stream.of(1, 2, 3, 4, 5, 6, 7, 8, 9, 10);
stream.forEach(id -> {
Expand All @@ -29,7 +36,7 @@ stream.forEach(id -> {
});
----

== Compliant Solution
=== Compliant solution

[source,java]
----
Expand All @@ -42,3 +49,7 @@ List<Employee> employees = employeeRepository.findAllById(ids);
private final List<Integer> ids = Stream.of(1, 2, 3, 4, 5, 6, 7, 8, 9, 10).toList();
List<Employee> employees = employeeRepository.findAllById(ids);
----

== Resources

Reference/Validation is unknown.
15 changes: 11 additions & 4 deletions src/main/rules/GCI10/GCI10.json
Original file line number Diff line number Diff line change
@@ -1,14 +1,21 @@
{
"title": "Avoid using unoptimized vector images",
"title": "Unoptimized vector images should be avoided",
"type": "CODE_SMELL",
"status": "ready",
"remediation": {
"func": "Constant\/Issue",
"constantCost": "60min"
},
"tags": [
"eco-design",
"creedengo"
"sustainability",
"creedengo",
"performance"
],
"defaultSeverity": "Minor"
"defaultSeverity": "Minor",
"code": {
"impacts": {
"RELIABILITY": "LOW"
},
"attribute": "EFFICIENT"
}
}
14 changes: 12 additions & 2 deletions src/main/rules/GCI10/python/GCI10.asciidoc
Original file line number Diff line number Diff line change
@@ -1,9 +1,15 @@
:!sectids:

== Why is this an issue?

SVG images generated by common drawing softwares contains unnecessary data: calc layer, metadata, namespaces and comments.

== Non compliant Code Example
== How to fix it
=== Noncompliant code example

[source,xml]
----
<!-- Noncompliant -->
<!-- Created with Inkscape (http://www.inkscape.org/) -->
<svg
width="210mm"
Expand Down Expand Up @@ -31,11 +37,15 @@ SVG images generated by common drawing softwares contains unnecessary data: calc
</svg>
----

== Compliant Solution
=== Compliant solution

[source,xml]
----
<svg xmlns="http://www.w3.org/2000/svg" viewBox="0 0 210 297" width="210mm" height="297mm">
<circle cx="104.02724" cy="152.19028" r="73.177132" style="fill:#ff00ff;stroke-width:0.264583"/>
</svg>
----

== Resources

- https://github.com/cnumr/best-practices/blob/main/chapters/BP_036_fr.md[CNumR best practices (3rd edition) BP_036]
16 changes: 8 additions & 8 deletions src/main/rules/GCI11/GCI11.json
Original file line number Diff line number Diff line change
@@ -1,23 +1,23 @@
{
"title": "Disallow multiple access of same DOM element.",
"title": "DOM manipulation should be limited",
"type": "CODE_SMELL",
"code": {
"impacts": {
"MAINTAINABILITY": "MEDIUM"
},
"attribute": "EFFICIENT"
},
"status": "ready",
"remediation": {
"func": "Constant\/Issue",
"constantCost": "5min"
},
"tags": [
"creedengo",
"eco-design",
"sustainability",
"performance"
],
"defaultSeverity": "Major",
"code": {
"impacts": {
"RELIABILITY": "MEDIUM"
},
"attribute": "EFFICIENT"
},
"compatibleLanguages": [
"JAVASCRIPT",
"TYPESCRIPT"
Expand Down
9 changes: 7 additions & 2 deletions src/main/rules/GCI11/javascript/GCI11.asciidoc
Original file line number Diff line number Diff line change
Expand Up @@ -10,14 +10,19 @@ Assigning the DOM object to a variable not only improves performance but also en
It makes the code more concise and self-explanatory.
Developers reading the code can understand that the variable holds a reference to a specific DOM element, and its subsequent use is likely for multiple operations.

== How to fix it
=== Noncompliant code example

Here's an example in JavaScript to illustrate this rule:

[source,js,data-diff-id="2",data-diff-type="noncompliant"]
----
const width = document.getElementById('block').clientWidth;
const height = document.getElementById('block').clientHeight; // Non-compliant
const height = document.getElementById('block').clientHeight; // Noncompliant
----

=== Compliant solution

[source,js,data-diff-id="1",data-diff-type="noncompliant"]
----
const blockElement = document.getElementById('block'); // Compliant
Expand All @@ -32,5 +37,5 @@ In the second example, the DOM element reference is cached in the `blockElement`

=== Documentation

- https://github.com/cnumr/best-practices/blob/main/chapters/BP_054_en.md[CNUMR best practices] - Reduce DOM access via JavaScript
- https://github.com/cnumr/best-practices/blob/main/chapters/BP_054_en.md[CNumR best practices] - Reduce DOM access via JavaScript
- https://developer.mozilla.org/en-US/docs/Learn/Performance/JavaScript#tips_for_writing_more_efficient_code[Mozilla Web Technology for Developers] - Tips for writing more efficient code
16 changes: 8 additions & 8 deletions src/main/rules/GCI12/GCI12.json
Original file line number Diff line number Diff line change
@@ -1,23 +1,23 @@
{
"title": "Disallow multiple style changes at once.",
"title": "Multiple style changes should be batched",
"type": "CODE_SMELL",
"code": {
"impacts": {
"MAINTAINABILITY": "MEDIUM"
},
"attribute": "EFFICIENT"
},
"status": "ready",
"remediation": {
"func": "Constant\/Issue",
"constantCost": "10min"
},
"tags": [
"creedengo",
"eco-design",
"sustainability",
"performance"
],
"defaultSeverity": "Major",
"code": {
"impacts": {
"RELIABILITY": "MEDIUM"
},
"attribute": "EFFICIENT"
},
"compatibleLanguages": [
"JAVASCRIPT",
"TYPESCRIPT"
Expand Down
11 changes: 8 additions & 3 deletions src/main/rules/GCI12/javascript/GCI12.asciidoc
Original file line number Diff line number Diff line change
Expand Up @@ -10,17 +10,22 @@ Making multiple CSS changes in a single batch can trigger multiple reflows and r
Reflows and repaints are resource-intensive operations that can lead to performance issues.
Applying changes individually minimizes the number of reflows and repaints, improving overall page performance.

== How to fix it
=== Noncompliant code example

Here's an example in JavaScript and CSS to illustrate this rule:

[source,html,data-diff-id="3",data-diff-type="noncompliant"]
----
<script>
element.style.height = "800px";
element.style.width = "600px"; // Non-compliant
element.style.color = "red"; // Non-compliant
element.style.width = "600px"; // Noncompliant
element.style.color = "red"; // Noncompliant
</script>
----

=== Compliant solution

[source,html,data-diff-id="10",data-diff-type="compliant"]
----
<style>
Expand All @@ -42,4 +47,4 @@ In the first example, multiple CSS properties are set in a single batch, while i

=== Documentation

- https://github.com/cnumr/best-practices/blob/main/chapters/BP_045_en.md[CNUMR best practices] - Modify several CSS properties at once
- https://github.com/cnumr/best-practices/blob/main/chapters/BP_045_en.md[CNumR best practices] - Modify several CSS properties at once
Loading
Loading