From 6bf741235d3527c2b0a44937c996a15431450d57 Mon Sep 17 00:00:00 2001
From: Alban Auzeill Shared naming conventions allow teams to collaborate efficiently. This rule checks that all method names match a provided regular expression. With default provided regular expression Shared naming conventions allow teams to collaborate efficiently. This rule raises an issue when a method name does not match a provided regular expression. For example, with the default provided regular expression should be renamed to Overriding methods are excluded. Shared coding conventions allow teams to collaborate effectively. This rule allows to check that all class names match a provided regular
-expression. With default provided regular expression Shared naming conventions allow teams to collaborate efficiently. This rule raises an issue when a class name does not match a provided regular expression. For example, with the default provided regular expression should be renamed to Having to scroll horizontally makes it harder to get a quick overview and understanding of any piece of code. Scrolling horizontally to see a full line of code lowers the code readability. A source file that grows too much tends to aggregate too many responsibilities and inevitably becomes harder to understand and therefore to
-maintain. Above a specific threshold, it is strongly advised to refactor it into smaller pieces of code which focus on well defined tasks. Those
-smaller files will not only be easier to understand but also probably easier to test. A source file that grows too much tends to aggregate too many responsibilities and inevitably becomes harder to understand and, therefore, to
+maintain. Above a specific threshold, refactor the file into smaller files whose code focuses on well-defined tasks. Those smaller files will be easier to
+understand and easier to test. Developers should not need to configure the tab width of their text editors in order to be able to read source code. So the use of the tabulation character must be banned. The tab width can differ from one development environment to another. Using tabs may require other developers to configure their environment (text
+editor, preferences, etc.) to read source code. That is why using spaces is preferable. Most of the time a block of code is empty when a piece of code is really missing. So such empty block must be either filled or removed. An empty code block is confusing. It will require some effort from maintainers to determine if it is intentional or indicates the implementation is
+incomplete. Removing or filling the empty code blocks takes away ambiguity and generally results in a more straightforward and less surprising code. When a block contains a comment, this block is not considered to be empty unless it is a The rule ignores code blocks that contain comments unless they are Inheritance is certainly one of the most valuable concepts in object-oriented programming. It’s a way to compartmentalize and reuse code by
-creating collections of attributes and behaviors called classes which can be based on previously created classes. But abusing this concept by creating
-a deep inheritance tree can lead to very complex and unmaintainable source code. Most of the time too deep of an inheritance tree is due to bad object
-oriented design which leads to a systematic use of 'inheritance' when 'composition' would be better suited. This rule raises an issue when the inheritance tree, starting from For the parameter of the rule, the following rules are applied: Inheritance is one of the most valuable concepts in object-oriented programming. It’s a way to categorize and reuse code by creating collections of
+attributes and behaviors called classes, which can be based on previously created classes. But abusing this concept by creating a deep inheritance tree can lead to complex and unmaintainable source code. Often, an inheritance tree
+becoming too deep is the symptom of systematic use of "inheritance" when other approaches like "composition" would be better suited. This rule raises an issue when the inheritance tree, starting from The rule has one parameter to filter out classes of the count of inheritance. The following rules apply to define this parameter: Examples: Exceptions: The rule stops counting when it encounters a class from one of the following packages (or sub-packages): According to the official javadoc documentation, this Object.finalize() is called by the garbage collector on an object when garbage collection
-determines that there are no more references to the object. Calling this method explicitly breaks this contract and so is misleading. Before it reclaims storage from an object that is no longer referenced, the garbage collector calls This is a good time to release resources held by the object. Because the general contract is that the An explicit call to an object’s finalize method will perform operations that most likely were supposed to be performed only when the object was not
+referenced anymore by any thread. Since it is an acceptable practice to override the finalize method in any subclass of The Before it reclaims storage from an object that is no longer referenced, the garbage collector calls But there is no guarantee that this method will be called as soon as the last references to the object are removed. It can be few microseconds to few minutes later. For this reason relying on overriding the More unexpected issues can be caused by relying on the This rule is deprecated, and will eventually be removed. Overriding the Calling the Overriding or shadowing a variable declared in an outer scope can strongly impact the readability, and therefore the maintainability, of a piece of
code. Further, it could lead maintainers to introduce bugs because they think they’re using one variable but are really using another. Java adds an implicit public constructor to every class which does not define at least one explicitly. Hence, at least one non-public constructor
should be defined. An exception in a Superfluous exceptions within The rule will not raise any issue for exceptions that cannot be thrown from the method body: Also, the rule won’t raise issues on Also, the rule will not raise issues on Sometimes the developer will not have the time or will simply forget to get back to that tag. This rule is meant to track those tags and to ensure that they do not go unnoticed. Early classes of the Java API, such as It is better to use their new unsynchronized replacements:Why is this an issue?
-Noncompliant code example
-^[a-z][a-zA-Z0-9]*$
:^[a-z][a-zA-Z0-9]*$
, the method:
-public int DoSomething(){...}
+public int DoSomething(){...} // Noncompliant
-Compliant solution
+
public int doSomething(){...}
@@ -13,6 +13,6 @@ Exceptions
@Override
-public int Do_Something(){...}
+public int Do_Something(){...} // Compliant by exception
diff --git a/java-checks/src/main/resources/org/sonar/l10n/java/rules/java/S101.html b/java-checks/src/main/resources/org/sonar/l10n/java/rules/java/S101.html
index f772827d774..09f1ed1e20c 100644
--- a/java-checks/src/main/resources/org/sonar/l10n/java/rules/java/S101.html
+++ b/java-checks/src/main/resources/org/sonar/l10n/java/rules/java/S101.html
@@ -1,12 +1,11 @@
Why is this an issue?
-Noncompliant code example
-^[A-Z][a-zA-Z0-9]*$
:^[A-Z][a-zA-Z0-9]*$
, the class:
-class my_class {...}
+class my_class {...} // Noncompliant
-Compliant solution
+
class MyClass {...}
diff --git a/java-checks/src/main/resources/org/sonar/l10n/java/rules/java/S103.html b/java-checks/src/main/resources/org/sonar/l10n/java/rules/java/S103.html
index 10a9058c202..013131f8714 100644
--- a/java-checks/src/main/resources/org/sonar/l10n/java/rules/java/S103.html
+++ b/java-checks/src/main/resources/org/sonar/l10n/java/rules/java/S103.html
@@ -1,3 +1,3 @@
Why is this an issue?
-Why is this an issue?
-Why is this an issue?
-Why is this an issue?
-Noncompliant code example
+
-for (int i = 0; i < 42; i++){} // Empty on purpose or missing piece of code ?
+for (int i = 0; i < 42; i++){} // Noncompliant: is the block empty on purpose, or is code missing?
+Exceptions
-synchronized
block. synchronized
-blocks are still considered empty even with comments because they can still affect program flow.synchronized
blocks because these can affect program flow.Why is this an issue?
-Object
has a greater depth than is allowed.Object
, has a greater depth than is allowed.
?
matches a single character *
matches zero or more characters Why is this an issue?
-
-java.fwk.AbstractFwkClass
will stop count when AbstractFwkClassclass is reached. java.fwk.*
will stop count when any member of java.fwkPackage package is reached. java.fwk.**
same as above, but including sub-packages. java.fwk.AbstractFwkClass
: the count stops when AbstractFwkClass class is reached. java.fwk.*
: any member of java.fwkPackage package is reached. java.fwk.**
: same as above, but including sub-packages. Exceptions:
+android.**
Why is this an issue?
org.eclipse.**
org.springframework.**
Resources
+Documentation
+
diff --git a/java-checks/src/main/resources/org/sonar/l10n/java/rules/java/S1111.html b/java-checks/src/main/resources/org/sonar/l10n/java/rules/java/S1111.html
index 69833610279..2b321a321aa 100644
--- a/java-checks/src/main/resources/org/sonar/l10n/java/rules/java/S1111.html
+++ b/java-checks/src/main/resources/org/sonar/l10n/java/rules/java/S1111.html
@@ -1,6 +1,13 @@
Why is this an issue?
-finalize()
on the object.finalize
method should only be called once per object, calling this method explicitly is
+misleading and does not respect this contract.What is the potential impact?
+Object
, by invoking it explicitly, we will run
+code that was designed to only be ran at a different time.Noncompliant code example
public void dispose() throws Throwable {
@@ -9,6 +16,7 @@
Noncompliant code example
Resources
+
diff --git a/java-checks/src/main/resources/org/sonar/l10n/java/rules/java/S1111.json b/java-checks/src/main/resources/org/sonar/l10n/java/rules/java/S1111.json
index 8d1dd3651b3..fa96fbc03ba 100644
--- a/java-checks/src/main/resources/org/sonar/l10n/java/rules/java/S1111.json
+++ b/java-checks/src/main/resources/org/sonar/l10n/java/rules/java/S1111.json
@@ -1,5 +1,5 @@
{
- "title": "The Object.finalize() method should not be called",
+ "title": "The \"Object.finalize()\" method should not be called",
"type": "BUG",
"status": "ready",
"remediation": {
diff --git a/java-checks/src/main/resources/org/sonar/l10n/java/rules/java/S1113.html b/java-checks/src/main/resources/org/sonar/l10n/java/rules/java/S1113.html
index 81f76dbe18f..81347a39289 100644
--- a/java-checks/src/main/resources/org/sonar/l10n/java/rules/java/S1113.html
+++ b/java-checks/src/main/resources/org/sonar/l10n/java/rules/java/S1113.html
@@ -1,20 +1,31 @@
Why is this an issue?
-Object.finalize()
method is called on an object by the garbage collector when it determines that there are no more references to
-the object. But there is absolutely no warranty that this method will be called AS SOON AS the last references to the object are removed. It can be
-few microseconds to few minutes later. So when system resources need to be disposed by an object, it’s better to not rely on this asynchronous
-mechanism to dispose them.finalize()
on the object.finalize()
method to release resources or to update the state of the program is highly
+discouraged.What is the potential impact?
+finalize()
method to perform important operations on the application state:
+
Noncompliant code example
public class MyClass {
- ...
+
+ @Override
protected void finalize() {
releaseSomeResources(); // Noncompliant
}
- ...
+
}
Resources
+
diff --git a/java-checks/src/main/resources/org/sonar/l10n/java/rules/java/S1113.json b/java-checks/src/main/resources/org/sonar/l10n/java/rules/java/S1113.json
index 16cd7394ddd..9407e453a2b 100644
--- a/java-checks/src/main/resources/org/sonar/l10n/java/rules/java/S1113.json
+++ b/java-checks/src/main/resources/org/sonar/l10n/java/rules/java/S1113.json
@@ -1,5 +1,5 @@
{
- "title": "The Object.finalize() method should not be overridden",
+ "title": "The \"Object.finalize()\" method should not be overridden",
"type": "CODE_SMELL",
"status": "ready",
"remediation": {
diff --git a/java-checks/src/main/resources/org/sonar/l10n/java/rules/java/S1114.html b/java-checks/src/main/resources/org/sonar/l10n/java/rules/java/S1114.html
index fb6513fbe7d..4ca94be8155 100644
--- a/java-checks/src/main/resources/org/sonar/l10n/java/rules/java/S1114.html
+++ b/java-checks/src/main/resources/org/sonar/l10n/java/rules/java/S1114.html
@@ -1,3 +1,4 @@
+Why is this an issue?
Object.finalize()
method must be done with caution to dispose some system resources.super.finalize()
at the end of this method implementation is highly recommended in case parent implementations must also
diff --git a/java-checks/src/main/resources/org/sonar/l10n/java/rules/java/S1114.json b/java-checks/src/main/resources/org/sonar/l10n/java/rules/java/S1114.json
index a123ee68766..7ebbe7efd6f 100644
--- a/java-checks/src/main/resources/org/sonar/l10n/java/rules/java/S1114.json
+++ b/java-checks/src/main/resources/org/sonar/l10n/java/rules/java/S1114.json
@@ -1,15 +1,12 @@
{
"title": "\"super.finalize()\" should be called at the end of \"Object.finalize()\" implementations",
"type": "BUG",
- "status": "ready",
+ "status": "deprecated",
"remediation": {
"func": "Constant\/Issue",
"constantCost": "5min"
},
- "tags": [
- "cwe",
- "cert"
- ],
+ "tags": [],
"defaultSeverity": "Critical",
"ruleSpecification": "RSPEC-1114",
"sqKey": "S1114",
diff --git a/java-checks/src/main/resources/org/sonar/l10n/java/rules/java/S1117.html b/java-checks/src/main/resources/org/sonar/l10n/java/rules/java/S1117.html
index fe4b6a30992..5942c2b9bc0 100644
--- a/java-checks/src/main/resources/org/sonar/l10n/java/rules/java/S1117.html
+++ b/java-checks/src/main/resources/org/sonar/l10n/java/rules/java/S1117.html
@@ -1,18 +1,18 @@
Why is this an issue?
Noncompliant code example
class Foo {
public int myField;
public void doSomething() {
- int myField = 0;
+ int myField = 0; // Noncompliant
...
}
}
Resources
+Documentation
Why is this an issue?
Noncompliant code example
-
+
class StringUtils { // Noncompliant
public static String concatenate(String s1, String s2) {
@@ -14,7 +14,7 @@
Noncompliant code example
}
Compliant solution
-
+
class StringUtils { // Compliant
private StringUtils() {
diff --git a/java-checks/src/main/resources/org/sonar/l10n/java/rules/java/S1130.html b/java-checks/src/main/resources/org/sonar/l10n/java/rules/java/S1130.html
index fbb92b15ee6..44a8f6ae449 100644
--- a/java-checks/src/main/resources/org/sonar/l10n/java/rules/java/S1130.html
+++ b/java-checks/src/main/resources/org/sonar/l10n/java/rules/java/S1130.html
@@ -1,56 +1,75 @@
Why is this an issue?
-throws
declaration in Java is superfluous if it is:throws
clauses have negative effects on the readability and maintainability of the code. An exception in
+a throws
clause is superfluous if it is:
Noncompliant code example
-
+
void foo() throws MyException, MyException {} // Noncompliant; should be listed once
void bar() throws Throwable, Exception {} // Noncompliant; Exception is a subclass of Throwable
+void boo() throws IOException { // Noncompliant; IOException cannot be thrown
+ System.out.println("Hi!");
+}
Compliant solution
-
+
void foo() throws MyException {}
void bar() throws Throwable {}
+void boo() {
+ System.out.println("Hi!");
+}
Exceptions
-
-default
methods throw
, have empty bodies, or a single return statement. RuntimeException
, or one of its descendants, because explicating runtime exceptions which could
-be thrown can ultimately help the method’s users, and can even be considered as good practice.
-class A extends B {
+interface MyInterface {
+ default void defaultMethod() throws IOException {
+ System.out.println("Hi!");
+ }
+ void doSomething() throws IOException;
+}
+
+class A implements MyInterface {
@Override
void doSomething() throws IOException {
- compute(a);
+ System.out.println("Hi!");
}
- public void foo() throws IOException {}
-
- public void qix() throws MyRuntimeException {}
+ public void emptyBody() throws IOException {}
- protected void bar() throws IOException {
+ protected void singleThrowStatement() throws IOException {
throw new UnsupportedOperationException("This method should be implemented in subclasses");
}
- Object foobar(String s) throws IOException {
+ Object singleReturnStatement() throws IOException {
return null;
}
/**
* @throws IOException Overriding classes may throw this exception if they print values into a file
*/
- protected void print() throws IOException { // no issue, method is overridable and the exception has proper javadoc
+ protected void overridable() throws IOException { // no issue, method is overridable and the exception has proper javadoc
System.out.println("foo");
}
}
+RuntimeException
, or one of its sub-classes, because documenting runtime exceptions which
+could be thrown can ultimately help users of the method understand its behavior.
+class B {
+ int possibleDivisionByZero(int a, int b) throws ArithmeticException {
+ return a / b;
+ }
+}
+
diff --git a/java-checks/src/main/resources/org/sonar/l10n/java/rules/java/S1130.json b/java-checks/src/main/resources/org/sonar/l10n/java/rules/java/S1130.json
index c362bb7794f..ba606d60340 100644
--- a/java-checks/src/main/resources/org/sonar/l10n/java/rules/java/S1130.json
+++ b/java-checks/src/main/resources/org/sonar/l10n/java/rules/java/S1130.json
@@ -1,5 +1,5 @@
{
- "title": "\"throws\" declarations should not be superfluous",
+ "title": "Exceptions in \"throws\" clauses should not be superfluous",
"type": "CODE_SMELL",
"status": "ready",
"remediation": {
diff --git a/java-checks/src/main/resources/org/sonar/l10n/java/rules/java/S1134.html b/java-checks/src/main/resources/org/sonar/l10n/java/rules/java/S1134.html
index 42d9a98a2a7..cc8ebaf5fab 100644
--- a/java-checks/src/main/resources/org/sonar/l10n/java/rules/java/S1134.html
+++ b/java-checks/src/main/resources/org/sonar/l10n/java/rules/java/S1134.html
@@ -2,14 +2,14 @@ Why is this an issue?
FIXME
tags are commonly used to mark places where a bug is suspected, but which the developer wants to deal with later.Noncompliant code example
int divide(int numerator, int denominator) {
return numerator / denominator; // FIXME denominator value might be 0
}
Resources
+Documentation
-
diff --git a/java-checks/src/main/resources/org/sonar/l10n/java/rules/java/S1134.json b/java-checks/src/main/resources/org/sonar/l10n/java/rules/java/S1134.json
index 00aef3d01f5..2fb339349af 100644
--- a/java-checks/src/main/resources/org/sonar/l10n/java/rules/java/S1134.json
+++ b/java-checks/src/main/resources/org/sonar/l10n/java/rules/java/S1134.json
@@ -18,5 +18,5 @@
546
]
},
- "quickfix": "unknown"
+ "quickfix": "infeasible"
}
diff --git a/java-checks/src/main/resources/org/sonar/l10n/java/rules/java/S1149.html b/java-checks/src/main/resources/org/sonar/l10n/java/rules/java/S1149.html
index ec4c6b1eb66..666a7a39687 100644
--- a/java-checks/src/main/resources/org/sonar/l10n/java/rules/java/S1149.html
+++ b/java-checks/src/main/resources/org/sonar/l10n/java/rules/java/S1149.html
@@ -1,27 +1,27 @@
Why is this an issue?
Vector
, Hashtable
and StringBuffer
, were synchronized to make them
-thread-safe. Unfortunately, synchronization has a big negative impact on performance, even when using these collections from a single thread.
It is often best to use their non-synchronized counterparts:
ArrayList
or LinkedList
instead of Vector
Deque
instead of Stack
HashMap
instead of Hashtable
StringBuilder
instead of StringBuffer
Even when used in synchronized context, you should think twice before using it, since it’s usage can be tricky. If you are confident the usage is -legitimate, you can safely ignore this warning.
+Even when used in synchronized contexts, you should think twice before using their synchronized counterparts, since their usage can be costly. If +you are confident the usage is legitimate, you can safely ignore this warning.
-Vector cats = new Vector(); ++Vector<Cat> cats = new Vector<>();Compliant solution
--ArrayList cats = new ArrayList(); ++ArrayList<Cat> cats = new ArrayList<>();Exceptions
-Use of those synchronized classes is ignored in the signatures of overriding methods.
+Usage of these synchronized classes is ignored in the signatures of overriding methods.
@Override -public Vector getCats() {...} +public Vector getCats() {...} // Compliantdiff --git a/java-checks/src/main/resources/org/sonar/l10n/java/rules/java/S1149.json b/java-checks/src/main/resources/org/sonar/l10n/java/rules/java/S1149.json index 10a7b2a7007..4ddefceef61 100644 --- a/java-checks/src/main/resources/org/sonar/l10n/java/rules/java/S1149.json +++ b/java-checks/src/main/resources/org/sonar/l10n/java/rules/java/S1149.json @@ -1,5 +1,5 @@ { - "title": "Synchronized classes Vector, Hashtable, Stack and StringBuffer should not be used", + "title": "Synchronized classes \"Vector\", \"Hashtable\", \"Stack\" and \"StringBuffer\" should not be used", "type": "CODE_SMELL", "status": "ready", "remediation": { diff --git a/java-checks/src/main/resources/org/sonar/l10n/java/rules/java/S1150.html b/java-checks/src/main/resources/org/sonar/l10n/java/rules/java/S1150.html index 9c8e5fbaa86..7e496b67c61 100644 --- a/java-checks/src/main/resources/org/sonar/l10n/java/rules/java/S1150.html +++ b/java-checks/src/main/resources/org/sonar/l10n/java/rules/java/S1150.html @@ -1,19 +1,20 @@Why is this an issue?
-From the official Oracle Javadoc:
--+NOTE: The functionality of this Enumeration interface is duplicated by the Iterator interface. In addition, Iterator adds an optional remove - operation, and has shorter method names. New implementations should consider using Iterator in preference to Enumeration.
-As documented in
Enumeration
's Javadoc, you should favor theIterator
interface overEnumeration
. +Iterator
offers a similar contract toEnumeration
with the addition of a method for removal and shorter method names.Noncompliant code example
--public class MyClass implements Enumeration { // Non-Compliant ++public class MyClass implements Enumeration { // Noncompliant /* ... */ }Compliant solution
-+public class MyClass implements Iterator { // Compliant /* ... */ }+Resources
+
Appending String.valueOf()
to a String
decreases the code readability.
The argument passed to String.valueOf()
should be directly appended instead.
-public void display(int i){ - System.out.println("Output is " + String.valueOf(i)); // Noncompliant -} ++String message = "Output is " + String.valueOf(12);Compliant solution
--public void display(int i){ - System.out.println("Output is " + i); // Compliant -} ++String message = "Output is " + 12;diff --git a/java-checks/src/main/resources/org/sonar/l10n/java/rules/java/S1153.json b/java-checks/src/main/resources/org/sonar/l10n/java/rules/java/S1153.json index 2f74fa2d401..58a05e7ac91 100644 --- a/java-checks/src/main/resources/org/sonar/l10n/java/rules/java/S1153.json +++ b/java-checks/src/main/resources/org/sonar/l10n/java/rules/java/S1153.json @@ -1,5 +1,5 @@ { - "title": "String.valueOf() should not be appended to a String", + "title": "\"String.valueOf()\" should not be appended to a \"String\"", "type": "CODE_SMELL", "status": "ready", "remediation": { @@ -13,5 +13,5 @@ "ruleSpecification": "RSPEC-1153", "sqKey": "S1153", "scope": "All", - "quickfix": "unknown" + "quickfix": "targeted" } diff --git a/java-checks/src/main/resources/org/sonar/l10n/java/rules/java/S1157.html b/java-checks/src/main/resources/org/sonar/l10n/java/rules/java/S1157.html index 6f556039cae..38aaf2d38c6 100644 --- a/java-checks/src/main/resources/org/sonar/l10n/java/rules/java/S1157.html +++ b/java-checks/src/main/resources/org/sonar/l10n/java/rules/java/S1157.html @@ -2,19 +2,25 @@Why is this an issue?
Using
toLowerCase()
ortoUpperCase()
to make case insensitive comparisons is inefficient because it requires the creation of temporary, intermediateString
objects.Noncompliant code example
--boolean result1 = foo.toUpperCase().equals(bar); // Noncompliant -boolean result2 = foo.equals(bar.toUpperCase()); // Noncompliant -boolean result3 = foo.toLowerCase().equals(bar.LowerCase()); // Noncompliant ++private void compareStrings(String foo, String bar){ + boolean result1 = foo.toUpperCase().equals(bar); // Noncompliant + boolean result2 = foo.equals(bar.toUpperCase()); // Noncompliant + boolean result3 = foo.toLowerCase().equals(bar.toLowerCase()); // Noncompliant +}Compliant solution
--boolean result = foo.equalsIgnoreCase(bar); // Compliant ++private void compareStrings(String foo, String bar){ + boolean result1 = foo.equalsIgnoreCase(bar); // Compliant +}Exceptions
-No issue will be raised when a locale is specified because the result could be different from "equalsIgnoreCase". (e.g.: using the Turkish -locale)
+No issue will be raised when a locale is specified because the result could be different from
equalsIgnoreCase()
. (e.g.: using the +Turkish locale)-boolean result1 = foo.toUpperCase(locale).equals(bar); // Compliant +private void compareStrings(String foo, String bar, java.util.Locale locale){ + boolean result1 = foo.toUpperCase(locale).equals(bar); // Compliant +}diff --git a/java-checks/src/main/resources/org/sonar/l10n/java/rules/java/S1157.json b/java-checks/src/main/resources/org/sonar/l10n/java/rules/java/S1157.json index d03b12a40df..0c2a870abee 100644 --- a/java-checks/src/main/resources/org/sonar/l10n/java/rules/java/S1157.json +++ b/java-checks/src/main/resources/org/sonar/l10n/java/rules/java/S1157.json @@ -7,7 +7,8 @@ "constantCost": "5min" }, "tags": [ - "clumsy" + "clumsy", + "performance" ], "defaultSeverity": "Minor", "ruleSpecification": "RSPEC-1157", diff --git a/java-checks/src/main/resources/org/sonar/l10n/java/rules/java/S1158.html b/java-checks/src/main/resources/org/sonar/l10n/java/rules/java/S1158.html index e71183a256f..daf5cb85d91 100644 --- a/java-checks/src/main/resources/org/sonar/l10n/java/rules/java/S1158.html +++ b/java-checks/src/main/resources/org/sonar/l10n/java/rules/java/S1158.html @@ -1,15 +1,23 @@Why is this an issue?
-Creating temporary primitive wrapper objects only for
String
conversion or the use of thecompareTo
method is +Creating temporary primitive wrapper objects only for
-String
conversion or the use of thecompareTo()
method is inefficient.Instead, the static
+toString
orcompare
method of the primitive wrapper class should be used.Instead, the static
toString()
orcompare()
method of the primitive wrapper class should be used.Noncompliant code example
--new Integer(myInteger).toString(); // Noncompliant -Integer.valueOf(myInt).compareTo(0); // Noncompliant ++private int isZero(int value){ + return Integer.valueOf(value).compareTo(0); // Noncompliant +} +private String convert(int value){ + return Integer.valueOf(value).toString(); // Noncompliant +}Compliant solution
--Integer.toString(myInteger); // Compliant -Integer.compare(myInteger, 0); // Compliant ++private int isZero(int value){ + return Integer.compare(value, 0); // Compliant +} +private String convert(int value){ + return Integer.toString(value); // Compliant +}diff --git a/java-checks/src/main/resources/org/sonar/l10n/java/rules/java/S1161.html b/java-checks/src/main/resources/org/sonar/l10n/java/rules/java/S1161.html index d9823986bfe..009468b6f2d 100644 --- a/java-checks/src/main/resources/org/sonar/l10n/java/rules/java/S1161.html +++ b/java-checks/src/main/resources/org/sonar/l10n/java/rules/java/S1161.html @@ -1,28 +1,27 @@Why is this an issue?
-Using the
-@Override
annotation is useful for two reasons :
While not mandatory, using the @Override
annotation on compliant methods improves readability by making it explicit that methods are
+overriden.
A compliant method either overrides a parent method or implements an interface or abstract method.
+class ParentClass { - public boolean doSomething(){...} + public boolean doSomething(){/*...*/} } class FirstChildClass extends ParentClass { - public boolean doSomething(){...} // Noncompliant + public boolean doSomething(){/*...*/} // Noncompliant }Compliant solution
-+class ParentClass { - public boolean doSomething(){...} + public boolean doSomething(){/*...*/} } class FirstChildClass extends ParentClass { @Override - public boolean doSomething(){...} // Compliant + public boolean doSomething(){/*...*/} // Compliant }Exceptions
-This rule is relaxed when overriding a method from the
+Object
class liketoString()
,hashCode()
, …This rule does not raise issues when overriding methods from
diff --git a/java-checks/src/main/resources/org/sonar/l10n/java/rules/java/S1163.html b/java-checks/src/main/resources/org/sonar/l10n/java/rules/java/S1163.html index 86f7cd1589b..123b7493f7e 100644 --- a/java-checks/src/main/resources/org/sonar/l10n/java/rules/java/S1163.html +++ b/java-checks/src/main/resources/org/sonar/l10n/java/rules/java/S1163.html @@ -1,8 +1,8 @@Object
(eg:equals()
,hashCode()
, +toString()
, …).Why is this an issue?
-Throwing an exception from within a finally block will mask any exception which was previously thrown in the
-try
orcatch
-block, and the masked’s exception message and stack trace will be lost.Noncompliant code example
-+If an exception is already being thrown within the
+try
block or caught in acatch
block, throwing another exception in +thefinally
block will override the original exception. This means that the original exception’s message and stack trace will be lost, +potentially making it challenging to diagnose and troubleshoot the root cause of the problem.try { /* some work which end up throwing an exception */ throw new IllegalArgumentException(); @@ -11,8 +11,7 @@-Noncompliant code example
throw new RuntimeException(); // Noncompliant; masks the IllegalArgumentException }Compliant solution
-+try { /* some work which end up throwing an exception */ throw new IllegalArgumentException(); diff --git a/java-checks/src/main/resources/org/sonar/l10n/java/rules/java/S1163.json b/java-checks/src/main/resources/org/sonar/l10n/java/rules/java/S1163.json index b63aaea0083..21c893aafd6 100644 --- a/java-checks/src/main/resources/org/sonar/l10n/java/rules/java/S1163.json +++ b/java-checks/src/main/resources/org/sonar/l10n/java/rules/java/S1163.json @@ -15,7 +15,7 @@ "ruleSpecification": "RSPEC-1163", "sqKey": "S1163", "scope": "Main", - "quickfix": "unknown", + "quickfix": "infeasible", "securityStandards": { "CERT": [ "ERR05-J." diff --git a/java-checks/src/main/resources/org/sonar/l10n/java/rules/java/S1165.html b/java-checks/src/main/resources/org/sonar/l10n/java/rules/java/S1165.html index c750d2bd554..6e463ee3f21 100644 --- a/java-checks/src/main/resources/org/sonar/l10n/java/rules/java/S1165.html +++ b/java-checks/src/main/resources/org/sonar/l10n/java/rules/java/S1165.html @@ -1,16 +1,13 @@Why is this an issue?
-Exceptions are meant to represent the application’s state at the point at which an error occurred.
-Making all fields in an
-Exception
classfinal
ensures that this state:
Exception
is instantiated. This will enable developers to quickly understand what went wrong.
+When a class has all final
fields, the compiler ensures that the object’s state remains constant. It also enforces a clear design
+intent of immutability, making the class easier to reason about and use correctly.
Exceptions are meant to represent the application’s state at the point at which an error occurred. Making all fields in an Exception
+class final
ensures that these class fields do not change after initialization.
+public class MyException extends Exception { - private int status; // Noncompliant + private int status; // Noncompliant public MyException(String message) { super(message); @@ -27,10 +24,10 @@Noncompliant code example
}Compliant solution
-+public class MyException extends Exception { - private final int status; + private final int status; // Compliant public MyException(String message, int status) { super(message); @@ -43,4 +40,8 @@+Compliant solution
}Resources
+
Non-static initializers are rarely used, and can be confusing for most developers because they only run when new class instances are created. When -possible, non-static initializers should be refactored into standard constructors or field initializers.
-+diff --git a/java-checks/src/main/resources/org/sonar/l10n/java/rules/java/S1182.html b/java-checks/src/main/resources/org/sonar/l10n/java/rules/java/S1182.html index a04f9593827..9f5b95ff66d 100644 --- a/java-checks/src/main/resources/org/sonar/l10n/java/rules/java/S1182.html +++ b/java-checks/src/main/resources/org/sonar/l10n/java/rules/java/S1182.html @@ -1,6 +1,6 @@Non-static initializers, also known as instance initializers, are blocks of code within a class that is executed when an instance of the class is +created. They are executed when an object of the class is created just before the constructor is called. Non-static initializers are useful when you +want to perform some common initialization logic for all objects of a class. They allow you to initialize instance variables in a concise and +centralized manner, without having to repeat the same initialization code in each constructor.
+While non-static initializers may have some limited use cases, they are rarely used and can be confusing for most developers because they only run +when new class instances are created.
+How to fix it
+Non-static initializers should be refactored into standard constructors or field initializers when possible.
+In most cases, the use of constructors, overloaded constructors, or factory methods is preferable for initializing instance variables. These +approaches provide more explicit and controlled initialization, separate concerns, allow for better error handling, and make the code easier to +understand and maintain.
+Code examples
+Noncompliant code example
+class MyClass { private static final Map<String, String> MY_MAP = new HashMap<String, String>() { - - // Noncompliant - HashMap should be extended only to add behavior, not for initialization { put("a", "b"); } + }; // Noncompliant - HashMap should be extended only to add behavior, not for initialization +} ++Compliant solution
+Using static initialization block:
++class MyClass { + private static final Map<String, String> MY_MAP = new HashMap<>(); - }; + static { + MY_MAP.put("a", "b"); // Compliant + } }-Compliant solution
+or using constructor:
class MyClass { - private static final Map<String, String> MY_MAP = new HashMap<String, String>(); + private static final Map<String, String> MY_MAP = new HashMap<>(); - static { - MY_MAP.put("a", "b"); + public MyClass() { + MY_MAP.put("a", "b"); // Compliant } }or using Java 9
Map.of
:class MyClass { - // Compliant - private static final Map<String, String> MY_MAP = java.util.Map.of("a", "b"); + private static final Map<String, String> MY_MAP = java.util.Map.of("a", "b"); // Compliant }-or using Guava:
+or using Guava
ImmutableMap.of
:class MyClass { - // Compliant - private static final Map<String, String> MY_MAP = ImmutableMap.of("a", "b"); + private static final Map<String, String> MY_MAP = com.google.common.collect.ImmutableMap.of("a", "b"); // Compliant }+Resources
+Articles & blog posts
+
Cloneable
is the marker Interface
that indicates that clone()
may be called on an object. Overriding
-clone()
without implementing Cloneable
can be useful if you want to control how subclasses clone themselves, but otherwise,
+clone()
without implementing Cloneable
can be helpful if you want to control how subclasses clone themselves, but otherwise,
it’s probably a mistake.
The usual convention for Object.clone()
according to Oracle’s Javadoc is:
super.clone()
returns a new object instance super.clone()
returns an object of the same type as the one clone()
was called on Object.clone()
performs a shallow copy of the object’s state Object.clone()
performs a shallow copy of the object’s state. -class BaseClass { // Noncompliant; should implement Cloneable +How to fix it
+Ensure that the
+clone()
method callssuper.clone()
and implementCloneable
in the class or remove the clone +method.Code examples
+Noncompliant code example
++class BaseClass { // Noncompliant - should implement Cloneable @Override - public Object clone() throws CloneNotSupportedException { // Noncompliant; should return the super.clone() instance + public Object clone() throws CloneNotSupportedException { // Noncompliant - should return the super.clone() instance return new BaseClass(); } } @@ -38,8 +42,8 @@-Noncompliant code example
} }Compliant solution
-+Compliant solution
+class BaseClass implements Cloneable { @Override public Object clone() throws CloneNotSupportedException { // Compliant diff --git a/java-checks/src/main/resources/org/sonar/l10n/java/rules/java/S1190.html b/java-checks/src/main/resources/org/sonar/l10n/java/rules/java/S1190.html index 1c910712436..b28d78ec2b0 100644 --- a/java-checks/src/main/resources/org/sonar/l10n/java/rules/java/S1190.html +++ b/java-checks/src/main/resources/org/sonar/l10n/java/rules/java/S1190.html @@ -1,7 +1,8 @@Why is this an issue?
-Through Java’s evolution keywords have been added. While code that uses those words as identifiers may be compilable under older versions of Java, -it will not be under modern versions.
-Following keywords are marked as invalid identifiers
+Programming languages evolve over time, and new versions of Java introduce additional keywords. If future keywords are used in the current code, it +can create compatibility issues when transitioning to newer versions of Java. The code may fail to compile or behave unexpectedly due to conflicts +with newly introduced keywords.
+The following keywords are marked as invalid identifiers:
Keyword | -Added | +Added in version |
---|
assert
and strictfp
are another example of valid identifiers which became keywords in later versions, but are not
supported by this rule.
-public void doSomething() { - int enum = 42; // Noncompliant - String _ = ""; // Noncompliant +diff --git a/java-checks/src/main/resources/org/sonar/l10n/java/rules/java/S1191.html b/java-checks/src/main/resources/org/sonar/l10n/java/rules/java/S1191.html index 713e3f967db..263be47b6de 100644 --- a/java-checks/src/main/resources/org/sonar/l10n/java/rules/java/S1191.html +++ b/java-checks/src/main/resources/org/sonar/l10n/java/rules/java/S1191.html @@ -1,10 +1,19 @@How to fix it
+Rename the identifiers that use Java keywords.
+Code examples
+Noncompliant code example
++public class MyClass { + int enum = 42; // Noncompliant + String _ = ""; // Noncompliant }-Compliant solution
--public void doSomething() { - int magic = 42; +Compliant solution
++public class MyClass { + int magic = 42; // Noncompliant + String s = ""; // Noncompliant }+Resources
+Documentation
+
Classes in the sun.*
packages are considered implementation details, and are not part of the Java API.
They can cause problems when moving to new versions of Java because there is no backwards compatibility guarantee. Similarly, they can cause -problems when moving to a different Java vendor, such as OpenJDK.
-Such classes are almost always wrapped by Java API classes that should be used instead.
+The classes in the sun.*
packages are not part of the official Java API and are not intended for public use. They are internal
+implementation details specific to the Oracle JDK (Java Development Kit). Therefore, their availability, behavior, or compatibility is not guaranteed
+across different Java implementations or versions.
Since these classes are not part of the official Java API, they usually lack proper documentation and support. Finding comprehensive and up-to-date +information about their usage, functionality, and potential limitations can be challenging. This lack of documentation can make it difficult to +understand how to use these classes correctly.
+Classes in the sun.*
packages are often platform-dependent and can vary between different operating systems or Java Virtual Machine
+(JVM) implementations. Relying on these classes may lead to code that works on one platform but fails on others, limiting your code’s portability and
+cross-platform compatibility.
import sun.misc.BASE64Encoder; // Noncompliant+
Multiple catch blocks of the appropriate type should be used instead of catching a general exception, and then testing on the type.
-+A
+try-catch
block is used to handle exceptions or errors that may occur during the execution of a block of code. It allows you to +catch and handle exceptions gracefully, preventing your program from terminating abruptly.The code that may throw an exception is enclosed within the
+try
block, while eachcatch
block specifies the type of +exception it can handle. The corresponding catch block is executed if the exception matches the type specified in any catch block. It is unnecessary +to manually check the types usinginstanceof
because Java automatically matches the exception type to the appropriate catch block based +on the declared exception type in the catch clauses.How to fix it
+Replace
+if
statements that check the exception type usinginstaceof
with correspondingcatch
blocks.Code examples
+Noncompliant code example
+try { /* ... */ } catch (Exception e) { @@ -9,8 +17,8 @@-Noncompliant code example
if(e instanceof NullPointerException{ /* ... */ } // Noncompliant }Compliant solution
-+Compliant solution
+try { /* ... */ } catch (IOException e) { /* ... */ } // Compliant @@ -20,5 +28,6 @@Resources
According to the Java Language Specification:
--+For compatibility with older versions of the Java SE platform,
-the declaration of a method that returns an array is allowed to place (some or all of) the empty bracket pairs that form the declaration of the - array type after the formal parameter list.
-This obsolescent syntax should not be used in new code.
-
Placing the array designators []
after the type helps maintain backward compatibility with older versions of the Java SE platform.
+This syntax contributes to better readability as it becomes easier to distinguish between array types and non-array types. It helps convey the
+intention of the method to both the developer implementing it and the developer using it.
-public int getVector()[] { /* ... */ } // Noncompliant - -public int[] getMatrix()[] { /* ... */ } // Noncompliant ++public class Cube { + private int magicNumbers[] = { 42 }; // Noncompliant + public int getVector()[] { /* ... */ } // Noncompliant + public int[] getMatrix()[] { /* ... */ } // Noncompliant +}Compliant solution
--public int[] getVector() { /* ... */ } - -public int[][] getMatrix() { /* ... */ } ++public class Cube { + private int[] magicNumbers = { 42 }; // Compliant + public int[] getVector() { /* ... */ } // Compliant + public int[][] getMatrix() { /* ... */ } // Compliant +}+Documentation
+
"equals" as a method name should be used exclusively to override Object.equals(Object)
to prevent any confusion.
It is tempting to overload the method to take a specific class instead of Object
as parameter, to save the class comparison check.
-However, this will not work as expected when that is the only override.
+diff --git a/java-checks/src/main/resources/org/sonar/l10n/java/rules/java/S121.html b/java-checks/src/main/resources/org/sonar/l10n/java/rules/java/S121.html index 4b0f6a52775..abb12a50290 100644 --- a/java-checks/src/main/resources/org/sonar/l10n/java/rules/java/S121.html +++ b/java-checks/src/main/resources/org/sonar/l10n/java/rules/java/S121.html @@ -1,22 +1,26 @@ +In Java, the
+Object.equals()
method is used for object comparison, and it is typically overridden in classes to provide a custom +equality check based on your criteria for equality.The default implementation of
+equals()
provided by theObject
class compares the memory references of the two objects, +that means it checks if the objects are actually the same instance in memory.The "equals" as a method name should be used exclusively to override
+Object.equals(Object)
to prevent confusion.It is important to note that when you override
+equals()
, you should also override thehashCode()
method to maintain the +contract betweenequals()
andhashCode()
.How to fix it
+Either override
+Object.equals(Object)
or rename the method.Code examples
+Noncompliant code example
+class MyClass { private int foo = 1; @@ -17,27 +24,18 @@-Noncompliant code example
System.out.println(o1.equals(o2)); // Prints "false" because o2 an Object not a MyClass } } - -class MyClass2 { - public boolean equals(MyClass2 o) { // Ignored; `boolean equals(Object)` also present - //.. - } - - public boolean equals(Object o) { - //... - } -}Compliant solution
-+Compliant solution
+class MyClass { private int foo = 1; @Override - public boolean equals(Object o) { + public boolean equals(Object o) { // Compliant if (this == o) { return true; } + if (o == null || getClass() != o.getClass()) { return false; } @@ -45,18 +43,12 @@+Compliant solution
MyClass other = (MyClass)o; return this.foo == other.foo; } - - /* ... */ -} - -class MyClass2 { - public boolean equals(MyClass2 o) { - //.. - } - - public boolean equals(Object o) { - //... - } }Resources
+Documentation
+
Control structures are code statements that impact the program’s control flow (e.g., if statements, for loops, etc.)
While not technically incorrect, the omission of curly braces can be misleading, and may lead to the introduction of errors during maintenance.
-While not technically incorrect, the omission of curly braces can be misleading and may lead to the introduction of errors during maintenance.
+In the following example, the two calls seem to be attached to the if
statement, but only the first one is, and
+checkSomething
will always be executed:
if (condition) // Noncompliant executeSomething(); + checkSomething();-
Adding curly braces improves the code readability and its robustness:
if (condition) { executeSomething(); + checkSomething(); }+
The rule raises an issue when a control structure has no curly braces.
When the body of an if
statement is a single return
, break
or continue
and is on the same
-line.
The rule doesn’t raise an issue when the body of an if
statement is a single return
, break
, or
+continue
and is on the same line.
This rule raises an issue when an interface consists only of constant definitions without other members.
According to Joshua Bloch, author of "Effective Java":
---The constant interface pattern is a poor use of interfaces.
-That a class uses some constants internally is an implementation detail.
-Implementing a constant interface causes this implementation detail to leak into the class’s exported API. It is of no consequence to the users - of a class that the class implements a constant interface. In fact, it may even confuse them. Worse, it represents a commitment: if in a future - release the class is modified so that it no longer needs to use the constants, it still must implement the interface to ensure binary compatibility. - If a nonfinal class implements a constant interface,
-all of its subclasses will have their namespaces polluted by the constants in the interface.
-
This rule raises an issue when an interface consists solely of fields, without any other members.
--interface Status { // Noncompliant - int OPEN = 1; - int CLOSED = 2; +An interface that consists solely of constant definitions is a bad practice. The purpose of interfaces is to provide an API, not implementation +details. That is, they should provide functions in the first place and constants only to assist these functions, for example, as possible +arguments.
+If an interface contains constants only, move them either to somewhere else, or replace the interface with an Enum or a final class with a +private constructor.
+How to fix it
+If the concrete value of the constants is not essential, and they serve as mere identifiers, replace the interface with an
+enum
like +in the following example:+public interface Status { // Noncompliant, enum should be used + int OPEN = 1; + int CLOSED = 2; }-Compliant solution
--public enum Status { // Compliant ++public enum Status { // Compliant OPEN, - CLOSED; + CLOSED }-or
--public final class Status { // Compliant - public static final int OPEN = 1; - public static final int CLOSED = 2; +In some cases, enums are not a suitable option because the concrete constant value is important. Then you should check whether it is appropriate to +move them to a specific existing class, for example, if that class is the primary user of the constants:
++interface AuxiliaryConstants { // Noncompliant, implementation detail of WordPacker + int BITS_PER_WORD = 16; + int WORD_MASK = (1 << BITS_PER_WORD) - 1; + int HI_WORD_BK_MASK = ~(WORD_MASK << BITS_PER_WORD); +} + +class WordPacker { + public static int getHiWord(int value) { + return (value >>> AuxiliaryConstants.BITS_PER_WORD); + } + + public static int setHiWord(int value, int wordValue) { + return (value & AuxiliaryConstants.HI_WORD_BK_MASK) | + (wordValue << AuxiliaryConstants.BITS_PER_WORD); + } +} +++class WordPacker { // Compliant + private static final int BITS_PER_WORD = 16; + private static final int WORD_MASK = (1 << BITS_PER_WORD) - 1; + private static final int HI_WORD_BK_MASK = ~(WORD_MASK << BITS_PER_WORD); + + public static int getHiWord(int value) { + return (value >>> BITS_PER_WORD); + } + + public static int setHiWord(int value, int wordValue) { + return (value & HI_WORD_BK_MASK) | (wordValue << BITS_PER_WORD); + } +} ++If this is not the case and several classes are using the constants equally, you should use a final class with a private constructor. Unlike +interfaces, they can neither be inherited from nor instantiated.
++public interface ColorTheme { // Noncomplient, final class should be used + int COLOR_ERROR = 0xff0000; // red + int COLOR_WARNING = 0xffff00; // yellow + int COLOR_OK = 0x00cf00; // green +} +++public final class ColorTheme { // Compliant + public static final int COLOR_ERROR = 0xff0000; // red + public static final int COLOR_WARNING = 0xffff00; // yellow + public static final int COLOR_OK = 0x00cf00; // green + + private ColorTheme() {} }+Resources
+Articles & blog posts
+
This rule raises an issue when Thread.run()
is called instead of Thread.start()
.
The purpose of the Thread.run()
method is to execute code in a separate, dedicated thread. Calling this method directly doesn’t make
-sense because it causes its code to be executed in the current thread.
To get the expected behavior, call the Thread.start()
method instead.
++The likely intention of a user calling
+Thread.run()
is to start the execution of code within a new thread. This, however, is not what +happens when this method is called.The purpose of
+Thread.run()
is to provide a method that users can overwrite to specify the code to be executed. The actual thread is +then started by callingThread.start()
. WhenThread.run()
is called directly, it will be executed as a regular method within +the current thread.How to fix it
+If you intend to execute the contents of the
+Thread.run()
method within a new thread, callThread.start()
instead.If your intention is only to have a container for a method but execute this method within the current thread, do not use
+Thread
but +Runnable
or another functional interface.Code examples
+Noncompliant code example
+Thread myThread = new Thread(runnable); -myThread.run(); // Noncompliant +myThread.run(); // Noncompliant, does not start a thread-Compliant solution
-+Compliant solution
+Thread myThread = new Thread(runnable); myThread.start(); // Compliant+Noncompliant code example
++class ComputePrimesThread extends Thread { + @Override + public void run() { + // ... + } +} +new ComputePrimesThread().run(); // Noncompliant, does not start a thread ++Compliant solution
++class ComputePrimesThread extends Thread { + @Override + public void run() { + // ... + } +} +new ComputePrimesThread().start(); // Compliant ++Noncompliant code example
++class Button { + + private Thread onClick; + + Button(Thread onClick) { + this.onClick = onClick; + } + + private void clicked() { + if (onClick != null) onClick.run(); // Noncompliant, use functional interface + } +} + +new Button(new Thread() { + @Override public void run() { + System.out.println("clicked!"); + } +}); ++Compliant solution
++class Button { + + private Runnable onClick; + + Button(Runnable onClick) { + this.onClick = onClick; + } + + private void clicked() { + if (onClick != null) onClick.run(); // compliant + } +} + +new Button(() -> System.out.println("clicked!")); +Resources
+Documentation
+
For better readability, do not put more than one statement on a single line.
-Putting multiple statements on a single line lowers the code readability and makes debugging the code more complex.
-if(someCondition) doSomething(); +if (someCondition) doSomething(); // Noncompliant-
Write one statement per line to improve readability.
-if(someCondition) { +if (someCondition) { doSomething(); }diff --git a/java-checks/src/main/resources/org/sonar/l10n/java/rules/java/S1220.html b/java-checks/src/main/resources/org/sonar/l10n/java/rules/java/S1220.html index b718b8eb09a..32163dae33a 100644 --- a/java-checks/src/main/resources/org/sonar/l10n/java/rules/java/S1220.html +++ b/java-checks/src/main/resources/org/sonar/l10n/java/rules/java/S1220.html @@ -1,18 +1,34 @@
According to the Java Language Specification:
---Unnamed packages are provided by the Java platform principally for convenience when developing small or temporary applications or when just - beginning development.
-
To enforce this best practice, classes located in default package can no longer be accessed from named ones since Java 1.4.
--public class MyClass { /* ... */ } +Java packages serve two purposes:
++
+- Structure — Packages give a structure to the set of classes of your project. It is a bad practice to put all classes flat into the source + directory of a project without a package structure. A structure helps to mentally break down a project into smaller parts, simplifying readers' + understanding of how components are connected and how they interact.
+- Avoiding name clashes — a class part of the default package if no explicit package name is specified. This can easily cause name + collisions when other projects define a class of the same name.
+When no package is explicitly specified for the classes in your project, this makes the project harder to understand and may cause name collisions +with other projects. Also, classes located in the default package not be accessed from classes within named packages since Java 1.4.
+How to fix it
+Move your class to a package directory and explicitly state the package’s name at the top of the class. If your project does not have a package +structure, think of a structure that fits your needs. The package names should be unique to your project. You can find some best practices when +choosing package names in the Ressources section below.
+Code examples
+Noncompliant code example
++public class MyClass { /* ... */ } // Noncompliant, no package spacified-Compliant solution
--package org.example; +Compliant solution
++package org.example; // Compliant public class MyClass{ /* ... */ }+Resources
+Articles & blog posts
+
Naming a method tostring
, hashcode
or equal
is either:
toString
, Object.hashCode()
(note the camelCasing) or
- Object.equals
(note the 's' on the end) was meant, and the application does not behave as expected. In both cases, the method should be renamed.
-Due to the similar name with the methods Object.toString
, Object.hashCode
and Object.equals
, there is a
+significant likelihood that a developer intended to override one of these methods but made a spelling error.
Even if no such error exists and the naming was done on purpose, these method names can be misleading. Readers might not notice the difference, or +if they do, they may falsely assume that the developer made a mistake.
+If you intended to override one of the methods Object.toString
, Object.hashCode
, or Object.equals
, correct
+the spelling. Also, you should add the @Override
modifier, which causes a compiler error message in case the annotated method does not
+override anything.
If the naming was done on purpose, you should rename the methods to be more distinctive.
+-public int hashcode() { /* ... */ } // Noncompliant +public int hashcode() { /* ... */ } // Noncompliant -public String tostring() { /* ... */ } // Noncompliant +public String tostring() { /* ... */ } // Noncompliant public boolean equal(Object obj) { /* ... */ } // Noncompliant-
@Override -public int hashCode() { /* ... */ } +public int hashCode() { /* ... */ } // Compliant @Override -public String toString() { /* ... */ } +public String toString() { /* ... */ } // Compliant @Override -public boolean equals(Object obj) { /* ... */ } +public boolean equals(Object obj) { /* ... */ } // Compliantdiff --git a/java-checks/src/main/resources/org/sonar/l10n/java/rules/java/S1317.html b/java-checks/src/main/resources/org/sonar/l10n/java/rules/java/S1317.html index 3f75d6dab1c..6b1b54f6574 100644 --- a/java-checks/src/main/resources/org/sonar/l10n/java/rules/java/S1317.html +++ b/java-checks/src/main/resources/org/sonar/l10n/java/rules/java/S1317.html @@ -1,13 +1,42 @@ +
This rule raises an issue when the StringBuilder
or StringBuffer
constructor is called with a single character as an
+argument.
Instantiating a StringBuilder
or a StringBuffer
with a character is misleading because most Java developers would expect
-the character to be the initial value of the StringBuffer
.
What actually happens is that the int representation of the character is used to determine the initial size of the StringBuffer
.
-StringBuffer foo = new StringBuffer('x'); //equivalent to StringBuffer foo = new StringBuffer(120); ++When a developer uses the
+StringBuilder
orStringBuffer
constructor with a single character as an argument, the likely +intention is to create an instance with the character as the initial string value.However, this is not what happens because of the absence of a dedicated
+StringBuilder(char)
orStringBuffer(char)
+constructor. Instead,StringBuilder(int)
orStringBuffer(int)
is invoked, which results in an instance with the provided +int
value as the initial capacity of theStringBuilder
orStringBuffer
.The reason behind this behavior lies in the automatic widening of
+char
expressions toint
when required. Consequently, +the UTF-16 code point value of the character (for example,65
for the character'A'
) is interpreted as anint
+to specify the initial capacity.How to fix it
+If the argument is a
+char
literal, use a string literal instead:+StringBuffer foo = new StringBuffer('x'); // Noncompliant, replace with String-Compliant solution
--StringBuffer foo = new StringBuffer("x"); ++StringBuffer foo = new StringBuffer("x"); // Compliant+If the argument is it is a non-literal
+char
expression, convert it toString
using theString.valueOf()
+method:+StringBuffer foo(char firstChar) { + return new StringBuffer(firstChar); // Noncompliant +} +++StringBuffer foo(char firstChar) { + return new StringBuffer(String.valueOf(firstChar)); // Compliant +} ++Resources
+Documentation
+
The purpose of the Java Collections API is to provide a well defined hierarchy of interfaces in order to hide implementation details.
-Implementing classes must be used to instantiate new collections, but the result of an instantiation should ideally be stored in a variable whose -type is a Java Collection interface.
-This rule raises an issue when an implementation class:
+This rule raises an issue when a collection implementation class from java.util.*
is used:
public
method. public
method. public
member. public
method. public
method. public
field. +Why is this an issue?
+The Java Collections API offers a well-structured hierarchy of interfaces designed to hide collection implementation details. For the various +collection data structures like lists, sets, and maps, specific interfaces (
+java.util.List
,java.util.Set
, +java.util.Map
) cover the essential features.When passing collections as method parameters, return values, or when exposing fields, it is generally recommended to use these interfaces instead +of the implementing classes. The implementing classes, such as
+java.util.LinkedList
,java.util.ArrayList
, and +java.util.HasMap
, should only be used for collection instantiation. They provide finer control over the performance characteristics of +those structures, and developers choose them depending on their use case.For example, if fast random element access is essential,
+java.util.ArrayList
should be instantiated. If inserting elements at a random +position into a list is crucial, ajava.util.LinkedList
should be preferred. However, this is an implementation detail your API should +not expose.Code examples
+Noncompliant code example
+public class Employees { - private HashSet<Employee> employees = new HashSet<Employee>(); // Noncompliant - "employees" should have type "Set" rather than "HashSet" + public final HashSet<Employee> employees // Noncompliant, field type should be "Set" + = new HashSet<Employee>(); - public HashSet<Employee> getEmployees() { // Noncompliant + public HashSet<Employee> getEmployees() { // Noncompliant, return type should be "Set" return employees; } }-Compliant solution
-+Compliant solution
+public class Employees { - private Set<Employee> employees = new HashSet<Employee>(); // Compliant + public final Set<Employee> employees // Compliant + = new HashSet<Employee>(); - public Set<Employee> getEmployees() { // Compliant + public Set<Employee> getEmployees() { // Compliant return employees; } } diff --git a/java-checks/src/main/resources/org/sonar/l10n/java/rules/java/S1452.html b/java-checks/src/main/resources/org/sonar/l10n/java/rules/java/S1452.html index 0a9122d37a2..4fd44d9ad9e 100644 --- a/java-checks/src/main/resources/org/sonar/l10n/java/rules/java/S1452.html +++ b/java-checks/src/main/resources/org/sonar/l10n/java/rules/java/S1452.html @@ -1,20 +1,52 @@Why is this an issue?
-It is highly recommended not to use wildcard types as return types. Because the type inference rules are fairly complex it is -unlikely the user of that API will know how to use it correctly.
-Let’s take the example of method returning a "List<? extends Animal>". Is it possible on this list to add a Dog, a Cat, … we simply don’t -know. And neither does the compiler, which is why it will not allow such a direct use. The use of wildcard types should be limited to method -parameters.
-This rule raises an issue when a method returns a wildcard type.
-Noncompliant code example
--List<? extends Animal> getAnimals(){...} +A return type containing wildcards cannot be narrowed down in any context. This indicates that the developer’s intention was likely something +else.
+The core problem lies in type variance. Expressions at an input position, such as arguments passed to a method, can have a more specific type than +the type expected by the method, which is called covariance. Expressions at an output position, such as a variable that receives the return +result from a method, can have a more general type than the method’s return type, which is called contravariance. This can be traced back to +the Liskov substitution principle.
+In Java, type parameters of a generic type are invariant by default due to their potential occurrence in both input and output positions at the +same time. A classic example of this is the methods
+T get()
(output position) andadd(T element)
(input position) in +interfacejava.util.List
. We could construct cases with invalid typing inList
ifT
were not invariant.Wildcards can be employed to achieve covariance or contravariance in situations where the type parameter appears in one position only:
+
<? extends Foo>
for covariance (input positions) <? super Foo>
for contravariance (output positions) However, covariance is ineffective for the return type of a method since it is not an input position. Making it contravariant also has no effect +since it is the receiver of the return value which must be contravariant (use-site variance in Java). Consequently, a return type containing wildcards +is generally a mistake.
+The solution to this problem depends on the original intention of the developer. Given the examples:
++List<? extends Animal> getAnimals() { ... } // Noncompliant, wildcard with no use +List<? super Plant> getLifeforms() { ... } // Noncompliant, wildcard with no use-
-List<Animal> getAnimals(){...} ++You can remove the wildcards to make the types invariant:
++List<Animal> getAnimals() { ... } // Compliant, using invariant type instead +List<Plant> getLifeforms() { ... } // Compliant, using invariant type instead-or
--List<Dog> getAnimals(){...} +Or replace them with a super- or subtypes (still invariant):
++List<Dog> getAnimals() { ... } // Compliant, using subtype instead +List<Lifeform> getLifeforms() { ... } // Compliant, using supertype instead+Resources
+Documentation
+
This rule raises an issue when the Collections.EMPTY_*
fields are used instead of the Collections.empty*()
methods.
Since the introduction of generics in Java 5, the use of generic types such as List<String>
is recommended over the use of raw
-ones such as List
. Assigning a raw type to a generic one is not type safe, and will generate a warning. The old EMPTY_...
-fields of the Collections
class return raw types, whereas the newer empty...()
methods return generic ones.
-List<String> collection1 = Collections.EMPTY_LIST; // Noncompliant -Map<String, String> collection2 = Collections.EMPTY_MAP; // Noncompliant -Set<String> collection3 = Collections.EMPTY_SET; // Noncompliant +Generic types (types with type parameters) have been introduced into Java with language version 1.5. If type parameters are specified for a class +or method, it is still possible to ignore them to keep backward compatibility with older code, which is called the raw type of the class or +interface.
+Using raw type expressions is highly discouraged because the compiler cannot perform static type checking on them. This means that the compiler +will not report typing errors about them at compile time, but a
+ClassCastException
will be thrown during runtime.In Java 1.5, generics were also added to the Java collections API, and the data structures in
+java.util
, such asList
, +Set
, orMap
, now feature type parameters.Collections.EMPTY_LIST
,Collections.EMPTY_SET
, and +Collections.EMPTY_MAP
are relics from before generics, and they return raw lists, sets, or maps, with the limitations mentioned +above.How to fix it
+Use:
+
Collections.emptyList()
instead of Collections.EMPTY_LIST
Collections.emptySet()
instead of Collections.EMPTY_SET
Collections.emptyMap()
instead of Collections.EMPTY_MAP
In addition, there are variants of Collections.empty*()
available also for other collection interfaces, such as
+Collections.emptyIterator()
, Collections.emptyNavigableMap()
, Collections.emptySortedSet()
.
+List<String> collection1 = Collections.EMPTY_LIST; // Noncompliant, raw List +Set<Float> collection2 = Collections.EMPTY_SET; // Noncompliant, raw Set +Map<Int, String> collection3 = Collections.EMPTY_MAP; // Noncompliant, raw Map-
-List<String> collection1 = Collections.emptyList(); -Map<String, String> collection2 = Collections.emptyMap(); -Set<String> collection3 = Collections.emptySet(); +Compliant solution
++List<String> collection1 = Collections.emptyList(); // Compliant, List<String> +Set<Float> collection2 = Collections.emptySet(); // Compliant, Set<Float> +Map<Int, String> collection3 = Collections.emptyMap(); // Compliant, Map<Int, String>+Resources
+Documentation
+
By convention, a Java class' physical location (source directories) and its logical representation (packages) should be kept in sync. Thus a Java
-file located at "src/org/bar/Foo.java"
should have a package of "org.bar"
.
Unfortunately, this convention is not enforced by Java compilers, and nothing prevents a developer from making the "Foo.java" class part of the -"com.apple" package, which could degrade the maintainability of both the class and its application.
-Similarly, source placed in a folder with dots in its name instead of having the equivalent folder structure will compile but cause problems at run
-time. For instance, code with a package declaration of org.foo.bar
that is placed in org/foo.bar
will compile, but the
-classloader will always search for the class into the folder based on package structure, and will consequently expect sources to be in
-org/foo/bar
folder. foo.bar
is therefore not a proper folder name for sources.
The purpose of Java packages is to give structure to your project. A structure helps to mentally break down a project into smaller parts, +simplifying readers' understanding of how components are connected and how they interact.
+By convention, the source files' directory structure should replicate the project’s package structure. This is for the following reasons:
+Similarly, a source directory should not have the character .
in its name, as this would make it impossible to match the directory to
+the package structure.
Either move the source file so that the relative file path within the source directory matches the package name, or change the package name so that +it matches the relative file path.
++// file: src/main/foo/Fubar.java +package com.foo.bar; + +class Fubar { +} ++
+// file: src/main/com/foo/bar/Fubar.java +package com.foo.bar; + +class Fubar { +} ++
+// file: src/main/foo/Fubar.java +package foo; + +class Fubar { +} ++
This rule raises an issue when a lambda expression uses block notation while expression notation could be used.
There are two ways to write lambdas that contain single statement, but one is definitely more compact and readable than the other.
-Note that this rule is automatically disabled when the project’s sonar.java.source
is lower than 8
.
-x -> {System.out.println(x+1);} -(a, b) -> { return a+b; } --
-x -> System.out.println(x+1) -(a, b) -> a+b //For return statement, the return keyword should also be dropped +The right-hand side of a lambda expression can be written in two ways:
++
+- Expression notation: the right-hand side is as an expression, such as in
+(a, b) → a + b
- Block notation: the right-hand side is a conventional function body with a code block and an optional return statement, such as in
+(a, b) + → {return a + b;}
By convention, expression notation is preferred over block notation. Block notation must be used when the function implementation requires more +than one statement. However, when the code block consists of only one statement (which may or may not be a
+return
statement), it can be +rewritten using expression notation.This convention exists because expression notation has a cleaner, more concise, functional programming style and is regarded as more readable.
+How to fix it
+
return
statement, replace the code block with the argument expression from the
+ return
statement. return
statement, replace the code block with that statement. +(a, b) -> { return a + b; } // Noncompliant, replace code block with expression ++
+(a, b) -> a + b // Compliant ++
+x -> {System.out.println(x+1);} // Noncompliant, replace code block with statement ++
+x -> System.out.println(x+1) // Compliantdiff --git a/java-checks/src/main/resources/org/sonar/l10n/java/rules/java/S1604.html b/java-checks/src/main/resources/org/sonar/l10n/java/rules/java/S1604.html index 0aa8f54e30a..fb99b9b290d 100644 --- a/java-checks/src/main/resources/org/sonar/l10n/java/rules/java/S1604.html +++ b/java-checks/src/main/resources/org/sonar/l10n/java/rules/java/S1604.html @@ -1,26 +1,40 @@
Before Java 8, the only way to partially support closures in Java was by using anonymous inner classes. But the syntax of anonymous classes may -seem unwieldy and unclear.
-With Java 8, most uses of anonymous inner classes should be replaced by lambdas to highly increase the readability of the source code.
-Note that this rule is automatically disabled when the project’s sonar.java.source
is lower than 8
.
-myCollection.stream().map(new Mapper<String,String>() { - public String map(String input) { +diff --git a/java-checks/src/main/resources/org/sonar/l10n/java/rules/java/S1610.html b/java-checks/src/main/resources/org/sonar/l10n/java/rules/java/S1610.html index 828fdbbd23a..e5b8e28f0b4 100644 --- a/java-checks/src/main/resources/org/sonar/l10n/java/rules/java/S1610.html +++ b/java-checks/src/main/resources/org/sonar/l10n/java/rules/java/S1610.html @@ -1,3 +1,4 @@ +Before Java 8, the only way to partially support closures in Java was by using anonymous inner classes. Java 8 introduced lambdas, which are +significantly more readable and should be used instead.
+This rule is automatically disabled when the project’s
+sonar.java.source
is lower than8
, as lambda expressions were +introduced in Java 8.Code examples
+Noncompliant code example
++myCollection.stream().map(new Function<String,String>() { // Noncompliant, use a lambda expression instead + @Override + public String apply(String input) { return new StringBuilder(input).reverse().toString(); } -}); - -Predicate<String> isEmpty = new Predicate<String> { - boolean test(String myString) { - return myString.isEmpty(); - } -} +}) + ...-Compliant solution
--myCollection.stream().map(input -> new StringBuilder(input).reverse().toString()); - -Predicate<String> isEmpty = myString -> myString.isEmpty(); +Compliant solution
++myCollection.stream() + .map(input -> new StringBuilder(input).reverse().toString()) // Compliant + ... ++Noncompliant code example
++Predicate<String> isEmpty = new Predicate<String>() { // Noncompliant, use a lambda expression instead + @Override + public boolean test(String myString) { + return myString.isEmpty(); + } +}; ++Compliant solution
++Predicate<String> isEmpty = myString -> myString.isEmpty(); // Compliant+Resources
+
This rule is deprecated, and will eventually be removed.
With Java 8’s "default method" feature, any abstract class without direct or inherited field should be converted into an interface. However, this change may not be appropriate in libraries or other applications where the class is intended to be used as an API.
diff --git a/java-checks/src/main/resources/org/sonar/l10n/java/rules/java/S1610.json b/java-checks/src/main/resources/org/sonar/l10n/java/rules/java/S1610.json index ab64ca1b895..af386c01dfd 100644 --- a/java-checks/src/main/resources/org/sonar/l10n/java/rules/java/S1610.json +++ b/java-checks/src/main/resources/org/sonar/l10n/java/rules/java/S1610.json @@ -1,14 +1,12 @@ { "title": "Abstract classes without fields should be converted to interfaces", "type": "CODE_SMELL", - "status": "ready", + "status": "deprecated", "remediation": { "func": "Constant\/Issue", "constantCost": "10min" }, - "tags": [ - "java8" - ], + "tags": [], "defaultSeverity": "Minor", "ruleSpecification": "RSPEC-1610", "sqKey": "S1610", diff --git a/java-checks/src/main/resources/org/sonar/l10n/java/rules/java/S1611.html b/java-checks/src/main/resources/org/sonar/l10n/java/rules/java/S1611.html index 0e6af4a0006..158cafb5f49 100644 --- a/java-checks/src/main/resources/org/sonar/l10n/java/rules/java/S1611.html +++ b/java-checks/src/main/resources/org/sonar/l10n/java/rules/java/S1611.html @@ -1,13 +1,14 @@There are two possible syntaxes for a lambda having only one input parameter with an inferred type: with and without parentheses around that single -parameter. The simpler syntax, without parentheses, is more compact and readable than the one with parentheses, and is therefore preferred.
-Note that this rule is automatically disabled when the project’s sonar.java.source
is lower than 8
.
Lambda expressions with only one argument with an inferred type (i.e., no explicit type declaration) can be written without parentheses around that +single parameter. This syntax is simpler, more compact and readable than using parentheses and is therefore preferred.
+This rule is automatically disabled when the project’s sonar.java.source
is lower than 8
, as lambda expressions were
+introduced in Java 8.
+(x) -> x * 2Compliant solution
-+x -> x * 2diff --git a/java-checks/src/main/resources/org/sonar/l10n/java/rules/java/S1611.json b/java-checks/src/main/resources/org/sonar/l10n/java/rules/java/S1611.json index 028d3bee7a7..f5c6fbad415 100644 --- a/java-checks/src/main/resources/org/sonar/l10n/java/rules/java/S1611.json +++ b/java-checks/src/main/resources/org/sonar/l10n/java/rules/java/S1611.json @@ -1,5 +1,5 @@ { - "title": "Parentheses should be removed from a single lambda input parameter when its type is inferred", + "title": "Parentheses should be removed from a single lambda parameter when its type is inferred", "type": "CODE_SMELL", "status": "ready", "remediation": { diff --git a/java-checks/src/main/resources/org/sonar/l10n/java/rules/java/S1612.html b/java-checks/src/main/resources/org/sonar/l10n/java/rules/java/S1612.html index 0db3ee29b4d..3404e19485a 100644 --- a/java-checks/src/main/resources/org/sonar/l10n/java/rules/java/S1612.html +++ b/java-checks/src/main/resources/org/sonar/l10n/java/rules/java/S1612.html @@ -1,21 +1,27 @@Why is this an issue?
-Method/constructor references are commonly agreed to be more readable than lambdas in most situations, and are therefore preferred.
-However, method references are sometimes less concise than lambdas. In such cases, it might be fine to keep the lambda if it is better for -readability. This choice is ultimately up to the programmer. Therefore, this rule only raises issues on lambda functions that could be replaced by -shorter method references.
--
null
checks can be replaced with references to theObjects::isNull
andObjects::nonNull
methods, -casts
can be replaced withSomeClass.class::cast
andinstanceof
can be replaced with -SomeClass.class::isInstance
.Note that this rule is automatically disabled when the project’s
-sonar.java.source
is lower than8
.Noncompliant code example
-+Method or constructor references are more readable than lambda expressions in many situations, and may therefore be preferred.
+However, method references are sometimes less concise than lambdas. In such cases, it might be preferrable to keep the lambda expression for better +readability. Therefore, this rule only raises issues on lambda expressions where the replacement method reference is shorter.
+This rule is automatically disabled when the project’s
+sonar.java.source
is lower than8
, as lambda expressions were +introduced in Java 8.How to fix it
+Refer to the called method by its reference instead of wrapping it in a lambda expression.
+For instance:
+
null
checks can be replaced with references to Objects::isNull
and Objects::nonNull
SomeClass.class::cast
instanceof
can be replaced with SomeClass.class::isInstance
class A { void process(List<A> list) { list.stream() - .filter(a -> a instanceof B) - .map(a -> (B) a) - .map(b -> b.<String>getObject()) - .forEach(b -> { System.out.println(b); }); + .filter(myListValue -> myListValue instanceof B) // Noncompliant + .map(listValueToMap -> (B) listValueToMap) // Noncompliant + .map(bValueToMap -> bValueToMap.<String>getObject()) // Noncompliant + .forEach(o -> System.out.println(o)); // Noncompliant } } @@ -25,15 +31,15 @@-Noncompliant code example
} }
+diff --git a/java-checks/src/main/resources/org/sonar/l10n/java/rules/java/S1640.html b/java-checks/src/main/resources/org/sonar/l10n/java/rules/java/S1640.html index fa657247342..ba9059813c5 100644 --- a/java-checks/src/main/resources/org/sonar/l10n/java/rules/java/S1640.html +++ b/java-checks/src/main/resources/org/sonar/l10n/java/rules/java/S1640.html @@ -1,30 +1,25 @@Compliant solution
+class A { void process(List<A> list) { list.stream() - .filter(B.class::isInstance) - .map(B.class::cast) // Note: keeping the lambda would also be compliant here, since it is shorter - .map(B::<String>getObject) - .forEach(System.out::println); + .filter(B.class::isInstance) // Compliant + .map(B.class::cast) // Compliant + .map(B::<String>getObject) // Compliant + .forEach(System.out::println); // Compliant } } @@ -43,4 +49,8 @@+Compliant solution
} }Resources
+
When all the keys of a Map are values from the same enum, the Map
can be replaced with an EnumMap
, which can be much more
-efficient than other sets because the underlying data structure is a simple array.
If all the keys in a Map
are values from a single enum, it is recommended to use an EnumMap
as the specific
+implementation. An EnumMap
, which has the advantage of knowing all possible keys in advance, is more efficient compared to other
+implementations, as it can use a simple array as its underlying data structure.
-public class MyClass { - - public enum COLOR { - RED, GREEN, BLUE, ORANGE; - } - - public void mapMood() { - Map<COLOR, String> moodMap = new HashMap<COLOR, String> (); - } +diff --git a/java-checks/src/main/resources/org/sonar/l10n/java/rules/java/S1640.json b/java-checks/src/main/resources/org/sonar/l10n/java/rules/java/S1640.json index adbc0cd830e..7b3f07671c2 100644 --- a/java-checks/src/main/resources/org/sonar/l10n/java/rules/java/S1640.json +++ b/java-checks/src/main/resources/org/sonar/l10n/java/rules/java/S1640.json @@ -1,5 +1,5 @@ { - "title": "Maps with keys that are enum values should be replaced with EnumMap", + "title": "Maps with keys that are enum values should use the EnumMap implementation", "type": "CODE_SMELL", "status": "ready", "remediation": { diff --git a/java-checks/src/main/resources/org/sonar/l10n/java/rules/java/S1710.html b/java-checks/src/main/resources/org/sonar/l10n/java/rules/java/S1710.html index f7d87a8907b..3507d4b86ca 100644 --- a/java-checks/src/main/resources/org/sonar/l10n/java/rules/java/S1710.html +++ b/java-checks/src/main/resources/org/sonar/l10n/java/rules/java/S1710.html @@ -1,10 +1,12 @@+public enum Color { + RED, GREEN, BLUE, ORANGE; } + +Map<Color, String> colorMap = new HashMap<>(); // NoncompliantCompliant solution
--public class MyClass { - - public enum COLOR { - RED, GREEN, BLUE, ORANGE; - } - - public void mapMood() { - EnumMap<COLOR, String> moodMap = new EnumMap<> (COLOR.class); - } ++public enum Color { + RED, GREEN, BLUE, ORANGE; } + +Map<Color, String> colorMap = new EnumMap<>(Color.class); // Compliant+Resources
+
Before Java 8 if you needed to use multiple instances of the same annotation, they had to be wrapped in a container annotation. With Java 8, that’s -no longer necessary, allowing for cleaner, more readable code.
-Note that this rule is automatically disabled when the project’s sonar.java.source
is lower than 8
.
-@SomeAnnotations({ // Noncompliant +diff --git a/java-checks/src/main/resources/org/sonar/l10n/java/rules/java/S1844.html b/java-checks/src/main/resources/org/sonar/l10n/java/rules/java/S1844.html index 6f7884be272..858e101f7f1 100644 --- a/java-checks/src/main/resources/org/sonar/l10n/java/rules/java/S1844.html +++ b/java-checks/src/main/resources/org/sonar/l10n/java/rules/java/S1844.html @@ -1,25 +1,29 @@Before Java 8, a container annotation was required as wrapper to use multiple instances of the same annotation. As of Java 8, this is no longer +necessary. Instead, these annotations should be used directly without a wrapper, resulting in cleaner and more readable code.
+This rule is automatically disabled when the project’s
+sonar.java.source
is lower than8
as repeating annotations were +introduced is Java 8.Code examples
+Noncompliant code example
++@SomeAnnotations({ // Noncompliant, wrapper annotations are not necessary in Java 8+ @SomeAnnotation(..a..), @SomeAnnotation(..b..), @SomeAnnotation(..c..), @@ -13,8 +15,8 @@-Noncompliant code example
... }Compliant solution
-+Compliant solution
+@SomeAnnotation(..a..) @SomeAnnotation(..b..) @SomeAnnotation(..c..) @@ -22,4 +24,8 @@+Compliant solution
... }References
+
From the Java API documentation:
----
Condition
factors out theObject
monitor methods (wait
,notify
andnotifyAll
) - into distinct objects to give the effect of having multiple wait-sets per object, by combining them with the use of arbitrary Lock implementations. - Where aLock
replaces the use ofsynchronized
methods and statements, aCondition
replaces the use of the -Object
monitor methods.
The purpose of implementing the Condition
interface is to gain access to its more nuanced await
methods. Therefore,
-calling the method Object.wait(...)
on a class implementing the Condition
interface is silly and confusing.
The java.util.concurrent.locks.Condition
interface provides an alternative to the Object
monitor methods
+(wait
, notify
and notifyAll
). Hence, the purpose of implementing said interface is to gain access to its more
+nuanced await
methods.
Consequently, calling the method Object.wait
on a class implementing the Condition
interface is contradictory and should
+be avoided. Use Condition.await
instead.
-final Lock lock = new ReentrantLock(); -final Condition notFull = lock.newCondition(); -... -notFull.wait(); +diff --git a/java-checks/src/main/resources/org/sonar/l10n/java/rules/java/S1844.json b/java-checks/src/main/resources/org/sonar/l10n/java/rules/java/S1844.json index 0d1b3e749b3..df61be69207 100644 --- a/java-checks/src/main/resources/org/sonar/l10n/java/rules/java/S1844.json +++ b/java-checks/src/main/resources/org/sonar/l10n/java/rules/java/S1844.json @@ -1,5 +1,5 @@ { - "title": "\"Object.wait(...)\" should never be called on objects that implement \"java.util.concurrent.locks.Condition\"", + "title": "\"Object.wait\" should not be called on objects that implement \"java.util.concurrent.locks.Condition\"", "type": "CODE_SMELL", "status": "ready", "remediation": { diff --git a/java-checks/src/main/resources/org/sonar/l10n/java/rules/java/S1849.html b/java-checks/src/main/resources/org/sonar/l10n/java/rules/java/S1849.html index b2869aff9c1..33f5ce9cea3 100644 --- a/java-checks/src/main/resources/org/sonar/l10n/java/rules/java/S1849.html +++ b/java-checks/src/main/resources/org/sonar/l10n/java/rules/java/S1849.html @@ -1,19 +1,57 @@+void doSomething(Condition condition) { + condition.wait(); // Noncompliant, Object.wait is called + + ... +}Compliant solution
--final Lock lock = new ReentrantLock(); -final Condition notFull = lock.newCondition(); -... -notFull.await(); ++void doSomething(Condition condition) { + condition.await(); // Compliant, Condition.await is called + + ... +}+References
+
Calling Iterator.hasNext()
is not supposed to have any side effects, and therefore should not change the state of the iterator.
-Iterator.next()
advances the iterator by one item. So calling it inside Iterator.hasNext()
, breaks the
-hasNext()
contract, and will lead to unexpected behavior in production.
Calling Iterator.hasNext()
is not supposed to have any side effects and hence should not change the iterator’s state.
+Iterator.next()
advances the iterator by one item. So calling it inside Iterator.hasNext()
breaks the hasNext()
+contract and will lead to unexpected behavior in production.
How to fix this issue strongly depends on the specific implementation of the iterator. Make sure that the logic of the hasNext()
+implementation does not change the state of the iterator or any underlying data sources. Instead, it should merely return state information.
-public class FibonacciIterator implements Iterator<Integer>{ -... -@Override -public boolean hasNext() { - if(next() != null) { - return true; +diff --git a/java-checks/src/main/resources/org/sonar/l10n/java/rules/java/S1860.html b/java-checks/src/main/resources/org/sonar/l10n/java/rules/java/S1860.html index eab93a229dd..634d18d517c 100644 --- a/java-checks/src/main/resources/org/sonar/l10n/java/rules/java/S1860.html +++ b/java-checks/src/main/resources/org/sonar/l10n/java/rules/java/S1860.html @@ -1,22 +1,29 @@+class MyIterator implements Iterator<Integer> { + + private Queue<Integer> elements; + + ... + + @Override + public boolean hasNext() { + try { + next(); // Noncompliant, next() is called from hasNext() + return true; + } catch (NoSuchElementException e) { + return false; + } + } + + @Override + public Integer next() { + return elements.remove(); } - return false; } -... ++Compliant solution
++class MyIterator implements Iterator<Integer> { + + private Queue<Integer> elements; + + ... + + @Override + public boolean hasNext() { + return !elements.isEmpty(); // Compliant, no call to next() + } + + @Override + public Integer next() { + return elements.remove(); + } }+Resources
+
Objects which are pooled and potentially reused should not be used for synchronization. If they are, it can cause unrelated threads to deadlock
-with unhelpful stacktraces. Specifically, String
literals, and boxed primitives such as Integers should not be used as lock objects
-because they are pooled and reused. The story is even worse for Boolean
objects, because there could possibly be only two instances of
-Boolean
, Boolean.TRUE
and Boolean.FALSE
and every class that uses a Boolean will be referring to one of the
-two.
Here is the list of types which shouldn’t be used for synchronization:
+Instances of value-based classes, which are pooled and potentially reused, should not be used for synchronization. If they are, it can cause +unrelated threads to deadlock with unhelpful stacktraces.
+Within the JDK, types which should not be used for synchronization include:
String
literals java.lang
(such as Boolean
with Boolean.FALSE
and
+ Boolean.TRUE
) java.lang.Runtime.Version
Optional*
classes in java.util
: Optional
, OptionalInt
, OptionalLong
, and
+ OptionalDouble
java.time
API: Instant
, LocalDate
, LocalTime
,
+ LocalDateTime
, ZonedDateTime
, ZoneId
, OffsetTime
, OffsetDateTime
,
+ ZoneOffset
, Duration
, Period
, Year
, YearMonth
, and MonthDay
java.time.chrono
API: MinguoDate
, HijrahDate
, JapaneseDate
, and
+ ThaiBuddhistDate
java.lang.ProcessHandle
and its implementation classes java.util
: List.of
, List.copyOf
,
+ Set.of
, Set.copyOf
, Map.of
, Map.copyOf
, Map.ofEntries
, and Map.entry
.
+ Replace instances of value-based classes with a new object instance to synchronize on.
++private static final Boolean bLock = Boolean.FALSE; private static final Integer iLock = Integer.valueOf(0); private static final String sLock = "LOCK"; @@ -25,20 +32,20 @@Noncompliant code example
public void doSomething() { synchronized(bLock) { // Noncompliant - // ... + ... } synchronized(iLock) { // Noncompliant - // ... + ... } synchronized(sLock) { // Noncompliant - // ... + ... } synchronized(listLock) { // Noncompliant - // ... + ... }Compliant solution
-+private static final Object lock1 = new Object(); private static final Object lock2 = new Object(); private static final Object lock3 = new Object(); @@ -46,22 +53,24 @@Compliant solution
public void doSomething() { - synchronized(lock1) { - // ... + synchronized(lock1) { // Compliant + ... } - synchronized(lock2) { - // ... + synchronized(lock2) { // Compliant + ... } - synchronized(lock3) { - // ... + synchronized(lock3) { // Compliant + ... } - synchronized(lock4) { - // ... + synchronized(lock4) { // Compliant + ... }Resources
This rule raises an issue on a non-transient and non-serializable field within a serializable class, if said class does not have
+writeObject
and readObject
methods defined.
Fields in a Serializable
class must themselves be either Serializable
or transient
even if the class is
-never explicitly serialized or deserialized. For instance, under load, most J2EE application frameworks flush objects to disk, and an allegedly
-Serializable
object with non-transient, non-serializable data members could cause program crashes, and open the door to attackers. In
-general a Serializable
class is expected to fulfil its contract and not have an unexpected behaviour when an instance is serialized.
This rule raises an issue on non-Serializable
fields, and on collection fields when they are not private
(because they
-could be assigned non-Serializable
values externally), and when they are assigned non-Serializable
types within the
-class.
+By contract, fields in a
+Serializable
class must themselves be eitherSerializable
ortransient
. Even if the +class is never explicitly serialized or deserialized, it is not safe to assume that this cannot happen. For instance, under load, most J2EE +application frameworks flush objects to disk.An object that implements
+Serializable
but contains non-transient, non-serializable data members (and thus violates the contract) +could cause application crashes and open the door to attackers. In general, aSerializable
class is expected to fulfil its contract and +not exhibit unexpected behaviour when an instance is serialized.This rule raises an issue on:
+
Serializable
fields, private
(because they could be assigned non-Serializable
values externally),
+ Serializable
type within the class. Consider the following scenario.
+public class Address { - //... + ... } public class Person implements Serializable { private static final long serialVersionUID = 1905122041950251207L; private String name; - private Address address; // Noncompliant; Address isn't serializable + private Address address; // Noncompliant, Address is not serializable }-
+How to fix this issue depends on the application’s needs. If the field’s value should be preserved during serialization and deserialization, you +may want to make the field’s value serializable.
+public class Address implements Serializable { private static final long serialVersionUID = 2405172041950251807L; + + ... +} + +public class Person implements Serializable { + private static final long serialVersionUID = 1905122041950251207L; + + private String name; + private Address address; // Compliant, Address is serializable +} ++If the field’s value does not need to be preserved during serialization and deserialization, mark it as
+transient
. The field will be +ignored when the object is serialized. After deserialization, the field will be set to the default value corresponding to its type (e.g., +null
for object references).+public class Address { + ... } public class Person implements Serializable { private static final long serialVersionUID = 1905122041950251207L; private String name; - private Address address; + private transient Address address; // Compliant, the field is transient }-Exceptions
-The alternative to making all members
-serializable
ortransient
is to implement special methods which take on the -responsibility of properly serializing and de-serializing the object. This rule ignores classes which implement the following methods:- private void writeObject(java.io.ObjectOutputStream out) - throws IOException - private void readObject(java.io.ObjectInputStream in) - throws IOException, ClassNotFoundException; +The alternative to making all members serializable or
+transient
is to implement special methods which take on the responsibility of +properly serializing and de-serializing the objectwriteObject
andreadObject
. These methods can be used to properly +(de-)serialize an object, even though it contains fields that are not transient or serializable. Hence, this rule does not raise issues on fields of +classes which implement these methods.+public class Address { + ... +} + +public class Person implements Serializable { + private static final long serialVersionUID = 1905122041950251207L; + + private String name; + private Address address; // Compliant, writeObject and readObject handle this field + + private void writeObject(java.io.ObjectOutputStream out) throws IOException { + // Appropriate serialization logic here + } + + private void readObject(java.io.ObjectInputStream in) throws IOException, ClassNotFoundException { + // Appropriate deserialization logic here + } +}Resources
Even though the signatures for methods in a servlet include throws IOException, ServletException
, it’s a bad idea to let such
-exceptions be thrown. Failure to catch exceptions in a servlet could leave a system in a vulnerable state, possibly resulting in denial-of-service
-attacks, or the exposure of sensitive information because when a servlet throws an exception, the servlet container typically sends debugging
-information back to the user. And that information could be very valuable to an attacker.
This rule checks all exceptions in methods named "do*" are explicitly handled in servlet classes.
-Servlets are components in Java web development, responsible for processing HTTP requests and generating responses. In this context, exceptions are +used to handle and manage unexpected errors or exceptional conditions that may occur during the execution of a servlet.
+Catching exceptions within the servlet allows us to convert them into meaningful, user-friendly messages. Otherwise, failing to catch exceptions +will propagate them to the servlet container, where the default error-handling mechanism may impact the overall security and stability of the +server.
+Possible security problems are:
+Unfortunately, servlet method signatures do not force developers to handle IOException
and ServletException
:
-public void doGet(HttpServletRequest request, HttpServletResponse response) - throws IOException, ServletException { - String ip = request.getRemoteAddr(); - InetAddress addr = InetAddress.getByName(ip); // Noncompliant; getByName(String) throws UnknownHostException +public void doGet(HttpServletRequest request, HttpServletResponse response) throws IOException, ServletException { +} ++
To prevent this risk, this rule enforces all exceptions to be caught within the "do*" methods of servlet classes.
+Surround all method calls that may throw an exception with a try/catch
block.
In the following example, the getByName
method may throw an UnknownHostException
.
+public void doGet(HttpServletRequest request, HttpServletResponse response) throws IOException, ServletException { + InetAddress addr = InetAddress.getByName(request.getRemoteAddr()); // Noncompliant //... }-
-public void doGet(HttpServletRequest request, HttpServletResponse response) - throws IOException, ServletException { +Compliant solution
++public void doGet(HttpServletRequest request, HttpServletResponse response) throws IOException, ServletException { try { - String ip = request.getRemoteAddr(); - InetAddress addr = InetAddress.getByName(ip); + InetAddress addr = InetAddress.getByName(request.getRemoteAddr()); //... } - catch (UnknownHostException uhex) { + catch (UnknownHostException ex) { // Compliant //... } }Resources
+Articles & blog posts
When a Serializable
object has a non-serializable ancestor in its inheritance chain, object deserialization (re-instantiating the
-object from file) starts at the first non-serializable class, and proceeds down the chain, adding the properties of each subsequent child class, until
-the final object has been instantiated.
In order to create the non-serializable ancestor, its no-argument constructor is called. Therefore the non-serializable ancestor of a
-Serializable
class must have a no-arg constructor. Otherwise the class is Serializable
but not deserializable.
Java serialization is the conversion from objects to byte streams for storage or transmission. And later, java deserialization is the reverse +conversion, it reconstructs objects from byte streams.
+To make a java class serializable, this class should implement the java.io.Serializable
interface directly or through its
+inheritance.
-public class Fruit { - private Season ripe; +import java.io.Serializable; - public Fruit (Season ripe) {...} - public void setRipe(Season ripe) {...} - public Season getRipe() {...} +public class NonSerializableClass { } -public class Raspberry extends Fruit - implements Serializable { // Noncompliant; nonserializable ancestor doesn't have no-arg constructor - private static final long serialVersionUID = 1; - - private String variety; +public class SerializableClass implements Serializable { +} - public Raspberry(Season ripe, String variety) { ...} - public void setVariety(String variety) {...} - public String getVarity() {...} +public class OtherSerializableClass extends SerializableClass { + // is also serializable because it is a subtype of Serializable }-
Given a serializable class, it is important to note that not all its superclasses are serializable. Eventually, its superclasses stop implementing
+java.io.Serializable
. It could be at the end, once reaching the java.lang.Object
class, or before.
This is important because the serialization/deserialization runs through the class hierarchy of an object to decide which object fields to write or +read, and applies two different logics:
+So developers should pay particular attention to the non-serializable classes in the class hierarchy, because the presence of an implicit or +explicit no-argument constructor is required in those classes.
+This is an example of mandatory no-argument constructors in the hierarchy of SerializableClass
:
-public class Fruit { - private Season ripe; +public class NonSerializableClassWithoutConstructor { + // after deserialization, "field1" will always be set to 42 + private int field1 = 42; - public Fruit () {...}; // Compliant; no-arg constructor added to ancestor - public Fruit (Season ripe) {...} - public void setRipe(Season ripe) {...} - public Season getRipe() {...} + // this non-serializable class has an implicit no-argument constructor } -public class Raspberry extends Fruit - implements Serializable { - private static final long serialVersionUID = 1; +public class NonSerializableClass extends NonSerializableClassWithoutConstructor { + // after deserialization, "field2" will always be set to 12 by the no-argument constructor + private int field2; - private String variety; + // this non-serializable class has an explicit no-argument constructor + public NonSerializableClass() { + field2 = 12; + } + + public NonSerializableClass(int field2) { + this.field2 = field2; + } +} - public Raspberry(Season ripe, String variety) {...} - public void setVariety(String variety) {...} - public String getVarity() {...} +public class SerializableClass extends NonSerializableClass implements Serializable { + // after deserialization, "field3" will have the previously serialized value. + private int field3; + + // deserialization does not use declared constructors + public SerializableClass(int field3) { + super(field3 * 2); + this.field3 = field3; + } +} ++
Unfortunately, there is no compilation error when a class implements java.io.Serializable
and extends a non-serializable superclass
+without a no-argument constructor. This is an issue because, at runtime, deserialization will fail to find the required constructor.
For example, deserialization of an instance of the following SerializableClass
class, throws an InvalidClassException: no valid
+constructor
.
+public class NonSerializableClass { + private int field; + // this class can not be deserialized because it does not have any implicit or explicit no-argument constructor + public NonSerializableClass(int field) { + this.field = field; + } +} + +public class SerializableClass extends NonSerializableClass implements Serializable { +} ++
This rule checks in the hierarchy of serializable classes and reports an issue when a non-serializable superclass does not have the required +no-argument constructor which will produce a runtime error.
+There are two solutions to fix the missing no-argument constructor issue on non-serializable classes:
+Solution 1
If the fields of a non-serializable class need to be persisted, add the java.io.Serializable
interface to
+ the class implements
definition. Solution 2
Otherwise, add a no-argument constructor and initialize the fields with some valid default values. Example #1
++// Noncompliant; this Raspberry's ancestor doesn't have a no-argument constructor +// this rule raises an issue on the Raspberry class declaration +public class Fruit { + private Season pickingSeason; + public Fruit(Season pickingSeason) { + this.pickingSeason = pickingSeason; + } +} ++
+public class Raspberry extends Fruit implements Serializable { + private static final long serialVersionUID = 1; + private String variety; + public Raspberry(String variety) { + super(Season.SUMMER); + this.variety = variety; + } +} ++
Solution 1
+// Compliant; this Raspberry's ancestor is serializable +public class Fruit implements Serializable { + private static final long serialVersionUID = 1; + private Season pickingSeason; + public Fruit(Season pickingSeason) { + this.pickingSeason = pickingSeason; + } +} ++
Example #2
++public class Fruit { + // Noncompliant; this Raspberry's ancestor doesn't have a no-argument constructor + // this rule raises an issue on the Raspberry class declaration + public Fruit(String debugMessage) { + LOG.debug(debugMessage); + } +} ++
+public class Raspberry extends Fruit implements Serializable { + private static final long serialVersionUID = 1; + private String variety; + public Raspberry(String variety) { + super("From Raspberry constructor"); + this.variety = variety; + } +} ++
Solution 2
+public class Fruit { + // Compliant; this Raspberry ancestor has a no-argument constructor + public Fruit() { + this("From serialization"); + } + public Fruit(String debugMessage) { + LOG.debug(debugMessage); + } }+
An Externalizable
class is one which handles its own Serialization
and deserialization. During deserialization, the first
-step in the process is a default instantiation using the class' no-argument constructor. Therefore an Externalizable
class without a
-no-arg constructor cannot be deserialized.
-public class Tomato implements Externalizable { // Noncompliant; no no-arg constructor - - public Tomato (String color, int weight) { ... } +A class that implements
+java.io.Externalizable
is a class that provides a way to customize the serialization and deserialization, +allowing greater control over how the object’s state is written or read.The first step of the deserialization process is to call the class' no-argument constructor before the
+readExternal(ObjectInput in)
+method.An implicit default no-argument constructor exists on a class when no constructor is explicitly defined within the class. But this implicit +constructor does not exist when any constructor is explicitly defined, and in this case, we should always ensure that one of the constructors has +no-argument.
+It is an issue if the implicit or explicit no-argument constructor is missing or not public, because the deserialization will fail and throw an +
+InvalidClassException: no valid constructor.
.How to fix it
+This issue can be fixed by:
+
+public class Tomato implements Externalizable { + + public Color color; + + // Noncompliant; because of this constructor there is no implicit no-argument constructor, + // deserialization will fail + public Tomato(Color color) { + this.color = color; + } + + @Override + public void writeExternal(ObjectOutput out) throws IOException { + out.writeUTF(color.name()); + } + + @Override + public void readExternal(ObjectInput in) throws IOException { + color = Color.valueOf(in.readUTF()); + } }-
+diff --git a/java-checks/src/main/resources/org/sonar/l10n/java/rules/java/S2068.html b/java-checks/src/main/resources/org/sonar/l10n/java/rules/java/S2068.html index 4ba14cc8434..f48b0a99ece 100644 --- a/java-checks/src/main/resources/org/sonar/l10n/java/rules/java/S2068.html +++ b/java-checks/src/main/resources/org/sonar/l10n/java/rules/java/S2068.html @@ -27,14 +27,14 @@Compliant solution
+public class Tomato implements Externalizable { - public Tomato() { ... } - public Tomato (String color, int weight) { ... } + public Color color; + + // Compliant; deserialization will invoke this public no-argument constructor + public Tomato() { + this.color = Color.UNKNOWN; + } + + public Tomato(Color color) { + this.color = color; + } + + @Override + public void writeExternal(ObjectOutput out) throws IOException { + out.writeUTF(color.name()); + } + + @Override + public void readExternal(ObjectInput in) throws IOException { + color = Color.valueOf(in.readUTF()); + } }+Resources
+Documentation
+
String username = getEncryptedUser(); String password = getEncryptedPassword(); Connection conn = DriverManager.getConnection("jdbc:mysql://localhost/test?" + - "user=" + uname + "&password=" + password); + "user=" + username + "&password=" + password);
Because of floating point imprecision, you’re unlikely to get the value you expect from the BigDecimal(double)
constructor.
From the JavaDocs:
---The results of this constructor can be somewhat unpredictable. One might assume that writing new BigDecimal(0.1) in Java creates a BigDecimal - which is exactly equal to 0.1 (an unscaled value of 1, with a scale of 1), but it is actually equal to - 0.1000000000000000055511151231257827021181583404541015625. This is because 0.1 cannot be represented exactly as a double (or, for that matter, as a - binary fraction of any finite length). Thus, the value that is being passed in to the constructor is not exactly equal to 0.1, appearances - notwithstanding.
-
Instead, you should use BigDecimal.valueOf
, which uses a string under the covers to eliminate floating point rounding errors, or the
-constructor that takes a String
argument.
+The
+BigDecimal
is used to represents immutable, arbitrary-precision signed decimal numbers.Differently from the
+BigDecimal
, thedouble
primitive type and theDouble
type have limited precision due to +the use of double-precision 64-bit IEEE 754 floating point. Because of floating point imprecision, theBigDecimal(double)
constructor can +be somewhat unpredictable.For example writing
+new BigDecimal(0.1)
doesn’t create a BigDecimal which is exactly equal to 0.1, but it is equal to +0.1000000000000000055511151231257827021181583404541015625. This is because 0.1 cannot be represented exactly as a double (or, for that matter, as a +binary fraction of any finite length).How to fix it
+Use
+BigDecimal.valueOf
, which uses a string under the covers to eliminate floating point rounding errors, or the constructor that +takes aString
argument.Code examples
+Noncompliant code example
+double d = 1.1; -BigDecimal bd1 = new BigDecimal(d); // Noncompliant; see comment above -BigDecimal bd2 = new BigDecimal(1.1); // Noncompliant; same result +BigDecimal bd1 = new BigDecimal(d); // Noncompliant +BigDecimal bd2 = new BigDecimal(1.1); // Noncompliant-Compliant solution
-+Compliant solution
+double d = 1.1; -BigDecimal bd1 = BigDecimal.valueOf(d); -BigDecimal bd2 = new BigDecimal("1.1"); // using String constructor will result in precise value +BigDecimal bd1 = BigDecimal.valueOf(d); // Compliant +BigDecimal bd2 = new BigDecimal("1.1"); // CompliantResources
+Documentation
The equals
and hashCode
methods of java.net.URL
both may trigger a name service (usually DNS) lookup to
-resolve the host name or IP address. Depending on the configuration, and network status, that can take a long time. URI
on the other hand
-makes no such calls and should be used instead unless the specific URL
functionality is required.
In general it is better to use the URI
class until access to the resource is actually needed, at which point you can just convert the
+
The equals
and hashCode
methods of java.net.URL
may trigger a name service lookup (typically DNS) to resolve
+the hostname or IP address. Depending on the configuration, and network status, this lookup can be time-consuming.
On the other hand, the URI
class does not perform such lookups and is a better choice unless you specifically require the
+functionality provided by URL
.
In general, it is better to use the URI
class until access to the resource is actually needed, at which point you can convert the
URI
to a URL
using URI.toURL()
.
This rule checks for uses of URL
's in Map
and Set
, and for explicit calls to the equals
and
-hashCode
methods.
+diff --git a/java-checks/src/main/resources/org/sonar/l10n/java/rules/java/S2114.json b/java-checks/src/main/resources/org/sonar/l10n/java/rules/java/S2114.json index 6a89ec2950a..ebf28dd9b6b 100644 --- a/java-checks/src/main/resources/org/sonar/l10n/java/rules/java/S2114.json +++ b/java-checks/src/main/resources/org/sonar/l10n/java/rules/java/S2114.json @@ -11,5 +11,5 @@ "ruleSpecification": "RSPEC-2114", "sqKey": "S2114", "scope": "All", - "quickfix": "unknown" + "quickfix": "infeasible" } diff --git a/java-checks/src/main/resources/org/sonar/l10n/java/rules/java/S2116.html b/java-checks/src/main/resources/org/sonar/l10n/java/rules/java/S2116.html index fe09a2b4faf..0a8e6929451 100644 --- a/java-checks/src/main/resources/org/sonar/l10n/java/rules/java/S2116.html +++ b/java-checks/src/main/resources/org/sonar/l10n/java/rules/java/S2116.html @@ -1,19 +1,37 @@hashCode
methods. It suggests reconsidering the use ofURL
in such scenarios to avoid potential performance issues related +to name service lookups. +How to fix it
+Use the
+URI
class until access to the resource is actually needed.Code examples
+Noncompliant code example
+public void checkUrl(URL url) { - Set<URL> sites = new HashSet<URL>(); // Noncompliant + Set<URL> sites = new HashSet<URL>(); // Noncompliant URL homepage = new URL("http://sonarsource.com"); // Compliant - if (homepage.equals(url)) { // Noncompliant + if (homepage.equals(url)) { // Noncompliant // ... } }-Compliant solution
-+Compliant solution
+public void checkUrl(URL url) { - Set<URI> sites = new HashSet<URI>(); // Compliant + Set<URI> sites = new HashSet<URI>(); // Compliant URI homepage = new URI("http://sonarsource.com"); // Compliant URI uri = url.toURI(); - if (homepage.equals(uri)) { // Compliant + if (homepage.equals(uri)) { // Compliant // ... } }+Resources
+
While hashCode
and toString
are available on arrays, they are largely useless. hashCode
returns the array’s
-"identity hash code", and toString
returns nearly the same value. Neither method’s output actually reflects the array’s contents.
-Instead, you should pass the array to the relevant static Arrays
method.
-public static void main( String[] args ) -{ - String argStr = args.toString(); // Noncompliant - int argHash = args.hashCode(); // Noncompliant +The purpose of the
+hashCode
method is to return a hash code based on the contents of the object. Similarly, the purpose of the +toString
method is to provide a textual representation of the object’s contents.Calling
+hashCode()
andtoString()
directly on array instances should be avoided because the default implementations +provided by theObject
class do not provide meaningful results for arrays.hashCode()
returns the array’s "identity hash +code", andtoString()
returns nearly the same value. Neither method’s output reflects the array’s contents.How to fix it
+Use relevant static
+Arrays
method.
Arrays.hashCode
or Arrays.deepHashCode
Arrays.toString
or Arrays.deepToString
+public static void main(String[] args) { + String argStr = args.toString(); // Noncompliant + int argHash = args.hashCode(); // Noncompliant +}-
-public static void main( String[] args ) -{ - String argStr = Arrays.toString(args); - int argHash = Arrays.hashCode(args); ++Compliant solution
++public static void main(String[] args) { + String argStr = Arrays.toString(args); // Compliant + int argHash = Arrays.hashCode(args); // Compliant +}+Resources
+Documentation
+
Nothing in a non-serializable class will be written out to file, and attempting to serialize such a class will result in an exception being thrown.
-Only a class that implements Serializable
or one that extends such a class can successfully be serialized (or de-serialized).
-public class Vegetable { // neither implements Serializable nor extends a class that does - //... +diff --git a/java-checks/src/main/resources/org/sonar/l10n/java/rules/java/S2118.json b/java-checks/src/main/resources/org/sonar/l10n/java/rules/java/S2118.json index 37552c4ca1b..895eb796f81 100644 --- a/java-checks/src/main/resources/org/sonar/l10n/java/rules/java/S2118.json +++ b/java-checks/src/main/resources/org/sonar/l10n/java/rules/java/S2118.json @@ -1,5 +1,5 @@ { - "title": "Non-serializable classes should not be written", + "title": "\"writeObject\" argument must implement \"Serializable\"", "type": "BUG", "status": "ready", "remediation": { diff --git a/java-checks/src/main/resources/org/sonar/l10n/java/rules/java/S2119.html b/java-checks/src/main/resources/org/sonar/l10n/java/rules/java/S2119.html index 13188fa3d02..b560c15161f 100644 --- a/java-checks/src/main/resources/org/sonar/l10n/java/rules/java/S2119.html +++ b/java-checks/src/main/resources/org/sonar/l10n/java/rules/java/S2119.html @@ -1,30 +1,44 @@Serialization is a platform-independent mechanism for writing the state of an object into a byte-stream. For serializing the object, we call the +
+writeObject()
method ofjava.io.ObjectOutputStream
class. Only classes that implementSerializable
or extend a +class that does it can successfully be serialized (or de-serialized).Attempting to write a class with the
+writeObject
method of theObjectOutputStream
class that does not implement +Serializable
or extends a class that implements it, will throw anIOException
.How to fix it
+The object class passed as an argument to the
+writeObject
must implementSerializable
.Code examples
+Noncompliant code example
++public class Vegetable { + // ... } public class Menu { - public void meal() throws IOException { - Vegetable veg; - //... - FileOutputStream fout = new FileOutputStream(veg.getName()); - ObjectOutputStream oos = new ObjectOutputStream(fout); - oos.writeObject(veg); // Noncompliant. Nothing will be written + public void meal(ObjectOutputStream oos) throws IOException { + Vegetable veg = new Vegetable(); + oos.writeObject(veg); // Noncompliant } }-Compliant solution
--public class Vegetable implements Serializable { // can now be serialized - //... +Compliant solution
++public class Vegetable implements Serializable { + // ... } public class Menu { - public void meal() throws IOException { - Vegetable veg; - //... - FileOutputStream fout = new FileOutputStream(veg.getName()); - ObjectOutputStream oos = new ObjectOutputStream(fout); + public void meal(ObjectOutputStream oos) throws IOException { + Vegetable veg = new Vegetable(); oos.writeObject(veg); } }+Resources
+
Creating a new Random
object each time a random value is needed is inefficient and may produce numbers which are not random depending
-on the JDK. For better efficiency and randomness, create a single Random
, then store, and reuse it.
The Random()
constructor tries to set the seed with a distinct value every time. However there is no guarantee that the seed will be
-random or even uniformly distributed. Some JDK will use the current time as seed, which makes the generated numbers not random at all.
Creating a new Random
object each time a random value is needed is inefficient and may produce numbers that are not random, depending
+on the JDK. For better efficiency and randomness, create a single Random
, store it, and reuse it.
The Random()
constructor tries to set the seed with a distinct value every time. However, there is no guarantee that the seed will be
+randomly or uniformly distributed. Some JDK will use the current time as seed, making the generated numbers not random.
This rule finds cases where a new Random
is created each time a method is invoked.
-public void doSomethingCommon() { - Random rand = new Random(); // Noncompliant; new instance created with each invocation - int rValue = rand.nextInt(); - //... ++Exceptions
+This rule doesn’t apply to classes that use a
+Random
in their constructors or the staticmain
function and nowhere +else.How to fix it
+Define and reuse the
+Random
object.Code examples
+Noncompliant code example
++class MyClass { + + public void doSomethingCommon() { + Random random = new Random(); // Noncompliant - new instance created with each invocation + int rValue = random.nextInt(); + } +}-Compliant solution
--private Random rand = SecureRandom.getInstanceStrong(); // SecureRandom is preferred to Random +Compliant solution
++class MyClass { + private Random random = new Random(); // Compliant -public void doSomethingCommon() { - int rValue = this.rand.nextInt(); - //... + public void doSomethingCommon() { + int rValue = this.random.nextInt(); + } +}-Exceptions
-A class which uses a
Random
in its constructor or in a staticmain
function and nowhere else will be ignored by this -rule.Resources
+Documentation
+
Creating a substring from 0 to the end is silly. You’ll end up with the same string you started with. Using the value of String.length
-as either the start or end of a substring has similarly predictable results.
Calling String.contains
with the argument being identical to the String on which contains is invoked doesn’t make sense.
-String speech = "Now is the time for all good people to come to the aid of their country."; +Operations performed on a string with predictable outcomes should be avoided. For example:
+
Avoid performing the operation that has a predictable outcome.
++String speech = "SonarQube is the best static code analysis tool." -String s1 = speech.substring(0); // Noncompliant. Yields the whole string -String s2 = speech.substring(speech.length()); // Noncompliant. Yields ""; -String s3 = speech.substring(5,speech.length()); // Noncompliant. Use the 1-arg version instead +String s1 = speech.substring(0); // Noncompliant - yields the whole string +String s2 = speech.substring(speech.length()); // Noncompliant - yields ""; +String s3 = speech.substring(5, speech.length()); // Noncompliant - use the 1-arg version instead -if (speech.contains(speech)) { // Noncompliant - // always true +if (speech.contains(speech)) { // Noncompliant - always true + // ... }-
-String speech = "Now is the time for all good people to come to the aid of their country."; +diff --git a/java-checks/src/main/resources/org/sonar/l10n/java/rules/java/S2121.json b/java-checks/src/main/resources/org/sonar/l10n/java/rules/java/S2121.json index 58801a816f1..817b7c88181 100644 --- a/java-checks/src/main/resources/org/sonar/l10n/java/rules/java/S2121.json +++ b/java-checks/src/main/resources/org/sonar/l10n/java/rules/java/S2121.json @@ -1,5 +1,5 @@ { - "title": "Silly String operations should not be made", + "title": "String operations with predictable outcomes should be avoided", "type": "BUG", "status": "ready", "remediation": { diff --git a/java-checks/src/main/resources/org/sonar/l10n/java/rules/java/S2122.html b/java-checks/src/main/resources/org/sonar/l10n/java/rules/java/S2122.html index 05008afddd1..1d1f7ac7b80 100644 --- a/java-checks/src/main/resources/org/sonar/l10n/java/rules/java/S2122.html +++ b/java-checks/src/main/resources/org/sonar/l10n/java/rules/java/S2122.html @@ -1,16 +1,35 @@Compliant solution
++String speech = "SonarQube is the best static code analysis tool." String s1 = speech; String s2 = ""; String s3 = speech.substring(5); + +// ...+Resources
+
java.util.concurrent.ScheduledThreadPoolExecutor
's pool is sized with corePoolSize
, so setting corePoolSize
-to zero means the executor will have no threads and run nothing.
This rule detects instances where corePoolSize
is set to zero, via either its setter or the object constructor.
ThreadPoolExecutor
is an object that efficiently manages and controls the execution of multiple tasks in a thread pool. A thread pool
+is a collection of pre-initialized threads ready to execute tasks. Instead of creating a new thread for each task, which can be costly in terms of
+system resources, a thread pool reuses existing threads.
java.util.concurrent.ScheduledThreadPoolExecutor
is an extension of ThreadPoolExecutor
that can additionally schedule
+commands to run after a given delay or to execute periodically.
ScheduledThreadPoolExecutor
's pool is sized with corePoolSize
, so setting corePoolSize
to zero means the
+executor will have no threads and run nothing. corePoolSize
should have a value greater than zero and valid for your tasks.
This rule detects instances where corePoolSize
is set to zero via its setter or the object constructor.
++public void do(){ - ScheduledThreadPoolExecutor stpe1 = new ScheduledThreadPoolExecutor(0); // Noncompliant + int poolSize = 5; // value greater than 0 - ScheduledThreadPoolExecutor stpe2 = new ScheduledThreadPoolExecutor(POOL_SIZE); - stpe2.setCorePoolSize(0); // Noncompliant + ScheduledThreadPoolExecutor threadPool1 = new ScheduledThreadPoolExecutor(0); // Noncompliant - ... + ScheduledThreadPoolExecutor threadPool2 = new ScheduledThreadPoolExecutor(poolSize); + threadPool2.setCorePoolSize(0); // Noncompliant +}+Resources
+Documentation
+
Double.longBitsToDouble
expects a 64-bit, long
argument. Pass it a smaller value, such as an int
and the
-mathematical conversion into a double
simply won’t work as anticipated because the layout of the bits will be interpreted incorrectly, as
-if a child were trying to use an adult’s gloves.
Double.longBitsToDouble
converts the bit pattern into its corresponding floating-point representation. The method expects a 64-bit
+long argument to interpret the bits as a double value correctly.
When the argument is a smaller data type, the cast to long
may lead to a different value than expected due to the interpretation of
+the most significant bit, which, in turn, results in Double.longBitsToDouble
returning an incorrect value.
-int i = 42; -double d = Double.longBitsToDouble(i); // Noncompliant +++int i = 0x80003800; +Double.longBitsToDouble(i); // Noncompliant - NaN+Compliant solution
++long i = 0x80003800L; +Double.longBitsToDouble(i); // Compliant - 1.0610049784E-314 ++Resources
+Documentation
+
Constructors for String
, BigInteger
, BigDecimal
and the objects used to wrap primitives should never be
-used. Doing so is less clear and uses more memory than simply using the desired value in the case of strings, and using valueOf
for
-everything else.
Calling constructors for String
, BigInteger
, BigDecimal
and the objects used to wrap primitives is less
+efficient and less clear than relying on autoboxing or valueOf
.
Consider simplifying when possible for more efficient and cleaner code.
+String empty = new String(); // Noncompliant; yields essentially "", so just use that. String nonempty = new String("Hello world"); // Noncompliant Double myDouble = new Double(1.1); // Noncompliant; use valueOf @@ -12,19 +12,26 @@Noncompliant code example
BigInteger bigInteger1 = new BigInteger("3"); // Noncompliant BigInteger bigInteger2 = new BigInteger("9223372036854775807"); // Noncompliant BigInteger bigInteger3 = new BigInteger("111222333444555666777888999"); // Compliant, greater than Long.MAX_VALUE +BigDecimal bigDecimal = new BigDecimal("42.0"); // Compliant (see Exceptions section)Compliant solution
-+String empty = ""; String nonempty = "Hello world"; -Double myDouble = Double.valueOf(1.1); -Integer integer = Integer.valueOf(1); -Boolean bool = Boolean.valueOf(true); +Double myDouble = 1.1; +Integer integer = 1; +Boolean bool = true; BigInteger bigInteger1 = BigInteger.valueOf(3); BigInteger bigInteger2 = BigInteger.valueOf(9223372036854775807L); BigInteger bigInteger3 = new BigInteger("111222333444555666777888999"); +BigDecimal bigDecimal = new BigDecimal("42.0");Exceptions
-+
BigDecimal
constructor withdouble
argument is ignored as usingvalueOf
instead might change resulting -value. See {rule:java:S2111} .+
BigDecimal
constructor with adouble
argument is ignored as usingvalueOf
instead might change the resulting +value. See {rule:java:S2111}.Resources
+
Rather than creating a boxed primitive from a String
to extract the primitive value, use the relevant parse
method
-instead. It will be clearer and more efficient.
parse
makes the code more efficient and the intent of the developer clearer.
-String myNum = "12.2"; - -float f = (new Float(myNum)).floatValue(); // Noncompliant; creates & discards a Float ++String myNum = "42.0"; +float myFloat = new Float(myNum); // Noncompliant +float myFloatValue = (new Float(myNum)).floatValue(); // Noncompliant +int myInteger = Integer.valueOf(myNum); // Noncompliant +int myIntegerValue = Integer.valueOf(myNum).intValue(); // NoncompliantCompliant solution
--String myNum = "12.2"; - ++String myNum = "42.0"; float f = Float.parseFloat(myNum); +int myInteger = Integer.parseInt(myNum);diff --git a/java-checks/src/main/resources/org/sonar/l10n/java/rules/java/S2133.html b/java-checks/src/main/resources/org/sonar/l10n/java/rules/java/S2133.html index 7617d712fdd..3f10332abee 100644 --- a/java-checks/src/main/resources/org/sonar/l10n/java/rules/java/S2133.html +++ b/java-checks/src/main/resources/org/sonar/l10n/java/rules/java/S2133.html @@ -1,13 +1,13 @@Why is this an issue?
-Creating an object for the sole purpose of calling
getClass
on it is a waste of memory and cycles. Instead, simply use the class' +Creating an object for the sole purpose of calling
getClass
on it is a waste of memory and cycles. Instead, simply use the class’s.class
property.Noncompliant code example
-+MyObject myOb = new MyObject(); // Noncompliant Class c = myOb.getClass();Compliant solution
-+Class c = MyObject.class;diff --git a/java-checks/src/main/resources/org/sonar/l10n/java/rules/java/S2133.json b/java-checks/src/main/resources/org/sonar/l10n/java/rules/java/S2133.json index 27b73662744..308005c67cb 100644 --- a/java-checks/src/main/resources/org/sonar/l10n/java/rules/java/S2133.json +++ b/java-checks/src/main/resources/org/sonar/l10n/java/rules/java/S2133.json @@ -1,5 +1,5 @@ { - "title": "Objects should not be created only to \"getClass\"", + "title": "Objects should not be created only to invoke \"getClass\"", "type": "CODE_SMELL", "status": "ready", "remediation": { @@ -13,5 +13,5 @@ "ruleSpecification": "RSPEC-2133", "sqKey": "S2133", "scope": "All", - "quickfix": "unknown" + "quickfix": "targeted" } diff --git a/java-checks/src/main/resources/org/sonar/l10n/java/rules/java/S2134.html b/java-checks/src/main/resources/org/sonar/l10n/java/rules/java/S2134.html index 027caa97e92..b8e1b91d708 100644 --- a/java-checks/src/main/resources/org/sonar/l10n/java/rules/java/S2134.html +++ b/java-checks/src/main/resources/org/sonar/l10n/java/rules/java/S2134.html @@ -1,30 +1,50 @@Why is this an issue?
-According to the Java API documentation:
---There are two ways to create a new thread of execution. One is to declare a class to be a subclass of Thread. This subclass should override the - run method of class Thread. An instance of the subclass can then be allocated and started…
-The other way to create a thread is to declare a class that implements the Runnable interface. That class then implements the run method. An - instance of the class can then be allocated, passed as an argument when creating Thread, and started.
-By definition, extending the Thread class without overriding the
+run
method doesn’t make sense, and implies that the contract of the -Thread
class is not well understood.The default implementation of
+java.lang.Thread
'srun
will only perform a task passed as aRunnable
. If no +Runnable
has been provided at construction time, then the thread will not perform any action.When extending
java.lang.Thread
, you should override therun
method or pass aRunnable
target to the +constructor ofjava.lang.Thread
.Noncompliant code example
-public class MyRunner extends Thread { // Noncompliant; run method not overridden - - public void doSometing() {...} +public class MyThread extends Thread { // Noncompliant + public void doSomething() { + System.out.println("Hello, World!"); + } +} ++How to fix it
+To fix this issue, you have 2 options:
+
run
method +public class MyThread extends Thread { + @Override + public void run() { + System.out.println("Hello, World!"); + } }-
If run()
is not overridden in a class extending Thread
, it means that starting the thread will actually call
-Thread.run()
. However, Thread.run()
does nothing if it has not been fed with a target Runnable
. The rule
-consequently ignore classes extending Thread
if they are calling, in their constructors, the super(...)
constructor with a
-proper Runnable
target.
Runnable
at construction time -class MyThread extends Thread { // Compliant - calling super constructor with a Runnable - MyThread(Runnable target) { - super(target); // calling super constructor with a Runnable, which will be used for when Thread.run() is executed - // ... +public class MyRunnable implements Runnable { + @Override + public void run() { + System.out.println("Hello, World!"); + } +} +public class MyThread extends Thread { + public MyThread(Runnable runnable) { + super(runnable); + } +} + +public class Main() { + public static void main(String [] args) { + Runnable runnable = new MyRunnable(); + Thread customThread = new MyThread(runnable); + Thread regularThread = new Thread(runnable); } }diff --git a/java-checks/src/main/resources/org/sonar/l10n/java/rules/java/S2134.json b/java-checks/src/main/resources/org/sonar/l10n/java/rules/java/S2134.json index 5d43190420c..d4ef3f7df21 100644 --- a/java-checks/src/main/resources/org/sonar/l10n/java/rules/java/S2134.json +++ b/java-checks/src/main/resources/org/sonar/l10n/java/rules/java/S2134.json @@ -1,5 +1,5 @@ { - "title": "Classes extending java.lang.Thread should override the \"run\" method", + "title": "Classes extending java.lang.Thread should provide a specific \"run\" behavior", "type": "BUG", "status": "ready", "remediation": { diff --git a/java-checks/src/main/resources/org/sonar/l10n/java/rules/java/S2140.html b/java-checks/src/main/resources/org/sonar/l10n/java/rules/java/S2140.html index 6acc5369d3d..b60b403cfd7 100644 --- a/java-checks/src/main/resources/org/sonar/l10n/java/rules/java/S2140.html +++ b/java-checks/src/main/resources/org/sonar/l10n/java/rules/java/S2140.html @@ -1,17 +1,16 @@
There is no need to multiply the output of Random
's nextDouble
method to get a random integer. Use the
-nextInt
method instead.
This rule raises an issue when the return value of any of Random
's methods that return a floating point value is converted to an
-integer.
Generating random floating point values to cast them into integers is inefficient. A random bounded integer value can be generated with a single
+proper method call. Use nextInt
to make the code more efficient and the intent clearer.
+Random r = new Random(); -int rand = (int)r.nextDouble() * 50; // Noncompliant way to get a pseudo-random value between 0 and 50 -int rand2 = (int)r.nextFloat(); // Noncompliant; will always be 0; +int rand = (int) (r.nextDouble() * 50); // Noncompliant way to get a pseudo-random value between 0 and 50 +int rand2 = (int) r.nextFloat(); // Noncompliant; will always be 0;Compliant solution
-+Random r = new Random(); int rand = r.nextInt(50); // returns pseudo-random value between 0 and 50 +int rand2 = 0;diff --git a/java-checks/src/main/resources/org/sonar/l10n/java/rules/java/S2140.json b/java-checks/src/main/resources/org/sonar/l10n/java/rules/java/S2140.json index 4aac4a34250..564dd9fd630 100644 --- a/java-checks/src/main/resources/org/sonar/l10n/java/rules/java/S2140.json +++ b/java-checks/src/main/resources/org/sonar/l10n/java/rules/java/S2140.json @@ -7,7 +7,8 @@ "constantCost": "5min" }, "tags": [ - "clumsy" + "clumsy", + "performance" ], "defaultSeverity": "Minor", "ruleSpecification": "RSPEC-2140", diff --git a/java-checks/src/main/resources/org/sonar/l10n/java/rules/java/S2142.html b/java-checks/src/main/resources/org/sonar/l10n/java/rules/java/S2142.html index f3913c6f2fb..31074fcb10a 100644 --- a/java-checks/src/main/resources/org/sonar/l10n/java/rules/java/S2142.html +++ b/java-checks/src/main/resources/org/sonar/l10n/java/rules/java/S2142.html @@ -1,22 +1,17 @@Why is this an issue?
-
InterruptedExceptions
should never be ignored in the code, and simply logging the exception counts in this case as "ignoring". The -throwing of theInterruptedException
clears the interrupted state of the Thread, so if the exception is not handled properly the -information that the thread was interrupted will be lost. Instead,InterruptedExceptions
should either be rethrown - immediately or after -cleaning up the method’s state - or the thread should be re-interrupted by callingThread.interrupt()
even if this is supposed to be a -single-threaded application. Any other course of action risks delaying thread shutdown and loses the information that the thread was interrupted - +If an
+InterruptedException
or aThreadDeath
error is not handled properly, the information that the thread was +interrupted will be lost. Handling this exception means either to re-throw it or manually re-interrupt the current thread by calling +Thread.interrupt()
. Simply logging the exception is not sufficient and counts as ignoring it. Between the moment the exception is caught +and handled, is the right time to perform cleanup operations on the method’s state, if needed.What is the potential impact?
+Failing to interrupt the thread (or to re-throw) risks delaying the thread shutdown and losing the information that the thread was interrupted - probably without finishing its task.
-Similarly, the
-ThreadDeath
exception should also be propagated. According to its JavaDoc:-If
-ThreadDeath
is caught by a method, it is important that it be rethrown so that the thread actually dies.Noncompliant code example
public void run () { try { - while (true) { - // do stuff - } - }catch (InterruptedException e) { // Noncompliant; logging is not enough + /*...*/ + } catch (InterruptedException e) { // Noncompliant; logging is not enough LOGGER.log(Level.WARN, "Interrupted!", e); } } @@ -25,15 +20,23 @@Compliant solution
public void run () { try { - while (true) { - // do stuff - } - }catch (InterruptedException e) { + /* ... */ + } catch (InterruptedException e) { // Compliant; the interrupted state is restored LOGGER.log(Level.WARN, "Interrupted!", e); - // Restore interrupted state... + /* Clean up whatever needs to be handled before interrupting */ Thread.currentThread().interrupt(); } } + +public void run () { + try { + /* ... */ + } catch (ThreadDeath e) { // Compliant; the error is being re-thrown + LOGGER.log(Level.WARN, "Interrupted!", e); + /* Clean up whatever needs to be handled before re-throwing */ + throw e; + } +}Resources
Running finalizers on JVM exit is disabled by default. It can be enabled with System.runFinalizersOnExit
and
-Runtime.runFinalizersOnExit
, but both methods are deprecated because they are inherently unsafe.
According to the Oracle Javadoc:
---It may result in finalizers being called on live objects while other threads are concurrently manipulating those objects, resulting in erratic - behavior or deadlock.
-
If you really want to execute something when the virtual machine begins its shutdown sequence, you should attach a shutdown hook.
+Enabling runFinalizersOnExit
is unsafe as it might result in erratic behavior and deadlocks on application exit.
Indeed, finalizers might be force-called on live objects while other threads are concurrently manipulating them.
+Instead, if you want to execute something when the virtual machine begins its shutdown sequence, you should attach a shutdown hook.
+public static void main(String [] args) { - ... System.runFinalizersOnExit(true); // Noncompliant - ... } protected void finalize(){ - doSomething(); + doShutdownOperations(); }Compliant solution
-+public static void main(String [] args) { - Runtime.addShutdownHook(new Runnable() { - public void run(){ - doSomething(); - } - }); - //... + Thread myThread = new Thread( () -> { doShutdownOperations(); }); + Runtime.getRuntime().addShutdownHook(myThread); +}Resources
Boxing is the process of putting a primitive value into an analogous object, such as creating an Integer
to hold an int
-value. Unboxing is the process of retrieving the primitive value from such an object.
Since the original value is unchanged during boxing and unboxing, there’s no point in doing either when not needed. This also applies to autoboxing -and auto-unboxing (when Java implicitly handles the primitive/object transition for you).
+Boxing is the process of putting a primitive value into a wrapper object, such as creating an Integer
to hold an int
+value. Unboxing is the process of retrieving the primitive value from such an object. Since the original value is unchanged during boxing and
+unboxing, there is no point in doing either when not needed.
Instead, you should rely on Java’s implicit boxing/unboxing to convert from the primitive type to the wrapper type and vice versa, for better +readability.
-public void examineInt(int a) { ++public void examinePrimitiveInt(int a) { //... } -public void examineInteger(Integer a) { +public void examineBoxedInteger(Integer a) { // ... } public void func() { - int i = 0; - Integer iger1 = Integer.valueOf(0); + int primitiveInt = 0; + Integer boxedInt = Integer.valueOf(0); double d = 1.0; - int dIntValue = new Double(d).intValue(); // Noncompliant + int dIntValue = Double.valueOf(d).intValue(); // Noncompliant; should be replaced with a simple cast - examineInt(new Integer(i).intValue()); // Noncompliant; explicit box/unbox - examineInt(Integer.valueOf(i)); // Noncompliant; boxed int will be auto-unboxed + examinePrimitiveInt(boxedInt.intValue()); // Noncompliant; unnecessary unboxing + examinePrimitiveInt(Integer.valueOf(primitiveInt)); // Noncompliant; boxed int will be auto-unboxed - examineInteger(i); // Compliant; value is boxed but not then unboxed - examineInteger(iger1.intValue()); // Noncompliant; unboxed int will be autoboxed - - Integer iger2 = new Integer(iger1); // Noncompliant; unnecessary unboxing, value can be reused + examineBoxedInteger(Integer.valueOf(primitiveInt)); // Noncompliant; unnecessary boxing + examineBoxedInteger(boxedInt.intValue()); // Noncompliant; unboxed int will be autoboxed }Compliant solution
--public void examineInt(int a) { ++public void examinePrimitiveInt(int a) { //... } -public void examineInteger(Integer a) { +public void examineBoxedInteger(Integer a) { // ... } public void func() { - int i = 0; - Integer iger1 = Integer.valueOf(0); + int primitiveInt = 0; + Integer boxedInt = Integer.valueOf(0); double d = 1.0; int dIntValue = (int) d; - examineInt(i); + examinePrimitiveInt(primitiveInt); + examinePrimitiveInt(boxedInt); - examineInteger(i); - examineInteger(iger1); + examineBoxedInteger(primitiveInt); + examineBoxedInteger(boxedInt); }diff --git a/java-checks/src/main/resources/org/sonar/l10n/java/rules/java/S2153.json b/java-checks/src/main/resources/org/sonar/l10n/java/rules/java/S2153.json index 43c323826bb..055f052985c 100644 --- a/java-checks/src/main/resources/org/sonar/l10n/java/rules/java/S2153.json +++ b/java-checks/src/main/resources/org/sonar/l10n/java/rules/java/S2153.json @@ -1,5 +1,5 @@ { - "title": "Boxing and unboxing should not be immediately reversed", + "title": "Unnecessary boxing and unboxing should be avoided", "type": "BUG", "status": "ready", "remediation": { diff --git a/java-checks/src/main/resources/org/sonar/l10n/java/rules/java/S2157.html b/java-checks/src/main/resources/org/sonar/l10n/java/rules/java/S2157.html index 37a7fa94c98..9d5c4bb2e12 100644 --- a/java-checks/src/main/resources/org/sonar/l10n/java/rules/java/S2157.html +++ b/java-checks/src/main/resources/org/sonar/l10n/java/rules/java/S2157.html @@ -1,32 +1,85 @@ +This rule raises an issue when a class implements the interface
java.lang.Cloneable
, but does not override the +Object.clone()
method.Why is this an issue?
-Simply implementing
-Cloneable
without also overridingObject.clone()
does not necessarily make the class cloneable. While -theCloneable
interface does not include aclone
method, it is required by convention, and ensures true cloneability. -Otherwise the default JVMclone
will be used, which copies primitive values and object references from the source to the target. I.e. -without overridingclone
, any cloned instances will potentially share members with the source instance.Removing the
-Cloneable
implementation and providing a good copy constructor is another viable (some say preferable) way of allowing a -class to be copied.Noncompliant code example
--class Team implements Cloneable { // Noncompliant - private Person coach; - private List<Person> players; - public void addPlayer(Person p) {...} - public Person getCoach() {...} ++
Cloneable
is a marker interface that defines the contract of theObject.clone
method, which is to create a +consistent copy of the instance. Theclone
method is not defined by the interface though, but by classObjects
.The general problem with marker interfaces is that their definitions cannot be enforced by the compiler because they have no own API. When a class +implements
+Cloneable
but does not overrideObject.clone
, it is highly likely that it violates the contract for +Cloneable
.How to fix it
+Consider the following example:
++class Foo implements Cloneable { // Noncompliant, override `clone` method + public int value; }-Compliant solution
--class Team implements Cloneable { - private Person coach; - private List<Person> players; - public void addPlayer(Person p) { ... } - public Person getCoach() { ... } +Override the
+clone
method in classFoo
. By convention, it must callsuper.clone()
. At this point, we know +that:
Object.clone
will not throw a CloneNotSupportedException
, because Foo
implements
+ Cloneable
. Foo
We can narrow down the return type of clone
to Foo
and handle the CloneNotSupportedException
inside the
+function instead of throwing it:
+class Foo implements Cloneable { // Compliant + + public int value; + + @Override + public Foo clone() { + try { + return (Foo) super.clone(); + } catch (CloneNotSupportedException e) { + throw new AssertionError(); + } + } +} ++
Be aware that super.clone()
returns a one-by-one copy of the fields of the original instance. This means that in our example, the
+Foo.value
field is not required to be explicitly copied in the overridden function.
If you require another copy behavior for some or all of the fields, for example, deep copy or certain invariants that need to be true for a field,
+these fields must be patched after super.clone()
:
+class Entity implements Cloneable { + + public int id; // unique per instance + public List<Entity> children; // deep copy wanted @Override - public Object clone() { - Team clone = (Team) super.clone(); - //... + public Entity clone() { + try { + Entity copy = (Entity) super.clone(); + copy.id = System.identityHashCode(this); + copy.children = children.stream().map(Entity::clone).toList(); + return copy; + } catch (CloneNotSupportedException e) { + throw new AssertionError(); + } + } +} ++
Be aware that the Cloneable
/ Object.clone
approach has several drawbacks. You might, therefore, also consider resorting
+to other solutions, such as a custom copy
method or a copy constructor:
+class Entity implements Cloneable { + + public int id; // unique per instance + public List<Entity> children; // deep copy wanted + + Entity(Entity template) { + id = System.identityHashCode(this); + children = template.children.stream().map(Entity::new).toList(); } }+
This rule raises an issue when a subclass of a class that overrides Object.equals
introduces new fields but does not also override the
+Object.equals
method.
Extend a class that overrides equals
and add fields without overriding equals
in the subclass, and you run the risk of
-non-equivalent instances of your subclass being seen as equal, because only the superclass fields will be considered in the equality test.
This rule looks for classes that do all of the following:
-equals
. equals
. When a class overrides Object.equals
, this indicates that the class not just considers object identity as equal (the default
+implementation of Object.equals
) but implements another logic for what is considered equal in the context of this class. Usually (but not
+necessarily), the semantics of equals
in this case is that two objects are equal when their state is equal field by field.
Because of this, adding new fields to a subclass of a class that overrides Object.equals
but not updating the implementation of
+equals
in the subclass is most likely an error.
Consider the following example:
-public class Fruit { - private Season ripe; +class Foo { + + final int a; - public boolean equals(Object obj) { - if (obj == this) { - return true; - } - if (this.class != obj.class) { - return false; - } - Fruit fobj = (Fruit) obj; - if (ripe.equals(fobj.getRipe()) { - return true; - } - return false; + @Override + public boolean equals(Object other) { + if (other == null) return false; + if (getClass() != other.getClass()) return false; + return a == ((Foo) other).a; } } - -public class Raspberry extends Fruit { // Noncompliant; instances will use Fruit's equals method - private Color ripeColor; ++
+class Bar extends Foo { // Noncompliant, `equals` ignores the value of `b` + final int b; }-
-public class Fruit { - private Season ripe; +diff --git a/java-checks/src/main/resources/org/sonar/l10n/java/rules/java/S2160.json b/java-checks/src/main/resources/org/sonar/l10n/java/rules/java/S2160.json index 366dce6c991..23229d2691b 100644 --- a/java-checks/src/main/resources/org/sonar/l10n/java/rules/java/S2160.json +++ b/java-checks/src/main/resources/org/sonar/l10n/java/rules/java/S2160.json @@ -1,5 +1,5 @@ { - "title": "Subclasses that add fields should override \"equals\"", + "title": "Subclasses that add fields to classes that override \"equals\" should also override \"equals\"", "type": "CODE_SMELL", "status": "ready", "remediation": { diff --git a/java-checks/src/main/resources/org/sonar/l10n/java/rules/java/S2165.html b/java-checks/src/main/resources/org/sonar/l10n/java/rules/java/S2165.html index e0bc9b9efb3..fd2ee76ec89 100644 --- a/java-checks/src/main/resources/org/sonar/l10n/java/rules/java/S2165.html +++ b/java-checks/src/main/resources/org/sonar/l10n/java/rules/java/S2165.html @@ -1,15 +1,45 @@ +Override the
+equals
method in the subclass to incorporate the new fields into the comparison:+class Bar extends Foo { // Compliant, `equals` now also considers `b` + + final int b; - public boolean equals(Object obj) { - if (obj == this) { - return true; - } - if (this.class != obj.class) { - return false; - } - Fruit fobj = (Fruit) obj; - if (ripe.equals(fobj.getRipe()) { - return true; - } - return false; + @Override + public boolean equals(Object other) { + if (!super.equals(other)) return false; + return b == ((Bar) other).b; } } ++In case the new fields should not be part of the comparison because they are, for example, auxiliary variables not contributing to the object value +(), still override the method to make the point clear that this was not just forgotten:
++class Bar extends Foo { // Compliant, we do explicitly not want to take `b` into account -public class Raspberry extends Fruit { - private Color ripeColor; + final int b; - public boolean equals(Object obj) { - if (! super.equals(obj)) { - return false; - } - Raspberry fobj = (Raspberry) obj; - if (ripeColor.equals(fobj.getRipeColor()) { // added fields are tested - return true; - } - return false; + @Override + public boolean equals(Object other) { + return super.equals(other); } }+Resources
+Documentation
+
This rule raises an issue when a finalizer assign null
to fields of the instance it is called on.
There is no point in setting class fields to null
in a finalizer. If this this is a hint to the garbage collector, it is unnecessary -
-the object will be garbage collected anyway - and doing so may actually cause extra work for the garbage collector.
++In the Java object lifecycle, the
+finalize
method for an instance is called after the garbage collector has determined that the +instance can be removed from the object heap. Therefore, it is unnecessary to implement a finalizer to set instance fields explicitly to +null
to tell the garbage collector that the instance no longer needs them.In the worst case, implementing
+finalize
is even counterproductive because it might accidentally create new references from other +(living) objects on the heap to the collectible instance, thus, reviving it.Important note about finalizers:
+There are no guarantees when the Java Runtime will call the
+finalize
method or whether it will be called at all.Using finalizers is, therefore, a bad practice. They should never be used to free resources, such as closing streams, freeing locks, or freeing +native system resources. Consider other freeing mechanisms instead, such as an explicit
+close
,unlock
, orfree
+method in your class.How to fix it
+Remove assignments from your finalizer that assign
+null
to fields of the instance the finalizer is called on. When this leaves you +with an empty finalizer body, remove the finalizer.Code examples
+Noncompliant code example
+public class Foo { private String name; @Override void finalize() { - name = null; // Noncompliant; completely unnecessary + name = null; // Noncompliant, instance will be removed anyway } }+Compliant solution
++public class Foo { // Compliant + private String name; +} ++Resources
+Documentation
+
According to Oracle Javadoc:
----
IllegalMonitorStateException
is thrown when a thread has attempted to wait on an object’s monitor or to notify other threads waiting - on an object’s monitor without owning the specified monitor.
In other words, this exception can be thrown only in case of bad design because Object.wait(...)
, Object.notify()
and
-Object.notifyAll()
methods should never be called on an object whose monitor is not held.
The IllegalMonitorStateException
is an exception that occurs when a thread tries to perform an operation on an object’s monitor that
+it does not own. This exception is typically thrown when a method like wait()
, notify()
, or notifyAll()
is
+called outside a synchronized block or method.
IllegalMonitorStateException
is specifically designed to be an unchecked exception to point out a programming mistake. This exception
+serves as a reminder for developers to rectify their code by correctly acquiring and releasing locks using synchronized blocks or methods. It also
+emphasizes the importance of calling monitor-related methods on the appropriate objects to ensure proper synchronization.
Catching and handling this exception can mask underlying synchronization issues and lead to unpredictable behavior.
-public void doSomething(){ - ... +diff --git a/java-checks/src/main/resources/org/sonar/l10n/java/rules/java/S2235.json b/java-checks/src/main/resources/org/sonar/l10n/java/rules/java/S2235.json index 2d083d6e2d4..67bea0b4838 100644 --- a/java-checks/src/main/resources/org/sonar/l10n/java/rules/java/S2235.json +++ b/java-checks/src/main/resources/org/sonar/l10n/java/rules/java/S2235.json @@ -1,5 +1,5 @@ { - "title": "IllegalMonitorStateException should not be caught", + "title": "\"IllegalMonitorStateException\" should not be caught", "type": "CODE_SMELL", "status": "ready", "remediation": { diff --git a/java-checks/src/main/resources/org/sonar/l10n/java/rules/java/S2236.html b/java-checks/src/main/resources/org/sonar/l10n/java/rules/java/S2236.html index f63ac98959e..0cb8d02a38f 100644 --- a/java-checks/src/main/resources/org/sonar/l10n/java/rules/java/S2236.html +++ b/java-checks/src/main/resources/org/sonar/l10n/java/rules/java/S2236.html @@ -1,17 +1,20 @@+public void doSomething() { try { - ... anObject.notify(); - ... - } catch(IllegalMonitorStateException e) { - ... + } catch(IllegalMonitorStateException e) { // Noncompliant } }Compliant solution
--public void doSomething(){ - ... ++public void doSomething() { synchronized(anObject) { - ... anObject.notify(); - ... } }+Resources
+
The methods wait(...)
, notify()
and notifyAll()
are available on a Thread
instance, but only
-because all classes in Java extend Object
and therefore automatically inherit those methods. But there are two very good reasons for not
-calling them on a Thread
:
BLOCKED
, WAITING
, …), so calling them
- will corrupt the behavior of the JVM. In Java, the Thread
class represents a thread of execution. Synchronization between threads is typically achieved using objects or
+shared resources.
The methods wait(…)
, notify()
, and notifyAll()
are related to the underlying object’s monitor and are
+designed to be called on objects that act as locks or monitors for synchronization. These methods are available on Java Object
and,
+therefore, automatically inherited by all objects, including Thread.
Calling these methods on a Thread
may corrupt the behavior of the JVM, which relies on them to change the state of the thread
+(BLOCKED,
WAITING,
…).
Thread myThread = new Thread(new RunnableJob()); ... -myThread.wait(2000); +myThread.wait(); // Noncompliant+
When a parent class references a member of a subclass during its own initialization, the results might not be what you expect because the child -class might not have been initialized yet. This could create what is known as an "initialisation cycle", or even a deadlock in some extreme cases.
-To make things worse, these issues are very hard to diagnose so it is highly recommended you avoid creating this kind of dependencies.
+Referencing a static member of a subclass from its parent during class initialization, makes the code more fragile and prone to future bugs. The +execution of the program will rely heavily on the order of initialization of classes and their static members.
+This could create what is known as an "initialization cycle", or even a deadlock in some extreme cases. Additionally, if the order of the static +class members is changed, the behavior of the program might change. These issues can be very hard to diagnose so it is highly recommended to avoid +creating this kind of dependencies.
class Parent { @@ -23,7 +26,7 @@Resources
While it is technically correct to use a Thread
where a Runnable
is called for, the semantics of the two objects are
-different, and mixing them is a bad practice that will likely lead to headaches in the future.
The crux of the issue is that Thread
is a larger concept than Runnable
. A Runnable
is an object whose
-running should be managed. A Thread
expects to manage the running of itself or other Runnables
.
The semantics of Thread
and Runnable
are different, and while it is technically correct to use Thread
where
+a Runnable
is expected, it is a bad practice to do so.
The crux of the issue is that Thread
is a larger concept than Runnable
. A Runnable
represents a task. A
+Thread
represents a task and its execution management (ie: how it should behave when started, stopped, resumed, …). It is both a task
+and a lifecycle management.
- public static void main(String[] args) { - Thread r =new Thread() { - int p; - @Override - public void run() { - while(true) - System.out.println("a"); - } - }; - new Thread(r).start(); // Noncompliant ++public static void main(String[] args) { + Thread runnable = new Thread() { + @Override + public void run() { /* ... */ } + }; + new Thread(runnable).start(); // Noncompliant +}Compliant solution
-- public static void main(String[] args) { - Runnable r =new Runnable() { - int p; - @Override - public void run() { - while(true) - System.out.println("a"); - } - }; - new Thread(r).start(); ++public static void main(String[] args) { + Runnable runnable = new Runnable() { + @Override + public void run() { /* ... */ } + }; + new Thread(runnable).start(); +}diff --git a/java-checks/src/main/resources/org/sonar/l10n/java/rules/java/S2438.json b/java-checks/src/main/resources/org/sonar/l10n/java/rules/java/S2438.json index 21381a5bc07..d40c898a4e3 100644 --- a/java-checks/src/main/resources/org/sonar/l10n/java/rules/java/S2438.json +++ b/java-checks/src/main/resources/org/sonar/l10n/java/rules/java/S2438.json @@ -1,5 +1,5 @@ { - "title": "\"Threads\" should not be used where \"Runnables\" are expected", + "title": "\"Thread\" should not be used where a \"Runnable\" argument is expected", "type": "CODE_SMELL", "status": "ready", "remediation": { diff --git a/java-checks/src/main/resources/org/sonar/l10n/java/rules/java/S2441.html b/java-checks/src/main/resources/org/sonar/l10n/java/rules/java/S2441.html index 258aa22c68c..5039880cfc9 100644 --- a/java-checks/src/main/resources/org/sonar/l10n/java/rules/java/S2441.html +++ b/java-checks/src/main/resources/org/sonar/l10n/java/rules/java/S2441.html @@ -1,20 +1,26 @@Why is this an issue?
-If you have no intention of writting an
-HttpSession
object to file, then storing non-serializable
objects in it may not -seem like a big deal. But whether or not you explicitly serialize the session, it may be written to disk anyway, as the server manages its memory use -in a process called "passivation". Further, some servers automatically write their active sessions out to file at shutdown & deserialize any such -sessions at startup.The point is, that even though
+HttpSession
does notextend Serializable
, you must nonetheless assume that it will be -serialized, and understand that if you’ve stored non-serializable objects in the session, errors will result.+
HttpSession
s are managed by web servers and can be serialized and stored on disk as the server manages its memory use in a process +called "passivation" (and later restored during "activation").Even though
HttpSession
does not extendSerializable
, you must nonetheless assume that it will be serialized. If +non-serializable objects are stored in the session, serialization might fail.Noncompliant code example
-+public class Address { //... } -//... HttpSession session = request.getSession(); session.setAttribute("address", new Address()); // Noncompliant; Address isn't serializable+Compliant solution
++public class Address implements Serializable { + //... +} + +HttpSession session = request.getSession(); +session.setAttribute("address", new Address()); +Resources
java.util.concurrent.locks.Lock
offers far more powerful and flexible locking operations than are available with
-synchronized
blocks. So synchronizing on a Lock
throws away the power of the object, and is just silly. Instead, such
-objects should be locked and unlocked using tryLock()
and unlock()
.
synchronized
blocks. So synchronizing on a Lock
instance throws away the power of the object, as it overrides its better
+locking mechanisms. Instead, such objects should be locked and unlocked using one of their lock
and unlock
method
+variants.
+Lock lock = new MyLockImpl(); synchronized(lock) { // Noncompliant - //... + // ... }Compliant solution
-+Lock lock = new MyLockImpl(); -lock.tryLock(); -//... +if (lock.tryLock()) { + try { + // ... + } finally { + lock.unlock(); + } +}Resources
The story is very similar for synchronizing on parameters; two different threads running the method in parallel could pass two different object instances in to the method as parameters, completely undermining the synchronization.
+private String color = "red"; private void doSomething(){ @@ -20,7 +20,7 @@Noncompliant code example
}Compliant solution
-+private String color = "red"; private final Object lockObj = new Object(); diff --git a/java-checks/src/main/resources/org/sonar/l10n/java/rules/java/S2446.html b/java-checks/src/main/resources/org/sonar/l10n/java/rules/java/S2446.html index 210809a6078..8c2db9c1bc9 100644 --- a/java-checks/src/main/resources/org/sonar/l10n/java/rules/java/S2446.html +++ b/java-checks/src/main/resources/org/sonar/l10n/java/rules/java/S2446.html @@ -1,28 +1,31 @@Why is this an issue?
-+
notify
andnotifyAll
both wake up sleeping threads, butnotify
only rouses one, whilenotifyAll
-rouses all of them. Sincenotify
might not wake up the right thread,notifyAll
should be used instead.
notify
andnotifyAll
both wake up sleeping threads waiting on the object’s monitor, butnotify
only wakes up +one single thread, whilenotifyAll
wakes them all up. Unless you do not care which specific thread is woken up,notifyAll
+should be used instead.Noncompliant code example
--class MyThread extends Thread{ ++class MyThread implements Runnable { + Object lock = new Object(); @Override - public void run(){ - synchronized(this){ + public void run() { + synchronized(lock) { // ... - notify(); // Noncompliant + lock.notify(); // Noncompliant } } }Compliant solution
--class MyThread extends Thread{ ++class MyThread implements Runnable { + Object lock = new Object(); @Override - public void run(){ - synchronized(this){ + public void run() { + synchronized(lock) { // ... - notifyAll(); + lock.notifyAll(); } } } diff --git a/java-checks/src/main/resources/org/sonar/l10n/java/rules/java/S2446.json b/java-checks/src/main/resources/org/sonar/l10n/java/rules/java/S2446.json index 4d043bf44bd..b8d43e88e6b 100644 --- a/java-checks/src/main/resources/org/sonar/l10n/java/rules/java/S2446.json +++ b/java-checks/src/main/resources/org/sonar/l10n/java/rules/java/S2446.json @@ -1,5 +1,5 @@ { - "title": "\"notifyAll\" should be used", + "title": "\"notifyAll()\" should be preferred over \"notify()\"", "type": "BUG", "status": "ready", "remediation": { diff --git a/java-checks/src/main/resources/org/sonar/l10n/java/rules/java/S2447.html b/java-checks/src/main/resources/org/sonar/l10n/java/rules/java/S2447.html index 73ed54d3a45..a0b5d2f9cc4 100644 --- a/java-checks/src/main/resources/org/sonar/l10n/java/rules/java/S2447.html +++ b/java-checks/src/main/resources/org/sonar/l10n/java/rules/java/S2447.html @@ -1,13 +1,39 @@Why is this an issue?
-While
+null
is technically a validBoolean
value, that fact, and the distinction betweenBoolean
and -boolean
is easy to forget. So returningnull
from aBoolean
method is likely to cause problems with callers' -code.Callers of a
Boolean
method may be expecting to receivetrue
orfalse
in response. ButBoolean
+objects can takenull
as a possible value.Boolean
methods should not returnnull
unless the code is annotated +appropriately. With the proper annotation, the caller is aware that the returned value could be null.Noncompliant code example
public Boolean isUsable() { // ... return null; // Noncompliant } + +public void caller() { + if (isUsable()) { // A NullPointerException might occur here + // ... + } +} ++Compliant solution
++@javax.annotation.Nullable +public Boolean isUsable() { + // ... + return null; +} + +@javax.annotation.CheckForNull +public Boolean isUsable() { + // ... + return null; +} + +public void caller() { + if (Boolean.True.equals(isUsable())) { // This caller knows to check and avoid ambiguity + // ... + } +}Resources
Conditional expressions which are always true
or false
can lead to dead code. Such code is always buggy and should never
-be used in production.
Conditional expressions which are always true
or false
can lead to unreachable code.
a = false; diff --git a/java-checks/src/main/resources/org/sonar/l10n/java/rules/java/S2629.html b/java-checks/src/main/resources/org/sonar/l10n/java/rules/java/S2629.html index d914c824b40..b96e9f5c42c 100644 --- a/java-checks/src/main/resources/org/sonar/l10n/java/rules/java/S2629.html +++ b/java-checks/src/main/resources/org/sonar/l10n/java/rules/java/S2629.html @@ -1,6 +1,8 @@Why is this an issue?
+Some method calls can effectively be "no-ops", meaning that the invoked method does nothing, based on the application’s configuration (eg: debug +logs in production). However, even if the method effectively does nothing, its arguments may still need to evaluated before the method is called.
Passing message arguments that require further evaluation into a Guava
+performance penalty. That is because whether or not they’re needed, each argument must be resolved before the method is actually called.com.google.common.base.Preconditions
check can result in a -performance penalty. That’s because whether or not they’re needed, each argument must be resolved before the method is actually called.Similarly, passing concatenated strings into a logging method can also incur a needless performance hit because the concatenation will be performed every time the method is called, whether or not the log level is low enough to show the message.
Instead, you should structure your code to pass static or pre-computed values into
Preconditions
conditions check and logging @@ -23,12 +25,11 @@Noncompliant code example
-logger.log(Level.SEVERE, "Something went wrong: {0} ", message); // String formatting only applied if needed +logger.log(Level.DEBUG, "Something went wrong: {0} ", message); // String formatting only applied if needed +logger.log(Level.SEVERE, () -> "Something went wrong: " + message); // since Java 8, we can use Supplier , which will be evaluated lazily logger.fine("An exception occurred with message: {}", message); // SLF4J, Log4j -logger.log(Level.SEVERE, () -> "Something went wrong: " + message); // since Java 8, we can use Supplier , which will be evaluated lazily - LOG.error("Unable to open file {0}", csvPath, e); if (LOG.isDebugEnabled()) { diff --git a/java-checks/src/main/resources/org/sonar/l10n/java/rules/java/S2681.html b/java-checks/src/main/resources/org/sonar/l10n/java/rules/java/S2681.html index 684e83d9a94..a3fa534db5a 100644 --- a/java-checks/src/main/resources/org/sonar/l10n/java/rules/java/S2681.html +++ b/java-checks/src/main/resources/org/sonar/l10n/java/rules/java/S2681.html @@ -1,43 +1,32 @@Why is this an issue?
-Curly braces can be omitted from a one-line block, such as with an
-if
statement orfor
loop, but doing so can be -misleading and induce bugs.This rule raises an issue when the whitespacing of the lines after a one line block indicates an intent to include those lines in the block, but +
Having inconsistent indentation and omitting curly braces from a control structure, such as an
+if
statement orfor
loop, +is misleading and can induce bugs.This rule raises an issue when the indentation of the lines after a control structure indicates an intent to include those lines in the block, but the omission of curly braces means the lines will be unconditionally executed once.
-Note that this rule considers tab characters to be equivalent to 1 space. If you mix spaces and tabs you will sometimes see issues in code which -look fine in your editor but are confusing when you change the size of tabs.
-Noncompliant code example
+The following patterns are recognized:
if (condition) firstActionInBlock(); - secondAction(); // Noncompliant; executed unconditionally + secondAction(); // Noncompliant: secondAction is executed unconditionally thirdAction(); - -if (condition) firstActionInBlock(); secondAction(); // Noncompliant; secondAction executed unconditionally - -if (condition) firstActionInBlock(); // Noncompliant - secondAction(); // Executed unconditionally - -if (condition); secondAction(); // Noncompliant; secondAction executed unconditionally - -String str = null; -for (int i = 0; i < array.length; i++) - str = array[i]; - doTheThing(str); // Noncompliant; executed only on last array element-Compliant solution
-if (condition) { - firstActionInBlock(); - secondAction(); -} -thirdAction(); - -String str = null; -for (int i = 0; i < array.length; i++) { +if (condition) firstActionInBlock(); secondAction(); // Noncompliant: secondAction is executed unconditionally +++if (condition) firstActionInBlock(); + secondAction(); // Noncompliant: secondAction is executed unconditionally +++if (condition); secondAction(); // Noncompliant: secondAction is executed unconditionally +++for (int i = 0; i < array.length; i++) str = array[i]; - doTheThing(str); -} + doTheThing(str); // Noncompliant: executed only on the last element+Note that this rule considers tab characters to be equivalent to 1 space. When mixing spaces and tabs, a code may look fine in one editor but be +confusing in another configured differently.
Resources
Most checks against an indexOf
value compare it with -1 because 0 is a valid index. Any checks which look for values >0 ignore the
-first element, which is likely a bug. If the intent is merely to check inclusion of a value in a String
or a List
, consider
-using the contains
method instead.
This rule raises an issue when an indexOf
value retrieved either from a String
or a List
is tested against
->0
.
Most checks against an indexOf
value compare it with -1 because 0 is a valid index. Checking against > 0
ignores the
+first element, which is likely a bug.
-String color = "blue"; String name = "ishmael"; -List<String> strings = new ArrayList<String> (); -strings.add(color); -strings.add(name); - -if (strings.indexOf(color) > 0) { // Noncompliant - // ... -} if (name.indexOf("ish") > 0) { // Noncompliant // ... } -if (name.indexOf("ae") > 0) { // Noncompliant - // ... -}-
Moreover, if the intent is merely to check the inclusion of a value in a String
or a List
, consider using the
+contains
method instead.
-String color = "blue"; String name = "ishmael"; -List<String> strings = new ArrayList<String> (); -strings.add(color); -strings.add(name); - -if (strings.indexOf(color) > -1) { +if (name.contains("ish") { // ... } -if (name.indexOf("ish") >= 0) { - // ... -} -if (name.contains("ae") { ++
If the intent is really to skip the first element, comparing it with >=1
will make it more straightforward.
+String name = "ishmael"; + +if (name.indexOf("ish") >= 1) { // ... }+
This rule raises an issue when an indexOf
value retrieved from a String
or a List
is tested against
+> 0
.
The use of operators pairs ( =+
, =-
or =!
) where the reversed, single operator was meant (+=
,
--=
or !=
) will compile and run, but not produce the expected results.
This rule raises an issue when =+
, =-
, or =!
is used without any spacing between the two operators and when
-there is at least one whitespace character after.
Using operator pairs (=+
, =-
, or =!
) that look like reversed single operators (+=
,
+-=
or !=
) is confusing. They compile and run but do not produce the same result as their mirrored counterpart.
int target = -5; int num = 3; -target =- num; // Noncompliant; target = -3. Is that really what's meant? -target =+ num; // Noncompliant; target = 3 +target =- num; // Noncompliant: target = -3. Is that the intended behavior? +target =+ num; // Noncompliant: target = 3-
This rule raises an issue when =+
, =-
, or =!
are used without any space between the operators and when there
+is at least one whitespace after.
Replace the operators with a single one if that is the intention
int target = -5; int num = 3; -target = -num; // Compliant; intent to assign inverse value of num is clear -target += num; +target -= num; // target = -8 ++
Or fix the spacing to avoid confusion
++int target = -5; +int num = 3; + +target = -num; // target = -3diff --git a/java-checks/src/main/resources/org/sonar/l10n/java/rules/java/S2757.json b/java-checks/src/main/resources/org/sonar/l10n/java/rules/java/S2757.json index bdf3a05503d..acd83d9f504 100644 --- a/java-checks/src/main/resources/org/sonar/l10n/java/rules/java/S2757.json +++ b/java-checks/src/main/resources/org/sonar/l10n/java/rules/java/S2757.json @@ -1,5 +1,5 @@ { - "title": "\"\u003d+\" should not be used instead of \"+\u003d\"", + "title": "Non-existent operators like \"\u003d+\" should not be used", "type": "BUG", "status": "ready", "remediation": { diff --git a/java-checks/src/main/resources/org/sonar/l10n/java/rules/java/S2761.html b/java-checks/src/main/resources/org/sonar/l10n/java/rules/java/S2761.html index 3a00dc423d5..1bcffdda9b8 100644 --- a/java-checks/src/main/resources/org/sonar/l10n/java/rules/java/S2761.html +++ b/java-checks/src/main/resources/org/sonar/l10n/java/rules/java/S2761.html @@ -1,30 +1,31 @@
The needless repetition of an operator is usually a typo. There is no reason to write !!!i
when !i
will do.
On the other hand, the repetition of increment and decrement operators may have been done on purpose, but doing so obfuscates the meaning, and -should be simplified.
-This rule raises an issue for sequences of: !
, ~
, -
, and +
.
The repetition of a unary operator is usually a typo. The second operator invalidates the first one in most cases:
int i = 1; -int j = - - -i; // Noncompliant; just use -i -int k = ~~~i; // Noncompliant; same as i -int m = + +i; // Noncompliant; operators are useless here +int j = - - -i; // Noncompliant: equivalent to "-i" +int k = ~~~i; // Noncompliant: equivalent to "~i" +int m = + +i; // Noncompliant: equivalent to "i" boolean b = false; boolean c = !!!b; // Noncompliant-
On the other hand, while repeating the increment and decrement operators is technically correct, it obfuscates the meaning:
-int i = 1; - -int j = -i; -int k = ~i; -int m = i; - -boolean b = false; -boolean c = !b; +int i = 1; +int j = ++ ++i; // Noncompliant +int k = i-- --; // Noncompliant ++
Using +=
or -=
improves readability:
+int i = 1; +i += 2; +int j = i; +int k = i; +i -=2;+
This rule raises an issue for repetitions of !
, ~
, -
, +
, prefix increments ++
and
+prefix decrements --
.
Overflow handling for GWT compilation using ~~
is ignored.
This rule raises an issue when a class overrides the Object.clone
method instead of resorting to a copy constructor or other copy
+mechanisms.
Many consider clone
and Cloneable
broken in Java, largely because the rules for overriding clone
are tricky
-and difficult to get right, according to Joshua Bloch:
--Object’s clone method is very tricky. It’s based on field copies, and it’s "extra-linguistic." It creates an object without calling a - constructor. There are no guarantees that it preserves the invariants established by the constructors. There have been lots of bugs over the years, - both in and outside Sun, stemming from the fact that if you just call super.clone repeatedly up the chain until you have cloned an object, you have - a shallow copy of the object. The clone generally shares state with the object being cloned. If that state is mutable, you don’t have two - independent objects. If you modify one, the other changes as well. And all of a sudden, you get random behavior.
-
A copy constructor or copy factory should be used instead.
-This rule raises an issue when clone
is overridden, whether or not Cloneable
is implemented.
-public class MyClass { - // ... - - public Object clone() { // Noncompliant - //... +The
+Object.clone
/java.lang.Cloneable
mechanism in Java should be considered broken for the following reasons and +should, consequently, not be used:
Cloneable
is a marker interface without API but with a contract about class behavior that the compiler cannot enforce.
+ This is a bad practice. Object.clone
, like type casts or the handling of
+ CloneNotSupportedException
exceptions. A copy constructor, copy factory or a custom copy function are suitable alternatives to the Object.clone
/
+java.lang.Cloneable
mechanism.
Consider the following example:
++class Entity implements Cloneable { // Noncompliant, using `Cloneable` + + public int value; + public List<Entity> children; // deep copy wanted + + Entity() { + EntityManager.register(this); // invariant + } + + @Override + public Entity clone() { + try { + Entity copy = (Entity) super.clone(); // invariant not enforced, because no constructor is caled + copy.children = children.stream().map(Entity::clone).toList(); + return copy; + } catch (CloneNotSupportedException e) { // this will not happen due to behavioral contract + throw new AssertionError(); + } } }-
-public class MyClass { - // ... +The
+Cloneable
/Object.clone
mechanism could easily be replaced by copy constructor:+class Entity { // Compliant + + public int value; + public List<Entity> children; // deep copy wanted + + Entity() { + EntityManager.register(this); // invariant + } + + Entity(Entity template) { + value = template.value; + children = template.children.stream().map(Entity::new).toList(); + } +} ++Or by a factory method:
++class Entity { // Compliant + + public int value; + public List<Entity> children; // deep copy wanted + + Entity() { + EntityManager.register(this); // invariant + } + + public static Entity create(Entity template) { + Entity entity = new Entity(); + entity.value = template.value; + entity.children = template.children.stream().map(Entity::new).toList(); + return Entity; + } +} ++Or by a custom
+copy
function:+class Entity { // Compliant + + public int value; + public List<Entity> children; // deep copy wanted + + Entity() { + EntityManager.register(this); // invariant + } - MyClass (MyClass source) { - //... + public Entity copy() { + Entity entity = new Entity(); + entity.value = value; + entity.children = children.stream().map(Entity::new).toList(); + return Entity; } }Resources
+Documentation
Altering or bypassing the accessibility of classes, methods, or fields through reflection violates the encapsulation principle. This can break the +internal contracts of the accessed target and lead to maintainability issues and runtime errors.
This rule raises an issue when reflection is used to change the visibility of a class, method or field, and when it is used to directly update a -field value. Altering or bypassing the accessibility of classes, methods, or fields violates the encapsulation principle and could lead to run-time -errors.
-public void makeItPublic(String methodName) throws NoSuchMethodException { @@ -14,7 +14,9 @@Noncompliant code example
}
Just because you can do something, doesn’t mean you should, and that’s the case with nested ternary operations. Nesting ternary operators -results in the kind of code that may seem clear as day when you write it, but six months later will leave maintainers (or worse - future you) -scratching their heads and cursing.
-Instead, err on the side of clarity, and use another line to express the nested operation as a separate statement.
-Nested ternaries are hard to read and can make the order of operations complex to understand.
public String getReadableStatus(Job j) { return j.isRunning() ? "Running" : j.hasErrors() ? "Failed" : "Succeeded"; // Noncompliant }-
Instead, use another line to express the nested operation in a separate statement.
public String getReadableStatus(Job j) { if (j.isRunning()) { diff --git a/java-checks/src/main/resources/org/sonar/l10n/java/rules/java/S3776.html b/java-checks/src/main/resources/org/sonar/l10n/java/rules/java/S3776.html index 73066d872eb..c69347fef18 100644 --- a/java-checks/src/main/resources/org/sonar/l10n/java/rules/java/S3776.html +++ b/java-checks/src/main/resources/org/sonar/l10n/java/rules/java/S3776.html @@ -3,8 +3,9 @@diff --git a/java-checks/src/main/resources/org/sonar/l10n/java/rules/java/S3923.html b/java-checks/src/main/resources/org/sonar/l10n/java/rules/java/S3923.html index 762051617d9..caf4dbad6cb 100644 --- a/java-checks/src/main/resources/org/sonar/l10n/java/rules/java/S3923.html +++ b/java-checks/src/main/resources/org/sonar/l10n/java/rules/java/S3923.html @@ -1,7 +1,6 @@Why is this an issue?
difficult to maintain.Exceptions
+understand, especially in the presence of many fields.
equals
andhashCode
methods are ignored because they might be automatically generated and might end up being difficult to -understand, especially in presence of many fields.Resources
+Documentation
Having all branches in a switch
or if
chain with the same implementation is an error. Either a copy-paste error was made
-and something different should be executed, or there shouldn’t be a switch
/if
chain at all.
Having all branches of a switch
or if
chain with the same implementation indicates a problem.
In the following code:
if (b == 0) { // Noncompliant doOneMoreThing(); @@ -25,9 +24,9 @@+Noncompliant code example
doSomething(); }
Either there is a copy-paste error that needs fixing or an unnecessary switch
or if
chain that needs removing.
This rule does not apply to if
chains without else
-s, or to switch
-es without default
-clauses.
This rule does not apply to if
chains without else
, nor to witch
without a default
clause.
if(b == 0) { //no issue, this could have been done on purpose to make the code more readable doSomething(); diff --git a/java-checks/src/main/resources/org/sonar/l10n/java/rules/java/S3972.html b/java-checks/src/main/resources/org/sonar/l10n/java/rules/java/S3972.html index b9a7ad8f6a8..e439ad34a8b 100644 --- a/java-checks/src/main/resources/org/sonar/l10n/java/rules/java/S3972.html +++ b/java-checks/src/main/resources/org/sonar/l10n/java/rules/java/S3972.html @@ -1,31 +1,30 @@Why is this an issue?
-Code is clearest when each statement has its own line. Nonetheless, it is a common pattern to combine on the same line an
-if
and its -resulting then statement. However, when anif
is placed on the same line as the closing}
from a preceding -then, else or else if part, it is either an error -else
is missing - or the invitation to a future error as -maintainers fail to understand that the two statements are unconnected.Noncompliant code example
-+Placing an
+if
statement on the same line as the closing}
from a precedingif
,else
, or +else if
block can lead to confusion and potential errors. It may indicate a missingelse
statement or create ambiguity for +maintainers who might fail to understand that the two statements are unconnected.The following code snippet is confusing:
+if (condition1) { // ... } if (condition2) { // Noncompliant //... }-Compliant solution
-+Either the two conditions are unrelated and they should be visually separated:
+if (condition1) { // ... -} else if (condition2) { +} + +if (condition2) { //... }-Or
+Or they were supposed to be exclusive and you should use
else if
instead:if (condition1) { // ... -} - -if (condition2) { +} else if (condition2) { //... }diff --git a/java-checks/src/main/resources/org/sonar/l10n/java/rules/java/S3973.html b/java-checks/src/main/resources/org/sonar/l10n/java/rules/java/S3973.html index 43d0aee8aff..de6a23baac6 100644 --- a/java-checks/src/main/resources/org/sonar/l10n/java/rules/java/S3973.html +++ b/java-checks/src/main/resources/org/sonar/l10n/java/rules/java/S3973.html @@ -1,25 +1,30 @@Why is this an issue?
-In the absence of enclosing curly braces, the line immediately after a conditional is the one that is conditionally executed. By both convention -and good practice, such lines are indented. In the absence of both curly braces and indentation the intent of the original programmer is entirely -unclear and perhaps not actually what is executed. Additionally, such code is highly likely to be confusing to maintainers.
-Noncompliant code example
+When the line immediately after a conditional has neither curly braces nor indentation, the intent of the code is unclear and perhaps not what is +executed. Additionally, such code is confusing to maintainers.
if (condition) // Noncompliant doTheThing(); - -doTheOtherThing(); -somethingElseEntirely(); - -foo(); +doTheOtherThing(); // Was the intent to call this function unconditionally? ++It becomes even more confusing and bug-prone if lines get commented out.
++if (condition) // Noncompliant +// doTheThing(); +doTheOtherThing(); // Was the intent to call this function conditionally?-Compliant solution
+Indentation alone or together with curly braces makes the intent clear.
if (condition) doTheThing(); +doTheOtherThing(); // Clear intent to call this function unconditionally -doTheOtherThing(); -somethingElseEntirely(); +// or -foo(); +if (condition) { + doTheThing(); +} +doTheOtherThing(); // Clear intent to call this function unconditionally+This rule raises an issue if the line controlled by a conditional has the same indentation as the conditional and is not enclosed in curly +braces.
diff --git a/java-checks/src/main/resources/org/sonar/l10n/java/rules/java/S4143.html b/java-checks/src/main/resources/org/sonar/l10n/java/rules/java/S4143.html index 00cab76d930..e1947bea91c 100644 --- a/java-checks/src/main/resources/org/sonar/l10n/java/rules/java/S4143.html +++ b/java-checks/src/main/resources/org/sonar/l10n/java/rules/java/S4143.html @@ -1,6 +1,6 @@Why is this an issue?
-It is highly suspicious when a value is saved for a key or index and then unconditionally overwritten. Such replacements are likely errors.
-Noncompliant code example
+Storing a value inside a collection at a given key or index and then unconditionally overwriting it without reading the initial value is a case of +"dead store".
letters.put("a", "Apple"); letters.put("a", "Boy"); // Noncompliant @@ -8,4 +8,5 @@+Noncompliant code example
towns[i] = "London"; towns[i] = "Chicago"; // NoncompliantAt best it is redundant and will confuse the reader, most often it is an error and not what the developer intended to do.
diff --git a/java-checks/src/main/resources/org/sonar/l10n/java/rules/java/S4144.html b/java-checks/src/main/resources/org/sonar/l10n/java/rules/java/S4144.html index 44cf6cc0be3..84c10192b09 100644 --- a/java-checks/src/main/resources/org/sonar/l10n/java/rules/java/S4144.html +++ b/java-checks/src/main/resources/org/sonar/l10n/java/rules/java/S4144.html @@ -1,9 +1,7 @@Why is this an issue?
-When two methods have the same implementation, either it was a mistake - something else was intended - or the duplication was intentional, but may -be confusing to maintainers. In the latter case, one implementation should invoke the other. Numerical and string literals are not taken into -account.
-Noncompliant code example
-+Two methods having the same implementation are suspicious. It might be that something else was intended. Or the duplication is intentional, which +becomes a maintenance burden.
+private final static String CODE = "bounteous"; public String calculateCode() { @@ -11,13 +9,14 @@-Noncompliant code example
return CODE; } -public String getName() { // Noncompliant +public String getName() { // Noncompliant: duplicates calculateCode doTheThing(); return CODE; }Compliant solution
-+If the identical logic is intentional, the code should be refactored to avoid duplication. For example, by having both methods call the same method +or by having one implementation invoke the other.
+private final static String CODE = "bounteous"; public String getCode() { @@ -25,7 +24,7 @@diff --git a/java-checks/src/main/resources/org/sonar/l10n/java/rules/java/S4275.html b/java-checks/src/main/resources/org/sonar/l10n/java/rules/java/S4275.html index 620e8b67618..e04f0b7814f 100644 --- a/java-checks/src/main/resources/org/sonar/l10n/java/rules/java/S4275.html +++ b/java-checks/src/main/resources/org/sonar/l10n/java/rules/java/S4275.html @@ -1,6 +1,6 @@Compliant solution
return CODE; } -public String getName() { +public String getName() { // The intent is clear return getCode(); }Why is this an issue?
Getters and setters provide a way to enforce encapsulation by providing
public
methods that give controlled access to -private
fields. However in classes with multiple fields it is not unusual that copy and paste is used to quickly create the needed +private
fields. However, in classes with multiple fields, it is not unusual that copy and paste is used to quickly create the needed getters and setters, which can result in the wrong field being accessed by a getter or setter.This rule raises an issue in any of these cases:
switch
can contain a default
clause for various reasons: to handle unexpected values, to show that all the cases were
-properly considered.
For readability purpose, to help a developer to quickly find the default behavior of a switch
statement, it is recommended to put the
-default
clause at the end of the switch
statement. This rule raises an issue if the default
clause is not the
-last one of the switch
's cases.
For readability purposes, to help a developer quickly spot the default behavior of a switch
statement, it is recommended to put the
+default
clause at the end of the switch
statement.
This rule raises an issue if the default
clause is not the last one of the switch
's cases.
switch (param) { case 0: doSomething(); break; - default: // default clause should be the last one + default: // Noncompliant: default clause should be the last one error(); break; case 1: @@ -18,18 +17,4 @@-Noncompliant code example
break; }
-switch (param) { - case 0: - doSomething(); - break; - case 1: - doSomethingElse(); - break; - default: - error(); - break; -} -diff --git a/java-checks/src/main/resources/org/sonar/l10n/java/rules/java/S5547.html b/java-checks/src/main/resources/org/sonar/l10n/java/rules/java/S5547.html index 6ea8c186971..429f5662275 100644 --- a/java-checks/src/main/resources/org/sonar/l10n/java/rules/java/S5547.html +++ b/java-checks/src/main/resources/org/sonar/l10n/java/rules/java/S5547.html @@ -1,63 +1,71 @@ +
This vulnerability makes it possible that the cleartext of the encrypted message might be recoverable without prior knowledge of the key.
Strong cipher algorithms are cryptographic systems resistant to cryptanalysis, they -are not vulnerable to well-known attacks like brute force attacks for example.
-A general recommendation is to only use cipher algorithms intensively tested and promoted by the cryptographic community.
-More specifically for block cipher, it’s not recommended to use algorithm with a block size inferior than 128 bits.
-+Encryption algorithms are essential for protecting sensitive information and ensuring secure communication in various domains. They are used for +several important reasons:
+
When selecting encryption algorithms, tools, or combinations, you should also consider two things:
+For these reasons, as soon as cryptography is included in a project, it is important to choose encryption algorithms that are considered strong and +secure by the cryptography community.
+The cleartext of an encrypted message might be recoverable. Additionally, it might be possible to modify the cleartext of an encrypted message.
+Below are some real-world scenarios that illustrate some impacts of an attacker exploiting the vulnerability.
+The encrypted message might contain data that is considered sensitive and should not be known to third parties.
+By using a weak algorithm the likelihood that an attacker might be able to recover the cleartext drastically increases.
+By modifying the cleartext of the encrypted message it might be possible for an attacker to trigger other vulnerabilities in the code. Encrypted +values are often considered trusted, since under normal circumstances it would not be possible for a third party to modify them.
+The following code contains examples of algorithms that are not considered highly resistant to cryptanalysis and thus should be avoided.
+import javax.crypto.Cipher; import java.security.NoSuchAlgorithmException; import javax.crypto.NoSuchPaddingException; -public class test { - - public static void main(String[] args) { - try - { - Cipher c1 = Cipher.getInstance("DES"); // Noncompliant: DES works with 56-bit keys allow attacks via exhaustive search - Cipher c7 = Cipher.getInstance("DESede"); // Noncompliant: Triple DES is vulnerable to meet-in-the-middle attack - Cipher c13 = Cipher.getInstance("RC2"); // Noncompliant: RC2 is vulnerable to a related-key attack - Cipher c19 = Cipher.getInstance("RC4"); // Noncompliant: vulnerable to several attacks (see https://en.wikipedia.org/wiki/RC4#Security) - Cipher c25 = Cipher.getInstance("Blowfish"); // Noncompliant: Blowfish use a 64-bit block size makes it vulnerable to birthday attacks - - NullCipher nc = new NullCipher(); // Noncompliant: the NullCipher class provides an "identity cipher" one that does not transform or encrypt the plaintext in any way. - } - catch(NoSuchAlgorithmException|NoSuchPaddingException e) - { - } +public static void main(String[] args) { + try { + Cipher des = Cipher.getInstance("DES"); // Noncompliant + } catch(NoSuchAlgorithmException|NoSuchPaddingException e) { + // ... } }-
+Compliant solution
+import javax.crypto.Cipher; import java.security.NoSuchAlgorithmException; import javax.crypto.NoSuchPaddingException; -public class test { - - public static void main(String[] args) { - try - { - Cipher c31 = Cipher.getInstance("AES/GCM/NoPadding"); // Compliant - } - catch(NoSuchAlgorithmException|NoSuchPaddingException e) - { - } +public static void main(String[] args) { + try { + Cipher aes = Cipher.getInstance("AES/GCM/NoPadding"); + } catch(NoSuchAlgorithmException|NoSuchPaddingException e) { + // ... } }+How does this work?
+Use a secure algorithm
+It is highly recommended to use an algorithm that is currently considered secure by the cryptographic community. A common choice for such an +algorithm is the Advanced Encryption Standard (AES).
+For block ciphers, it is not recommended to use algorithms with a block size that is smaller than 128 bits.
Resources
+Standards
Secrets should not be hard-coded in source code, instead be stored outside of the source code in a configuration file or a management service for -secrets.
-There would be a risk, if any of the following apply to you:
-Because it is easy to extract strings from an application source code or binary, secrets should not be hard-coded. This is particularly true for applications that are distributed or that are open-source.
In the past, it has led to the following vulnerabilities:
@@ -14,17 +5,24 @@Secrets should be stored outside of the source code in a configuration file or a management service for secrets.
This rule detects variables/fields having a name matching a list of words (secret, token, credential, auth, api[_.-]?key) being assigned a pseudorandom hard-coded value. The pseudorandomness of the hard-coded value is based on its entropy and the probability to be human-readable. The randomness sensibility can be adjusted if needed. Lower values will detect less random values, raising potentially more false positives.
-There would be a risk if you answered yes to any of those questions.
+private static final String MY_SECRET = "47828a8dd77ee1eb9dde2d5e93cb221ce8c32b37"; @@ -32,7 +30,7 @@-Noncompliant code example
MyClass.callMyService(MY_SECRET); }
Using AWS Secrets Manager:
import software.amazon.awssdk.services.secretsmanager.model.GetSecretValueRequest; @@ -82,12 +80,7 @@-Compliant solution
MyClass.callMyService(secret); }
This rule detects variables/fields having a name matching a list of words (secret, token, credential, auth, api[_.-]?key) being assigned a -pseudorandom hard-coded value. The pseudorandomness of the hard-coded value is based on its entropy and the probability to be human-readable. The -randomness sensibility can be adjusted if needed. Lower values will detect less random values, raising potentially more false positives.
-