Some tools work better when files end with an empty line.
+Some tools work better when files end with a newline.
This rule simply generates an issue if it is missing.
For example, a Git diff looks like this if the empty line is missing at the end of the file:
diff --git a/java-checks/src/main/resources/org/sonar/l10n/java/rules/java/S113.json b/java-checks/src/main/resources/org/sonar/l10n/java/rules/java/S113.json
index b5acb1513c0..1d0fcfdd81e 100644
--- a/java-checks/src/main/resources/org/sonar/l10n/java/rules/java/S113.json
+++ b/java-checks/src/main/resources/org/sonar/l10n/java/rules/java/S113.json
@@ -1,5 +1,5 @@
{
- "title": "Files should contain an empty newline at the end",
+ "title": "Files should end with a newline",
"type": "CODE_SMELL",
"code": {
"impacts": {
diff --git a/java-checks/src/main/resources/org/sonar/l10n/java/rules/java/S1144.html b/java-checks/src/main/resources/org/sonar/l10n/java/rules/java/S1144.html
index 678681fdd3a..c5b43b81fef 100644
--- a/java-checks/src/main/resources/org/sonar/l10n/java/rules/java/S1144.html
+++ b/java-checks/src/main/resources/org/sonar/l10n/java/rules/java/S1144.html
@@ -1,35 +1,34 @@
+This rule raises an issue when a private method is never referenced in the code.
Why is this an issue?
-private methods that are never executed are dead code: unnecessary, inoperative code that should be removed. Cleaning out dead code
-decreases the size of the maintained codebase, making it easier to understand the program and preventing bugs from being introduced.
-Note that this rule does not take reflection into account, which means that issues will be raised on private methods that are only
-accessed using the reflection API.
+A method that is never called is dead code, and should be removed. Cleaning out dead code decreases the size of the maintained codebase, making it
+easier to understand the program and preventing bugs from being introduced.
+This rule detects methods that are never referenced from inside a translation unit, and cannot be referenced from the outside.
+Code examples
Noncompliant code example
-
+
public class Foo implements Serializable
{
- private Foo(){} //Compliant, private empty constructor intentionally used to prevent any direct instantiation of a class.
- public static void doSomething(){
+ public static void doSomething() {
Foo foo = new Foo();
...
}
- private void unusedPrivateMethod(){...}
- private void writeObject(ObjectOutputStream s){...} //Compliant, relates to the java serialization mechanism
- private void readObject(ObjectInputStream in){...} //Compliant, relates to the java serialization mechanism
+
+ private void unusedPrivateMethod() {...}
+ private void writeObject(ObjectOutputStream s) {...} //Compliant, relates to the java serialization mechanism
+ private void readObject(ObjectInputStream in) {...} //Compliant, relates to the java serialization mechanism
}
Compliant solution
-
+
public class Foo implements Serializable
{
- private Foo(){} //Compliant, private empty constructor intentionally used to prevent any direct instantiation of a class.
public static void doSomething(){
Foo foo = new Foo();
...
}
- private void writeObject(ObjectOutputStream s){...} //Compliant, relates to the java serialization mechanism
-
- private void readObject(ObjectInputStream in){...} //Compliant, relates to the java serialization mechanism
+ private void writeObject(ObjectOutputStream s) {...} //Compliant, relates to the java serialization mechanism
+ private void readObject(ObjectInputStream in) {...} //Compliant, relates to the java serialization mechanism
}
Exceptions
@@ -38,4 +37,6 @@ Exceptions
@javax.enterprise.event.Observes The rule does not take reflection into account, which means that issues will be raised on private methods that are only accessed using
+the reflection API.
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.
+In Java, value-based classes are those for which instances are final and immutable, like String, Integer and so on, and
+their identity relies on their value and not their reference. When a variable of one of these types is instantiated, the JVM caches its value, and the
+variable is just a reference to that value. For example, multiple String variables with the same value "Hello world!" will refer to the
+same cached string literal in memory.
The synchronized keyword tells the JVM to only allow the execution of the code contained in the following block to one
+Thread at a time. This mechanism relies on the identity of the object that is being synchronized between threads, to prevent that if
+object X is locked, it will still be possible to lock another object Y.
It means that the JVM will fail to correctly synchronize threads on instances of the aforementioned value-based classes, for instance:
+
+// These variables "a" and "b" will effectively reference the same object in memory
+Integer a = 0;
+Integer b = 0;
+
+// This means that in the following code, the JVM could try to lock and execute
+// on the variable "a" because "b" was notified to be released, as the two Integer variables
+// are the same object to the JVM
+void syncMethod(int x) {
+ synchronized (a) {
+ if (a == x) {
+ // ... do something here
+ }
+ }
+ synchronized (b) {
+ if (b == x) {
+ // ... do something else
+ }
+ }
+}
+
+This behavior can cause unrelated threads to deadlock with unclear stacktraces.
Within the JDK, types which should not be used for synchronization include:
String literals 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); @@ -44,33 +72,33 @@-Noncompliant code example
... }
private static final Object lock1 = new Object();
-private static final Object lock2 = new Object();
-private static final Object lock3 = new Object();
-private static final Object lock4 = new Object();
+private static final Integer iLock = new Integer(42);
+private static final String sLock = new String("A brand new string in memory!");
+private static final List<String> listLock = new ArrayList<>();
public void doSomething() {
synchronized(lock1) { // Compliant
...
}
- synchronized(lock2) { // Compliant
+ synchronized(iLock) { // Compliant
...
}
- synchronized(lock3) { // Compliant
+ synchronized(sLock) { // Compliant
...
}
- synchronized(lock4) { // Compliant
+ synchronized(listLock) { // Compliant
...
}
When the same code is duplicated in two or more separate branches of a conditional, it can make the code harder to understand, maintain, and can +potentially introduce bugs if one instance of the code is changed but others are not.
Having two cases in a switch statement or two branches in an if chain with the same implementation is at
best duplicate code, and at worst a coding error.
+if (a >= 0 && a < 10) {
+ doFirstThing();
+ doTheThing();
+}
+else if (a >= 10 && a < 20) {
+ doTheOtherThing();
+}
+else if (a >= 20 && a < 50) {
+ doFirstThing();
+ doTheThing(); // Noncompliant; duplicates first condition
+}
+else {
+ doTheRest();
+}
+
+
switch (i) {
case 1:
doFirstThing();
@@ -18,27 +36,11 @@ Why is this an issue?
doTheRest();
}
-
-if (a >= 0 && a < 10) {
- doFirstThing();
- doTheThing();
-}
-else if (a >= 10 && a < 20) {
- doTheOtherThing();
-}
-else if (a >= 20 && a < 50) {
- doFirstThing();
- doTheThing(); // Noncompliant; duplicates first condition
-}
-else {
- doTheRest();
-}
-
If the same logic is truly needed for both instances, then:
if chain they should be combined
+
if ((a >= 0 && a < 10) || (a >= 20 && a < 50)) { // Compliant
doFirstThing();
doTheThing();
@@ -51,9 +53,9 @@ Why is this an issue?
}
switch, one should fall through to the other. switch, one should fall through to the other
+
switch (i) {
case 1:
case 3: // Compliant
@@ -74,7 +76,7 @@ Exceptions
switch statement that contains a single line of code with or without a following break.
if (a == 1) {
- doSomething(); //no issue, usually this is done on purpose to increase the readability
+ doSomething(); // Compliant, usually this is done on purpose to increase the readability
} else if (a == 2) {
doSomethingElse();
} else {
diff --git a/java-checks/src/main/resources/org/sonar/l10n/java/rules/java/S1905.html b/java-checks/src/main/resources/org/sonar/l10n/java/rules/java/S1905.html
index ded3c006f5d..b017dfa35a0 100644
--- a/java-checks/src/main/resources/org/sonar/l10n/java/rules/java/S1905.html
+++ b/java-checks/src/main/resources/org/sonar/l10n/java/rules/java/S1905.html
@@ -1,6 +1,6 @@
Why is this an issue?
Casting expressions are utilized to convert one data type to another, such as transforming an integer into a string. This is especially crucial in
-strongly typed languages like C, C++, C#, Java, Python, and others.
+strongly typed languages like C, C++, C#, Java, Python, and others.
However, there are instances where casting expressions are not needed. These include situations like:
A reference to null should never be dereferenced/accessed. Doing so will cause a NullPointerException to be thrown. At
best, such an exception will cause abrupt program termination. At worst, it could expose debugging information that would be useful to an attacker, or
it could allow an attacker to bypass security measures.
Note that when they are present, this rule takes advantage of @CheckForNull and @Nonnull annotations defined in JSR-305 to understand which values are and are not nullable except when @Nonnull is used
-on the parameter to equals, which by contract should always work with null.
+Note that when they are present, this rule takes advantage of nullability annotations, like
+@CheckForNullor@Nonnull, +defined in JSR-305 to understand which values can be null or not.@Nonnullwill be +ignored if used on the parameter of theequalsmethod, which by contract should always work with null.How to fix it
+Code examples
+Noncompliant code example
+The variable
+myObjectis equal tonull, meaning it has no value:+public void method() { + Object myObject = null; + System.out.println(myObject.toString()); // Noncompliant: myObject is null +} ++The parameter
+inputmight benullas suggested by theifcondition:+public void method(Object input) +{ + if (input == null) + { + // ... + } + System.out.println(input.toString()); // Noncompliant +} ++The unboxing triggered in the return statement will throw a
+NullPointerException:+public boolean method() { + Boolean boxed = null; + return boxed; // Noncompliant +} ++Both
+connandstmtmight benullin case an exception was thrown in the try{} block:+Connection conn = null; +Statement stmt = null; +try { + conn = DriverManager.getConnection(DB_URL,USER,PASS); + stmt = conn.createStatement(); + // ... +} catch(Exception e) { + e.printStackTrace(); +} finally { + stmt.close(); // Noncompliant + conn.close(); // Noncompliant +} ++As
+getName()is annotated with@CheckForNull, there is a risk ofNullPointerExceptionhere:@CheckForNull -String getName(){...} +String getName() {...} public boolean isNameEmpty() { - return getName().length() == 0; // Noncompliant; the result of getName() could be null, but isn't null-checked + return getName().length() == 0; // Noncompliant +} ++As
+merge(…)parameter is annotated with@Nonnull, passing an identified potential null value (thanks to @CheckForNull) +is not safe:+private void merge(@Nonnull Color firstColor, @Nonnull Color secondColor) {...} + +public void append(@CheckForNull Color color) { + merge(currentColor, color); // Noncompliant: color should be null-checked because merge(...) doesn't accept nullable parameters +} ++Compliant solution
+Ensuring the variable
+myObjecthas a value resolves the issue:+public void method() { + Object myObject = new Object(); + System.out.println(myObject.toString()); // Compliant: myObject is not null +} ++Preventing the non-compliant code to be executed by returning early:
++public void method(Object input) +{ + if (input == null) + { + return; + } + System.out.println(input.toString()); // Compliant: if 'input' is null, this is unreachable +} ++Ensuring that no unboxing of
+nullvalue can happen resolves the issue+public boolean method() { + Boolean boxed = true; + return boxed; // Compliant }-+Ensuring that both
+connandstmtare notnullresolves the issue:Connection conn = null; Statement stmt = null; -try{ +try { conn = DriverManager.getConnection(DB_URL,USER,PASS); stmt = conn.createStatement(); // ... - -}catch(Exception e){ +} catch(Exception e) { e.printStackTrace(); -}finally{ - stmt.close(); // Noncompliant; stmt could be null if an exception was thrown in the try{} block - conn.close(); // Noncompliant; conn could be null if an exception was thrown +} finally { + if (stmt != null) { + stmt.close(); // Compliant + } + if (conn != null) { + conn.close(); // Compliant + } }--private void merge(@Nonnull Color firstColor, @Nonnull Color secondColor){...} +Checking the returned value of
+getName()resolves the issue:+@CheckForNull +String getName() {...} -public void append(@CheckForNull Color color) { - merge(currentColor, color); // Noncompliant; color should be null-checked because merge(...) doesn't accept nullable parameters +public boolean isNameEmpty() { + String name = getName(); + if (name != null) { + return name.length() == 0; // Compliant + } else { + // ... + } }--void paint(Color color) { - if(color == null) { - System.out.println("Unable to apply color " + color.toString()); // Noncompliant; NullPointerException will be thrown - return; +Ensuring that the provided
+coloris notnullresolves the issue:+private void merge(@Nonnull Color firstColor, @Nonnull Color secondColor) {...} + +public void append(@CheckForNull Color color) { + if (color != null) { + merge(currentColor, color); // Compliant } - ... }Resources
This rule raises an issue when the code cognitive complexity of a function is above a certain threshold.
Cognitive Complexity is a measure of how hard the control flow of a method is to understand. Methods with high Cognitive Complexity will be -difficult to maintain.
+Cognitive Complexity is a measure of how hard it is to understand the control flow of a unit of code. Code with high cognitive complexity is hard +to read, understand, test, and modify.
+As a rule of thumb, high cognitive complexity is a sign that the code should be refactored into smaller, easier-to-manage pieces.
+Here are the core concepts:
+The method of computation is fully detailed in the pdf linked in the resources.
equals and hashCode methods are ignored because they might be automatically generated and might end up being difficult to
understand, especially in the presence of many fields.
Reducing cognitive complexity can be challenging.
Here are a few suggestions:
Extraction of a complex condition in a new function.
+The code is using a complex condition and has a cognitive cost of 3.
+
+double calculateFinalPrice(User user, Cart cart) {
+ double total = calculateTotal(cart);
+ if (user.hasMembership() // +1 (if)
+ && user.ordersCount() > 10 // +1 (more than one condition)
+ && user.isAccountActive()
+ && !user.hasDiscount()
+ || user.ordersCount() == 1) { // +1 (change of operator in condition)
+ total = applyDiscount(user, total);
+ }
+ return total;
+}
+
+Even if the cognitive complexity of the whole program did not change, it is easier for a reader to understand the code of the
+calculateFinalPrice function, which now only has a cognitive cost of 1.
+double calculateFinalPrice(User user, Cart cart) {
+ double total = calculateTotal(cart);
+ if (isEligibleForDiscount(user)) { // +1 (if)
+ total = applyDiscount(user, total);
+ }
+ return total;
+}
+
+boolean isEligibleForDiscount(User user) {
+ return user.hasMembership()
+ && user.ordersCount() > 10 // +1 (more than one condition)
+ && user.isAccountActive()
+ && !user.hasDiscount()
+ || user.ordersCount() == 1; // +1 (change of operator in condition)
+}
+
+Break down large functions.
+For example, consider a function that calculates the total price of a shopping cart, including sales tax and shipping.
Note: The code
+is simplified here, to illustrate the purpose. Please imagine there is more happening in the for loops.
+double calculateTotal(Cart cart) {
+ double total = 0;
+ for (Item item : cart.items()) { // +1 (for)
+ total += item.price;
+ }
+
+ // calculateSalesTax
+ for (Item item : cart.items()) { // +1 (for)
+ total += 0.2 * item.price;
+ }
+
+ //calculateShipping
+ total += 5 * cart.items().size();
+
+ return total;
+}
+
+This function could be refactored into smaller functions: The complexity is spread over multiple functions and the complex
+calculateTotal has now a complexity score of zero.
+double calculateTotal(Cart cart) {
+ double total = 0;
+ total = calculateSubtotal(cart, total);
+ total += calculateSalesTax(cart, total);
+ total += calculateShipping(cart, total);
+
+ return total;
+}
+
+double calculateShipping(Cart cart, double total) {
+ total += 5 * cart.items().size();
+ return total;
+}
+
+double calculateSalesTax(Cart cart, double total) {
+ for (Item item : cart.items()) { // +1 (for)
+ total += 0.2 * item.price;
+ }
+ return total;
+}
+
+double calculateSubtotal(Cart cart, double total) {
+ for (Item item : cart.items()) { // +1 (for)
+ total += item.price;
+ }
+ return total;
+}
+
+Avoid deep nesting by returning early.
+The below code has a cognitive complexity of 6.
+
+double calculateDiscount(double price, User user) {
+ if (isEligibleForDiscount(user)) { // +1 ( if )
+ if (user.hasMembership()) { // +2 ( nested if )
+ return price * 0.9;
+ } else if (user.ordersCount() == 1) { // +1 ( else )
+ return price * 0.95;
+ } else { // +1 ( else )
+ return price;
+ }
+ } else { // +1 ( else )
+ return price;
+ }
+}
+
+Checking for the edge case first flattens the if statements and reduces the cognitive complexity to 3.
+double calculateDiscount(double price, User user) {
+ if (!isEligibleForDiscount(user)) { // +1 ( if )
+ return price;
+ }
+ if (user.hasMembership()) { // +1
+ return price * 0.9;
+ }
+ if (user.ordersCount() == 1) { // +1 ( if )
+ return price * 0.95;
+ }
+ return price;
+}
+
+As this code is complex, ensure that you have unit tests that cover the code before refactoring.
Dependency injection frameworks such as Spring support dependency injection by using annotations such as @Inject and
-@Autowired. These annotation can be used to inject beans via constructor, setter, and field injection.
@Autowired. These annotations can be used to inject beans via constructor, setter, and field injection.
Generally speaking, field injection is discouraged. It allows the creation of objects in an invalid state and makes testing more difficult. The dependencies are not explicit when instantiating a class that uses field injection.
-Apart from that, field injection is not compatible with final fields. Keeping dependencies immutable where possible makes the code easier to +
In addition, field injection is not compatible with final fields. Keeping dependencies immutable where possible makes the code easier to understand, easing development and maintenance.
+Finally, because values are injected into fields after the object has been constructed, they cannot be used to initialize other non-injected fields +inline.
This rule raises an issue when the @Autowired or @Inject annotations are used on a field.
Use constructor injection instead.
@@ -16,17 +18,21 @@
public class SomeService {
@Autowired
- private SomeDependency someDependency;
+ private SomeDependency someDependency; // Noncompliant
+
+ private String name = someDependency.getName(); // Will throw a NullPointerException
}
public class SomeService {
private final SomeDependency someDependency;
+ private final String name;
@Autowired
public SomeService(SomeDependency someDependency) {
this.someDependency = someDependency;
+ name = someDependency.getName();
}
}
diff --git a/java-checks/src/main/resources/org/sonar/l10n/java/rules/java/S6813.json b/java-checks/src/main/resources/org/sonar/l10n/java/rules/java/S6813.json
index 2bcf98f6b5a..1ca158fc7ba 100644
--- a/java-checks/src/main/resources/org/sonar/l10n/java/rules/java/S6813.json
+++ b/java-checks/src/main/resources/org/sonar/l10n/java/rules/java/S6813.json
@@ -1,5 +1,5 @@
{
- "title": "Avoid field dependency injection",
+ "title": "Field dependency injection should be avoided",
"type": "CODE_SMELL",
"status": "ready",
"remediation": {
diff --git a/java-checks/src/main/resources/org/sonar/l10n/java/rules/java/S6818.json b/java-checks/src/main/resources/org/sonar/l10n/java/rules/java/S6818.json
index 4f57b81bea5..640596b3f8a 100644
--- a/java-checks/src/main/resources/org/sonar/l10n/java/rules/java/S6818.json
+++ b/java-checks/src/main/resources/org/sonar/l10n/java/rules/java/S6818.json
@@ -1,5 +1,5 @@
{
- "title": "Avoid Using \"@Autowired\" on Multiple Constructors in a Spring Component",
+ "title": "\"@Autowired\" should only be used on a single constructor",
"type": "BUG",
"status": "ready",
"remediation": {
diff --git a/sonarpedia.json b/sonarpedia.json
index 09651911a16..7a72ecacce3 100644
--- a/sonarpedia.json
+++ b/sonarpedia.json
@@ -3,7 +3,7 @@
"languages": [
"JAVA"
],
- "latest-update": "2023-10-20T09:15:09.502876096Z",
+ "latest-update": "2023-11-07T15:40:45.973420Z",
"options": {
"no-language-in-filenames": true,
"preserve-filenames": false