Java - 3 - CodeAnt AI

This rule involves the use of Math.abs and negation on numbers that could be MIN_VALUE. It is a problem because it can lead to incorrect results and unexpected behavior in the program.

When Math.abs and negation are used on numbers that could be MIN_VALUE, the result can be incorrect due to integer overflow. Common methods that can return a MIN_VALUE and raise an issue when used together with Math.abs are:

Random.nextInt() and Random.nextLong()
hashCode()
compareTo()

Alternatively, the absExact() method throws an ArithmeticException for MIN_VALUE.

public void doSomething(String str) {
if (Math.abs(str.hashCode()) > 0) { // Noncompliant
// ...
}
}

By definition, primitive types are not Objects and so they can’t be `null. Adding @CheckForNull or @Nullable on primitive types adds confusion and is useless.

This rule raises an issue when @CheckForNull or @Nullable` is set on a method returning a primitive type: byte, short, int, long, float, double, boolean, char.

@CheckForNull
boolean isFoo() {
...
}

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 the Object 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 the hashCode() method to maintain the contract between equals() and hashCode().

class MyClass {
private int foo = 1;

public boolean equals(MyClass o) {  // Noncompliant; does not override Object.equals(Object)
return o != null && o.foo == this.foo;
}

public static void main(String[] args) {
MyClass o1 = new MyClass();
Object o2 = new MyClass();
System.out.println(o1.equals(o2));  // Prints "false" because o2 an Object not a MyClass
}
}

According to the documentation,

For example (credit to Brian Goetz), imagine Foo is a value-based class:

Foo[] arr = new Foo[2];
arr[0] = new Foo(0);
arr[1] = new Foo(0);

Serialization promises that on deserialization of arr, elements 0 and 1 will not be aliased. Similarly, in:

Foo[] arr = new Foo[2];
arr[0] = new Foo(0);
arr[1] = arr[0];

Serialization promises that on deserialization of `arr, elements 0 and 1 will be aliased.

While these promises are coincidentally fulfilled in current implementations of Java, that is not guaranteed in the future, particularly when true value types are introduced in the language.

This rule raises an issue when a Serializable class defines a non-transient, non-static field field whose type is a known serializable value-based class. Known serializable value-based classes are: all the classes in the java.time package except Clock; the date classes for alternate calendars: HijrahDate, JapaneseDate, MinguoDate, ThaiBuddhistDate`.

class MyClass implements Serializable {
private HijrahDate date;  // Noncompliant; mark this transient
// ...
}

”Boxing” is the process of putting a primitive value into a primitive-wrapper object. When that’s done purely to use the wrapper class’ toString method, it’s a waste of memory and cycles because those methods are static, and can therefore be used without a class instance. Similarly, using the static method valueOf in the primitive-wrapper classes with a non-String argument should be avoided.

int myInt = 4;
String myIntString = (new Integer(myInt)).toString(); // Noncompliant; creates & discards an Integer object
myIntString = Integer.valueOf(myInt).toString(); // Noncompliant

The `Class.isInstance method is the dynamic equivalent of the instanceof operator. According to the JavaDoc, isInstance

Thus, calling isInstance with a class argument is likely a mistake, since any random Class will only be “an instance of the represented class” when the left-hand side of the call is Class.class itself. To test for a class/sub-class relationship, use isAssignableFrom` instead.

Class<Number> num = Number.class;
Class<BigInteger> bi = BigInteger.class;

System.out.println(num.isInstance(bi));  // Noncompliant. false
System.out.println(bi.isInstance(Class.class));  // Noncompliant. false
System.out.println(Class.class.isInstance(bi));  // Noncompliant. true

The String class has a toString method because Object itself does. I.e. it couldn’t not have the method. But having the method doesn’t mean it should be used. In fact doing so is worse than pointless since it returns the String itself.

String str1 = "Now is the time for all good people";
String str2 = str1.toString();

Singletons that aren’t actually singletons become problems instead. To make sure a singleton isn’t instantiable, make sure all constructors are `private. If the singleton doesn’t have any constructors then add a private no-args constructor to override the default constructor.

This rule raises an issue when a class that holds a public static final instance of itself has non-private` constructors or no constructor.

public class Highlander implements Immortal {  // Noncompliant; no constructor; default, public constructor generated
public static final Highlander INSTANCE = new Highlander();

public void eliminateRival(Immortal immortal) {
// ...
}
}

public class Kurgan implements Immortal {
public static final Kurgan INSTANCE = new Kurgan();

Kurgan() {  // Noncompliant; should be private
}

public void eliminateRival(Immortal immortal) {
// ...
}
}

A key facet of the `equals contract is that if a.equals(b) then b.equals(a), i.e. that the relationship is symmetric.

Using instanceof breaks the contract when there are subclasses, because while the child is an instanceof the parent, the parent is not an instanceof the child. For instance, assume that Raspberry extends Fruit and adds some fields (requiring a new implementation of equals):

Fruit fruit = new Fruit();
Raspberry raspberry = new Raspberry();
if (raspberry instanceof Fruit) { … } // true
if (fruit instanceof Raspberry) { … } // false

If similar instanceof checks were used in the classes’ equals methods, the symmetry principle would be broken:

raspberry.equals(fruit); // false
fruit.equals(raspberry); //true

Additionally, non final classes shouldn’t use a hardcoded class name in the equals method because doing so breaks the method for subclasses. Instead, make the comparison dynamic.

Further, comparing to an unrelated class type breaks the contract for that unrelated type, because while thisClass.equals(unrelatedClass) can return true, unrelatedClass.equals(thisClass)` will not.

public class Fruit extends Food {
private Season ripe;

public boolean equals(Object obj) {
if (obj == this) {
  return true;
}
if (obj == null) {
  return false;
}
if (Fruit.class == obj.getClass()) { // Noncompliant; broken for child classes
  return ripe.equals(((Fruit)obj).getRipe());
}
if (obj instanceof Fruit ) {  // Noncompliant; broken for child classes
  return ripe.equals(((Fruit)obj).getRipe());
}
else if (obj instanceof Season) { // Noncompliant; symmetry broken for Season class
  // ...
}
//...

Map computeIfAbsent and computeIfPresent methods are convenient to avoid the cumbersome process to check if a key exists or not, followed by the addition of the entry. However, when the function used to compute the value returns `null, the entry key->null will not be added to the Map. Furthermore, in the case of computeIfPresent, if the key is present the entry will be removed. These methods should therefore not be used to conditionally add an entry with a null value. The traditional way should be used instead.

This rule raises an issue when computeIfAbsent or computeIfPresent` is used with a lambda always returning null.

map.computeIfAbsent(key, k -> null); // Noncompliant, the map will not contain an entry key->null.
map.computeIfPresent(key, (k, oldValue) -> null); // Noncompliant

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 than 8, as lambda expressions were introduced in Java 8.

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();
}
})
...

If the region is not specified when creating a new AwsClient with an AwsClientBuilder, the AWS SDK will execute some logic to identify the endpoint automatically.

While it will probably identify the correct one, this extra logic will slow down startup time, already known to be a hotspot.

You should therefore always define the logic to set the region yourself. This is typically done by retrieving the region from the Lambda provided AWS_REGION environment variable.

This will make the code more explicit and spare initialization time.

This rule reports an issue when the region is not set when creating an AwsClient.

S3Client.builder()
.credentialsProvider(EnvironmentVariableCredentialsProvider.create())
.build();

Because Object implements hashCode, any Java class can be put into a hash structure. However, classes that define equals(Object) but not hashCode() aren’t truly hash-able because instances that are equivalent according to the equals method can return different hashes.

public class Student {  // no hashCode() method; not hash-able
// ...

public boolean equals(Object o) {
// ...
}
}

public class School {
private Map<Student, Integer> studentBody = // okay so far
      new HashTable<Student, Integer>(); // Noncompliant

// ...

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.

import java.io.Serializable;

public class NonSerializableClass {
}

public class SerializableClass implements Serializable {
}

public class OtherSerializableClass extends SerializableClass {
// is also serializable because it is a subtype of Serializable
}

Failure to specify a locale when calling the methods `toLowerCase(), toUpperCase() or format() on String objects means the system default encoding will be used, possibly creating problems with international characters or number representations. For instance with the Turkish language, when converting the small letter ‘i’ to upper case, the result is capital letter ‘I’ with a dot over it.

Case conversion without a locale may work fine in its “home” environment, but break in ways that are extremely difficult to diagnose for customers who use different encodings. Such bugs can be nearly, if not completely, impossible to reproduce when it’s time to fix them. For locale-sensitive strings, the correct locale should always be used, but Locale.ROOT` can be used for case-insensitive ones.

myString.toLowerCase()

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 enum Color {
RED, GREEN, BLUE, ORANGE;
}

Map<Color, String> colorMap = new HashMap<>(); // Noncompliant

The implementation of certain `ResultSet methods is optional for result sets of type TYPE_FORWARD_ONLY. Even if your current JDBC driver does implement those methods, there’s no guarantee you won’t change drivers in the future.

This rule looks for invocations of the following methods on TYPE_FORWARD_ONLY ResultSets:

isBeforeFirst
isAfterLast
isFirst
getRow`

Statement stmt = con.createStatement(ResultSet.TYPE_FORWARD_ONLY);
stmt.executeQuery("SELECT name, address FROM PERSON");
ResultSet rs = stmt.getResultSet();

if (rs.isBeforeFirst()) { // Noncompliant

}

Return of boolean literal statements wrapped into `if-then-else ones should be simplified.

Similarly, method invocations wrapped into if-then-else` differing only from boolean literals should be simplified into a single invocation.

boolean foo(Object param) {
if (expression) { // Noncompliant
bar(param, true, "qix");
} else {
bar(param, false, "qix");
}

if (expression) {  // Noncompliant
return true;
} else {
return false;
}
}

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(){/*...*/}
}
class FirstChildClass extends ParentClass {
public boolean doSomething(){/*...*/}  // Noncompliant
}

Since an int is a 32-bit variable, shifting by more than +/-31 is confusing at best and an error at worst. When the runtime shifts 32-bit integers, it uses the lowest 5 bits of the shift count operand. In other words, shifting an int by 32 is the same as shifting it by 0, and shifting it by 33 is the same as shifting it by 1.

Similarly, when shifting 64-bit integers, the runtime uses the lowest 6 bits of the shift count operand and shifting long by 64 is the same as shifting it by 0, and shifting it by 65 is the same as shifting it by 1.

public int shift(int a) {
int x = a >> 32; // Noncompliant
return a << 48;  // Noncompliant
}

Whenever a virtual thread is started, the JVM will mount it on an OS thread. As soon as the virtual thread runs into a blocking operation like an HTTP request or a filesystem read/write operation, the JVM will detect this and unmount the virtual thread. This allows another virtual thread to take over the OS thread and continue its execution.

This is why virtual threads should be preferred to platform threads for tasks that involve blocking operations. By default, a Java thread is a platform thread. To use a virtual thread it must be started either with Thread.startVirtualThread(Runnable) or Thread.ofVirtual().start(Runnable).

This rule raises an issue when a platform thread is created with a task that includes heavy blocking operations.

new Thread(() -> {
try {
    Thread.sleep(1000); // Noncompliant blocking operation in platform thread
} catch (InterruptedException e) {
    throw new RuntimeException(e);
}
});

There is no reason to concatenate literal strings. Doing so is an exercise is reducing code readability. Instead, the strings should be combined. Similarly, literal strings should not be appended to a StringBuffer or StringBuilder sequentially, but combined into one call.

String message = "Hello " + "world" + "!";  // Noncompliant
StringBuilder sb = new StringBuilder();
sb.append("I'm pleased").append(" to meet you.");  //Noncompliant

In Spring, singleton beans and their dependencies are initialized when the application context is created.

If a Singleton bean depends on a bean with a shorter-lived scope (like Request or Session beans), it retains the same instance of that bean, even when new instances are created for each Request or Session. This mismatch can cause unexpected behavior and bugs, as the Singleton bean doesn’t interact correctly with the new instances of the shorter-lived bean.

This rule raises an issue when non-singleton beans are injected into a singleton bean.

@Component
@Scope(value = WebApplicationContext.SCOPE_REQUEST, proxyMode = ScopedProxyMode.TARGET_CLASS)
public class RequestBean {
//...
}

public class SingletonBean {
@Autowired
private final RequestBean requestBean; // Noncompliant, the same instance of RequestBean is used for each HTTP request.

public RequestBean getRequestBean() {
    return requestBean;
}
}

Java 21 introduces the new Sequenced Collections API, which is applicable to all collections with a defined sequence on their elements, such as LinkedList, TreeSet, and others (see JEP 431). For projects using Java 21 and onwards, this API should be utilized instead of workaround implementations that were necessary before Java 21.

This rule reports when a collection is iterated in reverse through explicit implementation or workarounds, instead of using the reversed view of the collection.

void printLastToFirst(List<String> list) {
for (var it = list.listIterator(list.size()); it.hasPrevious();) {
var element = it.previous();
System.out.println(element);
}
}

The purpose of checked exceptions is to ensure that errors will be dealt with, either by propagating them or by handling them, but some believe that checked exceptions negatively impact the readability of source code, by spreading this error handling/propagation logic everywhere.

This rule verifies that no method throws a new checked exception.

public void myMethod1() throws CheckedException {
...
throw new CheckedException(message);   // Noncompliant
...
throw new IllegalArgumentException(message); // Compliant; IllegalArgumentException is unchecked
}   

public void myMethod2() throws CheckedException {  // Compliant; propagation allowed
myMethod1();
}

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..

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());
}
}

Deprecated method should be avoided, rather than overridden. Deprecation is a warning that the method has been superseded, and will eventually be removed. The deprecation period allows you to make a smooth transition away from the aging, soon-to-be-retired technology.

class A {
@Deprecated
void foo(){}
}

class B extends A {
@Override
void foo(){  // Noncompliant
}
}

In Java 16, the feature “Pattern matching for instanceof” is finalized and can be used in production. Previously developers needed to do 3 operations in order to do this: check the variable type, cast it and assign the casted value to the new variable. This approach is quite verbose and can be replaced with pattern matching for `instanceof, doing these 3 actions (check, cast and assign) in one expression.

This rule raises an issue when an instanceof` check followed by a cast and an assignment could be replaced by pattern matching.

int f(Object o) {
if (o instanceof String) {  // Noncompliant
String string = (String) o;
return string.length();
}
return 0;
}

Making a `public constant just final as opposed to static final leads to duplicating its value for every instance of the class, uselessly increasing the amount of memory required to execute the application.

Further, when a non-public, final field isn’t also static, it implies that different instances can have different values. However, initializing a non-static final field in its declaration forces every instance to have the same value. So such fields should either be made static` or initialized in the constructor.

public class Myclass {
public final int THRESHOLD = 3;   
}

Strings are immutable objects, so concatenation doesn’t simply add the new String to the end of the existing string. Instead, in each loop iteration, the first String is converted to an intermediate object type, the second string is appended, and then the intermediate object is converted back to a String. Further, performance of these intermediate operations degrades as the String gets longer. Therefore, the use of StringBuilder is preferred.

String str = "";
for (int i = 0; i < arrayOfStrings.length ; ++i) {
str = str + arrayOfStrings[i];
}

Using the standard getClassLoader() may not return the right class loader in a JEE context. Instead, go through the currentThread.

ClassLoader cl = this.getClass().getClassLoader();  // Noncompliant

A generic type is a generic class or interface that is parameterized over types. For example, java.util.List has one type parameter: the type of its elements.

When generic types are used raw (without type parameters), the compiler is not able to do generic type checking. For this reason, it is sometimes necessary to cast objects and defer type-checking to runtime.

List integers = new ArrayList<>();

// It is possible to add a string to a list that is supposed to be integers only
integers.add("Hello World!");

Integer a = (Integer) integers.get(0);  // ClassCastException!

In a multithreaded environment, a thread may need to wait for a particular condition to become true. One way of pausing execution in Java is Thread.sleep(…).

If a thread that holds a lock calls Thread.sleep(…), no other thread can acquire said lock. This can lead to performance and scalability issues, in the worst case leading to deadlocks.

public void doSomething(){
synchronized(monitor) {
while(notReady()){
  Thread.sleep(200); // Noncompliant, any other thread synchronizing on monitor is blocked from running while the first thread sleeps.
}
process();
}
...
}

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.

class MyClass {

public void doSomethingCommon() {
  Random random = new Random();        // Noncompliant - new instance created with each invocation
  int rValue = random.nextInt();
}
}

As mentioned in JUnit5 documentation, it is possible to integrate JUnit4 with JUnit5:

However, maintaining both systems is a temporary solution. This rule flags all the annotations from JUnit4 which would need to be migrated to JUnit5, hence helping migration of a project.

Here is the list of JUnit4 annotations tracked by the rule, with their corresponding annotations in JUnit5:

JUnit4	JUnit5
`org.junit.Test	org.junit.jupiter.api.Test
org.junit.Before	org.junit.jupiter.api.BeforeEach
org.junit.After	org.junit.jupiter.api.AfterEach
org.junit.BeforeClass	org.junit.jupiter.api.BeforeAll
org.junit.AfterClass	org.junit.jupiter.api.AfterAll
org.junit.Ignore	org.junit.jupiter.api.Disabled

JUnit4

JUnit5

`org.junit.Test

org.junit.jupiter.api.Test

org.junit.Before

org.junit.jupiter.api.BeforeEach

org.junit.After

org.junit.jupiter.api.AfterEach

org.junit.BeforeClass

org.junit.jupiter.api.BeforeAll

org.junit.AfterClass

org.junit.jupiter.api.AfterAll

org.junit.Ignore

org.junit.jupiter.api.Disabled

Note that the following annotations might requires some rework of the tests to have JUnit5 equivalent behavior. A simple replacement of the annotation won’t work immediately:

JUnit4	JUnit5
org.junit.experimental.categories.Category	org.junit.jupiter.api.Tag
org.junit.Rule	org.junit.jupiter.api.extension.ExtendWith
org.junit.ClassRule	org.junit.jupiter.api.extension.RegisterExtension
org.junit.runner.RunWith	org.junit.jupiter.api.extension.ExtendWith`

JUnit4

JUnit5

org.junit.experimental.categories.Category

org.junit.jupiter.api.Tag

org.junit.Rule

org.junit.jupiter.api.extension.ExtendWith

org.junit.ClassRule

org.junit.jupiter.api.extension.RegisterExtension

org.junit.runner.RunWith

org.junit.jupiter.api.extension.ExtendWith`

package org.foo;

import org.junit.After;
import org.junit.AfterClass;
import org.junit.Before;
import org.junit.BeforeClass;
import org.junit.Ignore;
import org.junit.Test;
import org.junit.experimental.categories.Category;
import org.junit.runner.RunWith;

@RunWith(MyJUnit4Runner.class)
public class MyJUnit4Test {

@BeforeClass
public static void beforeAll() {
System.out.println("beforeAll");
}

@AfterClass
public static void afterAll() {
System.out.println("AfterAll");
}

@Before
public void beforeEach() {
System.out.println("beforeEach");
}

@After
public void afterEach() {
System.out.println("afterEach");
}

@Test
public void test1() throws Exception {
System.out.println("test1");
}

public interface SomeTests { /* category marker */ }

@Test
@Category(SomeTests.class)
public void test2() throws Exception {
System.out.println("test2");
}

@Test
@Ignore("Requires fix of #42")
public void ignored() throws Exception {
System.out.println("ignored");
}
}

Field injection seems like a tidy way to get your classes what they need to do their jobs, but it’s really a `NullPointerException waiting to happen unless all your class constructors are private. That’s because any class instances that are constructed by callers, rather than instantiated by a Dependency Injection framework compliant with the JSR-330 (Spring, Guice, …), won’t have the ability to perform the field injection.

Instead @Inject should be moved to the constructor and the fields required as constructor parameters.

This rule raises an issue when classes with non-private` constructors (including the default constructor) use field injection.

class MyComponent {  // Anyone can call the default constructor 

@Inject MyCollaborator collaborator;  // Noncompliant

public void myBusinessMethod() {
collaborator.doSomething();  // this will fail in classes new-ed by a caller
}
}

Either use only spaces or only tabs for the indentation of a text block. Mixing white space will lead to a result with irregular indentation.

String textBlock = """
    this is
<tab>text block!
    !!!!
  """;

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.

Note that this rule is automatically disabled when the project’s sonar.java.source is lower than 8.

public abstract class Car {
public abstract void start(Environment c);

public void stop(Environment c) {
c.freeze(this);
}
}

Rather than creating a boxed primitive from a String to extract the primitive value, use the relevant parse method instead. Using parse makes the code more efficient and the intent of the developer clearer.

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(); // Noncompliant

In general, altering or bypassing the accessibility of classes, methods, or fields violates the encapsulation principle and could lead to runtime errors. For records the case is even trickier: you cannot change the visibility of records’s fields and trying to update the existing value will lead to IllegalAccessException at runtime.

This rule raises an issue when reflection is used to change the visibility of a record’s field, and when it is used to directly update a record’s field value.

record Person(String name, int age) {}

Person person = new Person("A", 26);
Field field = Person.class.getDeclaredField("name");
field.setAccessible(true); // secondary
field.set(person, "B"); // Noncompliant

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.

class Foo {

final int a;

@Override
public boolean equals(Object other) {
if (other == null) return false;
if (getClass() != other.getClass()) return false;
return a == ((Foo) other).a;
}
}

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.

public int hashcode() { /* ... */ }             // Noncompliant

public String tostring() { /* ... */ }          // Noncompliant

public boolean equal(Object obj) { /* ... */ }  // Noncompliant

Dependency injection frameworks such as Spring support dependency injection by using annotations such as @Inject and @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.

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.

public class SomeService {
@Autowired
private SomeDependency someDependency; // Noncompliant

private String name = someDependency.getName(); // Will throw a NullPointerException
}

JUnit5 is more tolerant regarding the visibility of test classes and methods than JUnit4, which required everything to be public. Test classes and methods can have any visibility except private. It is however recommended to use the default package visibility to improve readability.

— JUnit5 User Guide

import org.junit.jupiter.api.Test;

public class MyClassTest { // Noncompliant - modifier can be removed
@Test
protected void test() { // Noncompliant - modifier can be removed
// ...
}
}

A method annotated with Spring’s @Async or @Transactional annotations will not work as expected if invoked directly from within its class.

This is because Spring generates a proxy class with wrapper code to manage the method’s asynchronicity (@Async) or to handle the transaction (@Transactional). However, when called using this, the proxy instance is bypassed, and the method is invoked directly without the required wrapper code.

@Service
public class AsyncNotificationProcessor implements NotificationProcessor {

@Override
public void process(Notification notification) {
processAsync(notification); // Noncompliant, call bypasses proxy
}

@Async
public processAsync(Notification notification) {
// ...
}
}

An infinite loop will never end while the program runs, meaning you have to kill the program to get out of the loop. Every loop should have an end condition, whether by meeting the loop’s termination condition or via a break statement.

for (;;) {  // Noncompliant; end condition omitted
// ...
}

In Spring Framework, the @Qualifier annotation is typically used to disambiguate between multiple beans of the same type when auto-wiring dependencies. It is not necessary to use @Qualifier when defining a bean using the @Bean annotation because the bean’s name can be explicitly specified using the name attribute or derived from the method name. Using @Qualifier on @Bean methods can lead to confusion and redundancy. Beans should be named appropriately using either the name attribute of the @Bean annotation or the method name itself.

@Configuration
public class MyConfiguration {
@Bean
@Qualifier("myService")
public MyService myService() {
// ...
return new MyService();
}

@Bean
@Qualifier("betterService")
public MyService aBetterService() {
// ...
return new MyService();
}

@Bean
@Qualifier("evenBetterService")
public MyService anEvenBetterService() {
// ...
return new MyService();
}

@Bean
@Qualifier("differentService")
public MyBean aDifferentService() {
// ...
return new MyBean();
}
}

Maps use hashes of the keys to select a bucket to store data in. Objects that hash to the same value will be added to the same bucket.

When the hashing function has a poor distribution, buckets can grow to very large sizes. This may negatively affect lookup performance, as, by default, matching a key within a bucket has linear complexity.

In addition, as the default hashCode function can be selected at runtime, performance expectations cannot be maintained.

Implementing Comparable mitigates the performance issue for objects that hash to the same value.

class MyKeyType {
// ...
}

class Program {
Map<MyKeyType, MyValueType> data = new HashMap<>(); // Noncompliant

Map<MyKeyType, MyValueType> buildMap() { // Noncompliant
    //...
}
}

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() {
try {
anObject.notify();
} catch(IllegalMonitorStateException e) { // Noncompliant
}
}

Tests should always:

Make sure that production code behaves as expected, including edge cases.
Be easy to debug, i.e. understandable and reproducible.

Using random values in tests will not necessarily check edge cases, and it will make test logs a lot harder to read. It is better to use easily readable hardcoded values. If this makes your code bigger you can use helper functions.

There is one valid use case for random data in tests: when testing every value would make tests impractically slow. In this case the best you can do is use random to test every value on the long run. You should however make sure that random values are logged so that you can reproduce failures. Some libraries exist to make all this easier. You can for example use property-based testing libraries such as jqwik.

This rule raises an issue when new Random() or UUID.randomUUID() are called in test code.

int userAge = new Random().nextInt(42);  // Noncompliant
UUID userID = UUID.randomUUID(); // Noncompliant

Double-checked locking is the practice of checking a lazy-initialized object’s state both before and after a synchronized block is entered to determine whether to initialize the object. In early JVM versions, synchronizing entire methods was not performant, which sometimes caused this practice to be used in its place.

Apart from float and int types, this practice does not work reliably in a platform-independent manner without additional synchronization of mutable instances. Using double-checked locking for the lazy initialization of any other type of primitive or mutable object risks a second thread using an uninitialized or partially initialized member while the first thread is still creating it. The results can be unexpected, potentially even causing the application to crash.

public class ResourceFactory {
private static Resource resource;

public static Resource getInstance() {
    if (resource == null) {
        synchronized (DoubleCheckedLocking.class) { // Noncompliant, not thread-safe due to the use of double-checked locking.
            if (resource == null)
                resource = new Resource();
        }
    }
    return resource;
}
}

To check the type of an object there are at least two options:

The simplest and shortest one with help of the `instanceof operator
The cumbersome and error-prone one with help of the Class.isAssignableFrom(…)` method

if (MyClass.class.isAssignableFrom(x.getClass())) { // Noncompliant 
MyClass mc = (MyClass)x; 
}

When a cycle exists between classes during their static initialization, the results can be unpredictable because they depend on which class was initialized first.

public class A {
public static int a = B.b + 1;  // Noncompliant; sometimes a = 1, others a = 2
}

public class B {
public static int b = A.a + 1;  // Noncompliant; sometimes b = 1, others b = 2
}

`transient fields are ignored by Java’s automatic serizalization mechanisms, which means that when a object is deserialized, those fields will be set to their default values.

transient fields that are referenced in multiple places in a class seem to play a significant role in that class. Allowing such significant fields to be left in a default state after deserialization may not be the best course, so they should probably be set in either a readObject() or readResolve()` method.

class Fruit implements Serializable {
private transient Seed seed;

public Fruit (Seed seed) {
this.seed = seed;  // 1st set
}
public void setSeed (Seed seed) {
this.seed = seed;  // 2nd set
}
public Seed getSeed() {
return seed; // not counted; read, not write.
}

public void sprout () {
this.seed = new GerminatedSeed();  // Noncompliant; 3rd write
//...
}
}

A synchronized method is a method marked with the synchronized keyword, meaning it can only be accessed by one thread at a time. If multiple threads try to access the synchronized method simultaneously, they will be blocked until the method is available.

Synchronized methods prevent race conditions and data inconsistencies in multi-threaded environments. Ensuring that only one thread can access a method at a time, prevents multiple threads from modifying the same data simultaneously, and causing conflicts.

When one part of a getter/setter pair is synchronized the other should be too. Failure to synchronize both sides may result in inconsistent behavior at runtime as callers access an inconsistent method state.

This rule raises an issue when either the method or the contents of one method in a getter/setter pair are synchronized, but the other is not.

public class Person {
String name;
int age; 

public synchronized void setName(String name) {
this.name = name;
}

public String getName() {  // Noncompliant
return this.name;
}

public void setAge(int age) {  // Noncompliant
this.age = age;
}

public int getAge() {
synchronized (this) {
  return this.age;
}
}
}

In the past, it was required to load a JDBC driver before creating a `java.sql.Connection. Nowadays, when using JDBC 4.0 drivers, this is no longer required and Class.forName() can be safely removed because JDBC 4.0 (JDK 6) drivers available in the classpath are automatically loaded.

This rule raises an issue when Class.forName() is used with one of the following values:

com.mysql.jdbc.Driver
oracle.jdbc.driver.OracleDriver
com.ibm.db2.jdbc.app.DB2Driver
com.ibm.db2.jdbc.net.DB2Driver
com.sybase.jdbc.SybDriver
com.sybase.jdbc2.jdbc.SybDriver
com.teradata.jdbc.TeraDriver
com.microsoft.sqlserver.jdbc.SQLServerDriver
org.postgresql.Driver
sun.jdbc.odbc.JdbcOdbcDriver
org.hsqldb.jdbc.JDBCDriver
org.h2.Driver
org.firebirdsql.jdbc.FBDriver
net.sourceforge.jtds.jdbc.Driver
com.ibm.db2.jcc.DB2Driver`

import java.sql.Connection;
import java.sql.DriverManager;
import java.sql.SQLException;

public class Demo {
private static final String DRIVER_CLASS_NAME = "org.postgresql.Driver";
private final Connection connection;

public Demo(String serverURI) throws SQLException, ClassNotFoundException {
Class.forName(DRIVER_CLASS_NAME); // Noncompliant; no longer required to load the JDBC Driver using Class.forName()
connection = DriverManager.getConnection(serverURI);
}
}

The Spring framework comes with dedicated classes to help writing better and simpler unit tests. In particular, when testing applications built on top of Spring MVC, it is recommended to use Spring’s `ModelAndViewAssert assertions class, instead of manually testing MVC’s properties.

This rule raises an issue when Spring’s ModelAndViewAssert` assertions should be used instead of manual testing.

ModelAndView mav = getMyModelAndView();

Assert.assertEquals("register", mav.getViewName());
Assert.assertTrue((Boolean) mav.getModelMap().get("myAttribute"));
Assert.assertFalse((Boolean) mav.getModelMap().get("myAttribute"));
Assert.assertEquals(myObject, mav.getModelMap().get("myAttribute"));

There is no reason to have a `main method in a web application. It may have been useful for debugging during application development, but such a method should never make it into production. Having a main method in a web application opens a door to the application logic that an attacker may never be able to reach (but watch out if one does!), but it is a sloppy practice and indicates that other problems may be present.

This rule raises an issue when a main` method is found in a servlet or an EJB.

public class MyServlet extends HttpServlet {
public void doGet(HttpServletRequest req, HttpServletResponse res) throws ServletException, IOException {
if (userIsAuthorized(req)) {
  updatePrices(req);
}
}

public static void main(String[] args) { // Noncompliant
updatePrices(req);
}
}

By default, Hibernate session flushing is set to FlushMode.AUTO, and is called from Transaction.commit, Session.flush and before some queries are executed. Setting it to FlushMode.COMMIT, FlushMode.NEVER, or FlushMode.MANUAL could mean that parts of your application get stale data, so you should be sure of what you’re doing before you use any of these modes.

This rule raises an issue when flush mode is explicitly set to any of these modes.

Session session = sessionFactory.openSession();
session.setFlushMode(FlushMode.NEVER);  // Noncompliant

While this can be useful, whenever we want to instantiate and start an unnamed virtual thread, there is a more convenient static method to do so: Thread.startVirtualThread(Runnable task)

This rule raises an issue every time the form Thread.ofVirtual().start(task); is found.

Thread virtualThread = Thread.ofVirtual().start(task); // Noncompliant `Thread.startVirtualThread` should be used instead

Since Java 9, `@Deprecated has two additional arguments to the annotation:

since allows you to describe when the deprecation took place
forRemoval, indicates whether the deprecated element will be removed at some future date

In order to ease the maintainers work, it is recommended to always add one or both of these arguments.

This rule reports an issue when @Deprecated` is used without any argument.

@Deprecated

rm is security-sensitive. For example, their use has led in the past to the following vulnerability:

CVE-2014-0114

All classes extending org.apache.struts.action.Action are potentially remotely reachable. The ActionForm object provided as a parameter of the execute method is automatically instantiated and populated with the HTTP parameters. One should review the use of these parameters to be sure they are used safely.

// Struts 1.1+
public final class CashTransferAction extends Action {

public String fromAccount = ""; 
public String toAccount = "";

public ActionForward execute(ActionMapping mapping, ActionForm form, HttpServletRequest req, HttpServletResponse res) throws Exception {
// usage of the "form" object to call some services doing JDBC actions
[...]
return mapping.findForward(resultat);
}
}

The array brackets ([]) for methods that return arrays may appear either immediately after the array type or after the list of parameters. Both styles will compile, but placing array brackets at the end of the method signature is deprecated, and retained in the language specification only for backward compatibility.

Additionally, placing the array brackets at the end is far less readable than keeping the brackets with the return type. Therefore, this style should be found only in legacy code, never in new code.

String sayHello() [] {    // Noncompliant
return new String[] {"hello", "world"};
}

The hasNext method of an Iterator should only report on the state of the iterator, not change it. Making a change to an iterator in its hasNext method violates all expectations of what the method will do, and almost guarantees bad results when the iterator class is used.

public class MyItr implements Iterator<MyClass> {
//...

public boolean hasNext() {
if (next() != null) {  // Noncompliant
  return true;
}
return false;
}

xtract strings from an application source code or binary, passwords 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:

Passwords should be stored outside of the code in a configuration file, a database, or a password management service.

This rule flags instances of hard-coded passwords used in database and LDAP connections. It looks for hard-coded passwords in connection strings, and for variable names that match any of the patterns from the provided list.

String username = "steve";
String password = "blue";
Connection conn = DriverManager.getConnection("jdbc:mysql://localhost/test?" +
              "user=" + username + "&password=" + password); // Sensitive

Because Double Brace Initialization (DBI) creates an anonymous class with a reference to the instance of the owning object, its use can lead to memory leaks if the anonymous inner class is returned and held by other objects. Even when there’s no leak, DBI is so obscure that it’s bound to confuse most maintainers.

For collections, use Arrays.asList instead, or explicitly add each item directly to the collection.

Map source = new HashMap(){{ // Noncompliant
put("firstName", "John");
put("lastName", "Smith");
}};

In records, the default behavior of the `equals() method is to check the equality by field values. This works well for primitive fields or fields, whose type overrides equals(), but this behavior doesn’t work as expected for array fields.

By default, array fields are compared by their reference, and overriding equals() is highly appreciated to achieve the deep equality check. The same strategy applies to hashCode() and toString() methods.

This rule reports an issue if a record class has an array field and is not overriding equals(), hashCode() or toString()` methods.

record Person(String[] names, int age) {} // Noncompliant

According to Hibernate’s documentation:

This rule raises an issue when a hibernate.connection.pool_size value is found in a hibernate.cfg.xml or hibernate.properties file.

<hibernate-configuration>
<session-factory>
<!-- ... -->
<property name="connection.pool_size">10</property>  <!-- Noncompliant -->
</session-factory>
</hibernate-configuration>

Nested `switch structures are difficult to understand because you can easily confuse the cases of an inner switch as belonging to an outer statement or expression. Therefore nested switch statements and expressions should be avoided.

Specifically, you should structure your code to avoid the need for nested switch statements or expressions, but if you cannot, then consider moving the inner switch` to another method.

void foo(int n, int m) {
switch (n) { 
case 0:
  switch (m) {  // Noncompliant; nested switch
    // ...
  }
case 1:
  // ...
default:
  // ...
}
}

Invoking other Lambdas synchronously from a Lambda is a scalability anti-pattern. Lambdas have a maximum execution time before they timeout (15 minutes as of May 2021). Having to wait for another Lambda to finish its execution could lead to a timeout.

A better solution is to generate events that can be consumed asynchronously by other Lambdas.

InvokeRequest invokeRequest = new InvokeRequest()
     .withFunctionName("myFunction");

AWSLambda awsLambda = AWSLambdaClientBuilder.standard()
	.withCredentials(new ProfileCredentialsProvider())
	.withRegion(Regions.US_WEST_2).build();

awsLambda.invoke(invokeRequest); // Noncompliant

Non-static initializers, also known as instance initializers, are blocks of code within a class that are 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.

class MyClass {
private static final Map<String, String> MY_MAP = new HashMap<String, String>() {
{
  put("a", "b");
}
}; // Noncompliant - HashMap should be extended only to add behavior, not for initialization
}

The counter of a for loop should be updated in the loop’s increment clause. The purpose of a for loop is to iterate over a range using a counter variable. It should not be used for other purposes, and alternative loops should be used in those cases.

If the counter is not updated, the loop will be infinite with a constant counter variable. If this is intentional, use a while or do while loop instead of a for loop.

If the counter variable is updated within the loop’s body, try to move it to the increment clause. If this is impossible due to certain conditions, replace the for loop with a while or do while loop.

for (int i = 0; i < 10; ) { // Noncompliant, i not updated in increment clause
// ...
i++;
}

Many resources in Java need be closed after they have been used. If they are not, the garbage collector cannot reclaim the resources’ memory, and they are still considered to be in use by the operating system. Such resources are considered to be leaked, which can lead to performance issues.

Java 7 introduced the try-with-resources statement, which guarantees that the resource in question will be closed.

try (InputStream input = Files.newInputStream(path)) {
// "input" will be closed after the execution of this block
}

When a test fails due, for example, to infrastructure issues, you might want to ignore it temporarily. But without some kind of notation about why the test is being ignored, it may never be reactivated. Such tests are difficult to address without comprehensive knowledge of the project, and end up polluting their projects.

This rule raises an issue for each ignored test that does not have any comment about why it is being skipped.

For Junit4, this rule targets the @Ignore annotation.
For Junit5, this rule targets the @Disabled annotation.
Cases where assumeTrue(false) or assumeFalse(true) are used to skip tests are targeted as well.

@Ignore  // Noncompliant
@Test
public void testDoTheThing() { 
// ...

JUnit5 is more tolerant regarding the visibilities of Test classes and methods than JUnit4, which required everything to be public. JUnit5 supports default package, public and protected visibility, even if it is recommended to use the default package visibility, which improves the readability of code.

But JUnit5 ignores without any warning:

private classes and private methods
static methods
methods returning a value without being a TestFactory

import org.junit.jupiter.api.Test;

class MyClassTest {
@Test
private void test1() { // Noncompliant - ignored by JUnit5
// ...
}
@Test
static void test2() { // Noncompliant - ignored by JUnit5
// ...
}
@Test
boolean test3() { // Noncompliant - ignored by JUnit5
// ...
}
@Nested
private class MyNestedClass { // Noncompliant - ignored by JUnit5
@Test
void test() {
  // ...
}
}
}

A test case without assertions ensures only that no exceptions are thrown. Beyond basic runnability, it ensures nothing about the behavior of the code under test.

This rule raises an exception when no assertions from any of the following known frameworks are found in a test:

AssertJ
Awaitility
EasyMock
Eclipse Vert.x
Fest 1.x and 2.x
Hamcrest
JMock
JMockit
JUnit
Mockito
Rest-assured 2.x, 3.x and 4.x
RxJava 1.x and 2.x
Selenide
Spring’s `org.springframework.test.web.servlet.ResultActions.andExpect() and org.springframework.test.web.servlet.ResultActions.andExpectAll()
Truth Framework
WireMock

Furthermore, as new or custom assertion frameworks may be used, the rule can be parametrized to define specific methods that will also be considered as assertions. No issue will be raised when such methods are found in test cases. The parameter value should have the following format <FullyQualifiedClassName>#<MethodName>, where MethodName can end with the wildcard character. For constructors, the pattern should be <FullyQualifiedClassName>#<init>.

Example: com.company.CompareToTester#compare*,com.company.CustomAssert#customAssertMethod,com.company.CheckVerifier#<init>`.

@Test 
public void testDoSomething() {  // Noncompliant
MyClass myClass = new MyClass();
myClass.doSomething();
}

Non-static inner classes contain a reference to an instance of the outer class. Hence, serializing a non-static inner class will result in an attempt at serializing the outer class as well. If the outer class is not serializable, serialization will fail, resulting in a runtime error.

Making the inner class static (i.e., “nested”) avoids this problem, as no reference to an instance of the outer class is required. Serializing the inner class can be done independently of the outer class. Hence, inner classes implementing Serializable should be static if the outer class does not implement Serializable.

Be aware of the semantic differences between an inner class and a nested one:

an inner class can only be instantiated within the context of an instance of the outer class.
a nested (static) class can be instantiated independently of the outer class.

public class Pomegranate {
// ...

public class Seed implements Serializable {  // Noncompliant, serialization will fail due to the outer class not being serializable
// ...
}
}

The java.util.Collection type and its subtypes provide methods to access and modify collections such as Collection.remove(Object o) and Collection.contains(Object o). Some of these methods accept arguments of type java.lang.Object and will compare said argument with objects already in the collection.

If the actual type of the argument is unrelated to the type of object contained in the collection, these methods will always return false, null, or -1. This behavior is most likely unintended and can be indicative of a design issue.

This rule raises an issue when the type of the argument provided to one of the following methods is unrelated to the type used for the collection declaration:

Collection.remove(Object o)
Collection.removeAll(Collection<?>)
Collection.contains(Object o)
List.indexOf(Object o)
List.lastIndexOf(Object o)
Map.containsKey(Object key)
Map.containsValue(Object value)
Map.get(Object key)
Map.getOrDefault(Object key, V defaultValue)
Map.remove(Object key)
Map.remove(Object key, Object value)

void removeFromMap(Map<Integer, Object> map, String strKey) {
map.remove(strKey); // Noncompliant, this call will remove nothing and always return 'null' because 'map' is handling only Integer keys and String cannot be cast to Integer.
}

void listContains(List<String> list, Integer integer) {
if (list.contains(integer)) { // Noncompliant; always false as the list only contains Strings, not integers.
// ...
}
}

When implementing the `Comparable<T>.compareTo method, the parameter’s type has to match the type used in the Comparable declaration. When a different type is used this creates an overload instead of an override, which is unlikely to be the intent.

This rule raises an issue when the parameter of the compareTo method of a class implementing Comparable<T> is not same as the one used in the Comparable` declaration.

public class Foo {
static class Bar implements Comparable<Bar> {
public int compareTo(Bar rhs) {
  return -1;
}
}

static class FooBar extends Bar {
public int compareTo(FooBar rhs) {  // Noncompliant: Parameter should be of type Bar
  return 0;
}
}
}

The @Remote annotation indicates that an interface may be called from a remote client. Therefore the parameters and return types of methods in the interface must be Serializable.

public class Employee {  // Nonserializable
}

@Remote
public interface EmployeeServiceRemote {
public Employee getEmployee(String id);  // Noncompliant
}

Using `return, break, throw, and so on from a finally block suppresses the propagation of any unhandled Throwable which was thrown in the try or catch block.

This rule raises an issue when a jump statement (break, continue, return, throw, and goto) would force control flow to leave a finally` block.

public static void main(String[] args) {
try {
doSomethingWhichThrowsException();
System.out.println("OK");   // incorrect "OK" message is printed
} catch (RuntimeException e) {
System.out.println("ERROR");  // this message is not shown
}
}

public static void doSomethingWhichThrowsException() {
try {
throw new RuntimeException();
} finally {
for (int i = 0; i < 10; i ++) {
  //...
  if (q == i) {
    break; // ignored
  }
}

/* ... */
return;      // Noncompliant - prevents the RuntimeException from being propagated
}
}

If not annotated with `@Nested, an inner class containing some tests will never be executed during tests execution. While you could still be able to manually run its tests in an IDE, it won’t be the case during the build. By contrast, a static nested class containing some tests should not be annotated with @Nested, JUnit5 will not share setup and state with an instance of its enclosing class.

This rule raises an issue on inner classes and static nested classes containing JUnit5 test methods which has a wrong usage of @Nested` annotation.

Note: This rule does not check if the context in which JUnit 5 is running (e.g. Maven Surefire Plugin) is properly configured to execute static nested classes, it could not be the case using the default configuration.

import org.junit.jupiter.api.Test;

class MyJunit5Test {
@Test
void test() { /* ... */ }

class InnerClassTest { // Noncompliant, missing @Nested annotation
@Test
void test() { /* ... */ }
}

@Nested
static class StaticNestedClassTest { // Noncompliant, invalid usage of @Nested annotation
@Test
void test() { /* ... */ }
}
}

Comparing a variable to multiple cases is a frequent operation. This can be done using a sequence of if-else statements. However, for many cases like enums or simple value comparisons, a switch statement is the better alternative. With Java 21, the switch statement has been significantly improved to support pattern matching and record pattern.

Using a switch statement instead of an if-else chain provides benefits like clearer code, certainty of covering all cases, and may even improve performance.

This rule raises an issue when an if-else chain should be replaced by a switch statement.

sealed interface Expression {}
record Plus(Expression left, Expression right) implements Expression {}
record Minus(Expression left, Expression right) implements Expression {}
record Div(Expression left, Expression right) implements Expression {}

int eval(Expression expr){
if(expr instanceof Plus plus){ // Noncompliant; should be replaced by a switch expression
return eval(plus.left) + eval(plus.right);
}else if(expr instanceof Div div){
return eval(div.left) / eval(div.right);
}else if(expr instanceof Minus minus){
return eval(minus.left) - eval(minus.right);
} else {
throw new IllegalArgumentException("Unknown expression");
}
}

The Java Language Specification defines a set of rules called naming conventions that apply to Java programs. These conventions provide recommendations for naming packages, classes, methods, and variables.

By following shared naming conventions, teams can collaborate more efficiently.

This rule checks that static non-final field names match a provided regular expression.

public class MyClass {
private static String foo_bar; // Noncompliant
}

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 = 0x80003800;
Double.longBitsToDouble(i);   // Noncompliant - NaN

”@EnableAutoConfiguration” is a convenient feature to configure the Spring Application Context by attempting to guess the beans that you are likely to need. The drawback is that it may load and configure beans the application will never use and therefore consume more CPU and RAM than really required. `@EnableAutoConfiguration should be configured to exclude all the beans not required by the application. Alternatively, use the @Import annotation instead of @EnableAutoConfiguration, to explicitly import the useful AutoConfiguration classes.

This rule applies for @SpringBootApplication` as well.

@SpringBootApplication
public class MyApplication {
...
}

Overriding the `Object.finalize() method must be done with caution to dispose some system resources.

Calling the super.finalize()` at the end of this method implementation is highly recommended in case parent implementations must also dispose some system resources.

protected void finalize() {   // Noncompliant; no call to super.finalize();
releaseSomeResources();
}

protected void finalize() {
super.finalize();  // Noncompliant; this call should come last
releaseSomeResources();
}

If a lock is acquired and released within a method, then it must be released along all execution paths of that method.

Failing to do so will expose the conditional locking logic to the method’s callers and hence be deadlock-prone.

public class MyClass {
public void doSomething() {
Lock lock = new Lock();
lock.lock(); // Noncompliant
if (isInitialized()) {
  // ...
  lock.unlock();
}
}
}

Exposing HTTP endpoints is security-sensitive. It has led in the past to the following vulnerabilities:

HTTP endpoints are webservices’ main entrypoint. Attackers will take advantage of any vulnerability by sending crafted inputs for headers (including cookies), body and URI. No input should be trusted and extreme care should be taken with all returned value (header, body and status code).

This rule flags code which creates HTTP endpoint. It guides security code reviews to security-sensitive code.

@RequestMapping(path = "/profile", method = RequestMethod.GET) // Noncompliant
public UserProfile getUserProfile(String name) {
...
}

Overly complicated regular expressions are hard to read and to maintain and can easily cause hard-to-find bugs. If a regex is too complicated, you should consider replacing it or parts of it with regular code or splitting it apart into multiple patterns at least.

The complexity of a regular expression is determined as follows:

Each of the following operators increases the complexity by an amount equal to the current nesting level and also increases the current nesting level by one for its arguments:

`| - when multiple | operators are used together, the subsequent ones only increase the complexity by 1
&& (inside character classes) - when multiple && operators are used together, the subsequent ones only increase the complexity by 1
Quantifiers (*, +, ?, {n,m}, {n,} or {n})
Non-capturing groups that set flags (such as (?i:some_pattern) or (?i)some_pattern`)
Lookahead and lookbehind assertions

Additionally, each use of the following features increase the complexity by 1 regardless of nesting:

character classes
back references

If a regular expression is split among multiple variables, the complexity is calculated for each variable individually, not for the whole regular expression. If a regular expression is split over multiple lines, each line is treated individually if it is accompanied by a comment (either a Java comment or a comment within the regular expression), otherwise the regular expression is analyzed as a whole.

if (dateString.matches("^(?:(?:31(\\/|-|\\.)(?:0?[13578]|1[02]))\\1|(?:(?:29|30)(\\/|-|\\.)(?:0?[13-9]|1[0-2])\\2))(?:(?:1[6-9]|[2-9]\\d)?\\d{2})$|^(?:29(\\/|-|\\.)0?2\\3(?:(?:(?:1[6-9]|[2-9]\\d)?(?:0[48]|[2468][048]|[13579][26])|(?:(?:16|[2468][048]|[3579][26])00))))$|^(?:0?[1-9]|1\\d|2[0-8])(\\/|-|\\.)(?:(?:0?[1-9])|(?:1[0-2]))\\4(?:(?:1[6-9]|[2-9]\\d)?\\d{2})$")) {
handleDate(dateString);
}

Jump statements such as return and continue let you change the default flow of program execution, but jump statements that direct the control flow to the original direction are just a waste of keystrokes.

public void foo() {
while (condition1) {
if (condition2) {
  continue; // Noncompliant
} else {
  doTheThing();
}
}
return; // Noncompliant; this is a void method
}

Inappropriate casts are errors that will lead to bugs as the members are accessed. This includes casts from one unrelated type to another, as well as untested casts down an inheritance hierarchy.

public class S1944 {

public static void main(String[] args) {
List<String> list = (List<String>) getAttributes(); // Noncompliant; List<Integer> return by getAttributes() is not be casted to List<String> 
String s = list.get(0); // java.lang.ClassCastException will be raised here
}

private static List<?> getAttributes() {
List<Integer> result = new ArrayList<>();
result.add(0);
return result;
}

}

Storing data locally is a common task for mobile applications. Such data includes preferences or authentication tokens for external services, among other things. There are many convenient solutions that allow storing data persistently, for example SQLiteDatabase, SharedPreferences, and Realm. By default these systems store the data unencrypted, thus an attacker with physical access to the device can read them out easily. Access to sensitive data can be harmful for the user of the application, for example when the device gets stolen.

SQLiteDatabase db = SQLiteDatabase.openOrCreateDatabase("test.db", getKey(), null);

Regular expressions have their own syntax that is understood by regular expression engines. Those engines will throw an exception at runtime if they are given a regular expression that does not conform to that syntax.

To avoid syntax errors, special characters should be escaped with backslashes when they are intended to be matched literally and references to capturing groups should use the correctly spelled name or number of the group.

Pattern.compile("([");
str.matches("([");
str.replaceAll("([", "{");
str.matches("(\\w+-(\\d+)");

Operating systems have global directories where any user has write access. Those folders are mostly used as temporary storage areas like `/tmp in Linux based systems. An application manipulating files from these folders is exposed to race conditions on filenames: a malicious user can try to create a file with a predictable name before the application does. A successful attack can result in other files being accessed, modified, corrupted or deleted. This risk is even higher if the application runs with elevated permissions.

In the past, it has led to the following vulnerabilities:

This rule raises an issue whenever it detects a hard-coded path to a publicly writable directory like /tmp (see examples bellow). It also detects access to environment variables that point to publicly writable directories, e.g., TMP and TMPDIR.

/tmp
/var/tmp
/usr/tmp
/dev/shm
/dev/mqueue
/run/lock
/var/run/lock
/Library/Caches
/Users/Shared
/private/tmp
/private/var/tmp
\Windows\Temp
\Temp
\TMP`

new File("/myDirectory/myfile.txt");  // Compliant

File.createTempFile("prefix", "suffix", new File("/mySecureDirectory"));  // Compliant

if(SystemUtils.IS_OS_UNIX) {
FileAttribute<Set<PosixFilePermission>> attr = PosixFilePermissions.asFileAttribute(PosixFilePermissions.fromString("rwx------"));
Files.createTempFile("prefix", "suffix", attr); // Compliant
}
else {
File f = Files.createTempFile("prefix", "suffix").toFile();  // Compliant
f.setReadable(true, true);
f.setWritable(true, true);
f.setExecutable(true, true);
}

Shared naming conventions allow teams to collaborate efficiently. This rule raises an issue when a test class name does not match the provided regular expression.

class Foo {  // Noncompliant
@Test
void check() {  }
}

class Bar {  // Noncompliant
@Nested
class PositiveCase {
@Test
void check() {  }
}
}

WebViews can be used to display web content as part of a mobile application. A browser engine is used to render and display the content. Like a web application, a mobile application that uses WebViews can be vulnerable to Cross-Site Scripting if untrusted code is rendered.

If malicious JavaScript code in a WebView is executed this can leak the contents of sensitive files when access to local files is enabled.

import android.webkit.WebView;

WebView webView = (WebView) findViewById(R.id.webview);
webView.getSettings().setAllowFileAccess(true); // Sensitive
webView.getSettings().setAllowContentAccess(true); // Sensitive

Sub-patterns can be wrapped by parentheses to build a group. This enables to restrict alternations, back reference the group or apply quantifier to the sub-pattern.

If this group should not be part of the match result or if no reference to this group is required, a non-capturing group can be created by adding ?: behind the opening parenthesis.

However, if this non-capturing group does not have a quantifier, or does not wrap an alternation, then imaging this group is redundant.

"(?:number)\\d{2}"

Curly brace quantifiers in regular expressions can be used to have a more fine-grained control over how many times the character or the sub-expression preceeding them should occur. They can be used to match an expression exactly n times with `{n}, between n and m times with {n,m}, or at least n times with {n,}. In some cases, using such a quantifier is superfluous for the semantic of the regular expression, and it can be removed to improve readability. This rule raises an issue when one of the following quantifiers is encountered:

{1,1} or {1}: they match the expression exactly once. The same behavior can be achieved without the quantifier.
{0,0} or {0}`: they match the expression zero times. The same behavior can be achieved by removing the expression.

"ab{1,1}c"
"ab{1}c"
"ab{0,0}c"
"ab{0}c"

Most of the regular expression engines use backtracking to try all possible execution paths of the regular expression when evaluating an input, in some cases it can cause performance issues, called catastrophic backtracking situations. In the worst case, the complexity of the regular expression is exponential in the size of the input, this means that a small carefully-crafted input (like 20 chars) can trigger catastrophic backtracking and cause a denial of service of the application. Super-linear regex complexity can lead to the same impact too with, in this case, a large carefully-crafted input (thousands chars).

This rule determines the runtime complexity of a regular expression and informs you of the complexity if it is not linear.

Note that, due to improvements to the matching algorithm, some cases of exponential runtime complexity have become impossible when run using JDK 9 or later. In such cases, an issue will only be reported if the project’s target Java version is 8 or earlier.

java.util.regex.Pattern.compile("(a+)++").matcher(
"aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa"+
"aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa"+
"aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa"+
"aaaaaaaaaaaaaaa!").matches(); // Compliant

java.util.regex.Pattern.compile("(h|h|ih(((i|a|c|c|a|i|i|j|b|a|i|b|a|a|j))+h)ahbfhba|c|i)*+").matcher(
"hchcchicihcchciiicichhcichcihcchiihichiciiiihhcchi"+
"cchhcihchcihiihciichhccciccichcichiihcchcihhicchcciicchcccihiiihhihihihi"+
"chicihhcciccchihhhcchichchciihiicihciihcccciciccicciiiiiiiiicihhhiiiihchccch"+
"chhhhiiihchihcccchhhiiiiiiiicicichicihcciciihichhhhchihciiihhiccccccciciihh"+
"ichiccchhicchicihihccichicciihcichccihhiciccccccccichhhhihihhcchchihih"+
"iihhihihihicichihiiiihhhhihhhchhichiicihhiiiiihchccccchichci").matches(); // Compliant

The complexity of an expression is defined by the number of &&, || and condition ? ifTrue : ifFalse operators it contains.

A single expression’s complexity should not become too high to keep the code readable.

if (((condition1 && condition2) || (condition3 && condition4)) && condition5) { ... }

A cross-site request forgery (CSRF) attack occurs when a trusted user of a web application can be forced, by an attacker, to perform sensitive actions that he didn’t intend, such as updating his profile or sending a message, more generally anything that can change the state of the application.

The attacker can trick the user/victim to click on a link, corresponding to the privileged action, or to visit a malicious web site that embeds a hidden web request and as web browsers automatically include cookies, the actions can be authenticated and sensitive.

@EnableWebSecurity
public class WebSecurityConfig extends WebSecurityConfigurerAdapter {

@Override
protected void configure(HttpSecurity http) throws Exception {
// http.csrf().disable(); // Compliant
}
}

Type parameters that aren’t used are dead code, which can only distract and possibly confuse developers during maintenance. Therefore, unused type parameters should be removed.

int <T> Add(int a, int b) // Noncompliant; <T> is ignored
{
return a + b; 
}

When executing an OS command and unless you specify the full path to the executable, then the locations in your application’s PATH environment variable will be searched for the executable. That search could leave an opening for an attacker if one of the elements in PATH is a directory under his control.

Runtime.getRuntime().exec("/usr/bin/make");  // Compliant
Runtime.getRuntime().exec(new String[]{"~/bin/make"});  // Compliant

ProcessBuilder builder = new ProcessBuilder("./bin/make");  // Compliant
builder.command("../bin/make");  // Compliant
builder.command(Arrays.asList("..\bin\make", "-j8")); // Compliant

builder = new ProcessBuilder(Arrays.asList(".\make"));  // Compliant
builder.command(Arrays.asList("C:\bin\make", "-j8"));  // Compliant
builder.command(Arrays.asList("\\SERVER\bin\make"));  // Compliant

When arithmetic is performed on integers, the result will always be an integer. You can assign that result to a `long, double, or float with automatic type conversion, but having started as an int or long, the result will likely not be what you expect.

For instance, if the result of int division is assigned to a floating-point variable, precision will have been lost before the assignment. Likewise, if the result of multiplication is assigned to a long`, it may have already overflowed before the assignment.

In either case, the result will not be what was expected. Instead, at least one operand should be cast or promoted to the final type before the operation takes place.

float twoThirds = 2/3; // Noncompliant; int division. Yields 0.0
long millisInYear = 1_000*3_600*24*365; // Noncompliant; int multiplication. Yields 1471228928
long bigNum = Integer.MAX_VALUE + 2; // Noncompliant. Yields -2147483647
long bigNegNum =  Integer.MIN_VALUE-1; //Noncompliant, gives a positive result instead of a negative one. 
Date myDate = new Date(seconds * 1_000); //Noncompliant, won't produce the expected result if seconds > 2_147_483
...
public long compute(int factor){
return factor * 10_000;  //Noncompliant, won't produce the expected result if factor > 214_748
}

public float compute2(long factor){
return factor / 123;  //Noncompliant, will be rounded to closest long integer
}

Having a permissive Cross-Origin Resource Sharing policy is security-sensitive. It has led in the past to the following vulnerabilities:

Same origin policy in browsers prevents, by default and for security-reasons, a javascript frontend to perform a cross-origin HTTP request to a resource that has a different origin (domain, protocol, or port) from its own. The requested target can append additional HTTP headers in response, called CORS, that act like directives for the browser and change the access control policy / relax the same origin policy.

@Override
protected void doGet(HttpServletRequest req, HttpServletResponse resp) throws ServletException, IOException {
resp.setHeader("Content-Type", "text/plain; charset=utf-8");
resp.setHeader("Access-Control-Allow-Origin", "*"); // Sensitive
resp.setHeader("Access-Control-Allow-Credentials", "true"); 
resp.setHeader("Access-Control-Allow-Methods", "GET"); 
resp.getWriter().write("response");
}

The use of break and continue statements increases the complexity of the control flow and makes it harder to understand the program logic. In order to keep a good program structure, they should not be applied more than once per loop.

This rule reports an issue when there is more than one break or continue statement in a loop. The code should be refactored to increase readability if there is more than one.

for (int i = 1; i <= 10; i++) {     // Noncompliant; two "continue" statements
if (i % 2 == 0) {
continue;
}

if (i % 3 == 0) {
continue;
}
// ...
}

Character classes in regular expressions are a convenient way to match one of several possible characters by listing the allowed characters or ranges of characters. If a character class contains only one character, the effect is the same as just writing the character without a character class.

Thus, having only one character in a character class is usually a simple oversight that remained after removing other characters of the class.

"a[b]c"
"[\\^]"

Declaring a variable only to immediately return or throw it is considered a bad practice because it adds unnecessary complexity to the code. This practice can make the code harder to read and understand, as it introduces an extra step that doesn’t add any value. Instead of declaring a variable and then immediately returning or throwing it, it is generally better to return or throw the value directly. This makes the code cleaner, simpler, and easier to understand.

public long computeDurationInMilliseconds() {
long duration = (((hours * 60) + minutes) * 60 + seconds) * 1000;
return duration;
}

The use of a comparison operator outside of a boolean context is an error. At best it is meaningless code, and should be eliminated. However the far more likely scenario is that it is an assignment gone wrong, and should be corrected.

private void called(int foo) {
foo==1; // Noncompliant
if (foo==1) {
System.out.println("foo\n");
}
}

public void caller(String [ ] args {
called(2);
return 0;
}

Development tools and frameworks usually have options to make debugging easier for developers. Although these features are useful during development, they should never be enabled for applications deployed in production. Debug instructions or error messages can leak detailed information about the system, like the application’s path or file names.

try {
/* ... */
} catch(Exception e) {
e.printStackTrace(); // Sensitive 
}

In software development, logs serve as a record of events within an application, providing crucial insights for debugging. When logging, it is essential to ensure that the logs are:

easily accessible
uniformly formatted for readability
properly recorded
securely logged when dealing with sensitive data

Those requirements are not met if a program directly writes to the standard outputs (e.g., {language_std_outputs}). That is why defining and using a dedicated logger is highly recommended.

class MyClass {
public void doSomething() {
System.out.println("My Message");  // Noncompliant, output directly to System.out without a logger
}
}

Shadowing makes it impossible to use the type parameter from the outer scope. Also, it can be confusing to distinguish which type parameter is being used.

This rule raises an issue when a type parameter from an inner scope uses the same name as one in an outer scope.

public class TypeParameterHidesAnotherType<T> {

public class Inner<T> { // Noncompliant
  //...
}

private <T> T method() { // Noncompliant
  return null;
}

}

Magic numbers make the code more complex to understand as it requires the reader to have knowledge about the global context to understand the number itself. Their usage may seem obvious when writing the code, but it may not be the case for another developer or later once the context faded away. -1, 0, and 1 are not considered magic numbers.

public static void doSomething() {
for (int i = 0; i < 4; i++) {  // Noncompliant, 4 is a magic number
...
}
}

This rule is meant to be used as a way to track code which is marked as being deprecated. Deprecated code should eventually be removed.

class Foo {
/**
* @deprecated
*/
public void foo() {    // Noncompliant
}

@Deprecated            // Noncompliant
public void bar() {
}

public void baz() {    // Compliant
}
}

Integer literals starting with a zero are octal rather than decimal values. While using octal values is fully supported, most developers do not have experience with them. They may not recognize octal values as such, mistaking them instead for decimal values.

int myNumber = 010; // Noncompliant. myNumber will hold 8, not 10 - was this really expected?

Using pseudorandom number generators (PRNGs) is security-sensitive. For example, it has led in the past to the following vulnerabilities:

When software generates predictable values in a context requiring unpredictability, it may be possible for an attacker to guess the next value that will be generated, and use this guess to impersonate another user or access sensitive information.

SecureRandom random = new SecureRandom(); // Compliant for security-sensitive use cases
byte bytes[] = new byte[20];
random.nextBytes(bytes);

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:

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.

private static final String MY_SECRET = "47828a8dd77ee1eb9dde2d5e93cb221ce8c32b37";

public static void main(String[] args) {
MyClass.callMyService(MY_SECRET);
}

When exceptions occur, it is usually a bad idea to simply ignore them. Instead, it is better to handle them properly, or at least to log them.

public void doTheThing() {
try {
// ...
catch (IOException e) {  // Noncompliant
}
}

Whenever there are portions of code that are duplicated and do not depend on the state of their container class, they can be centralized inside a “utility class”. A utility class is a class that only has static members, hence it should not be instantiated.

class StringUtils { // Noncompliant

public static String concatenate(String s1, String s2) {
return s1 + s2;
}

}

Successful Zip Bomb attacks occur when an application expands untrusted archive files without controlling the size of the expanded data, which can lead to denial of service. A Zip bomb is usually a malicious archive file of a few kilobytes of compressed data but turned into gigabytes of uncompressed data. To achieve this extreme compression ratio, attackers will compress irrelevant data (eg: a long string of repeated bytes).

File f = new File("ZipBomb.zip");
ZipFile zipFile = new ZipFile(f);
Enumeration<? extends ZipEntry> entries = zipFile.entries();

int THRESHOLD_ENTRIES = 10000;        
int THRESHOLD_SIZE = 1000000000; // 1 GB
double THRESHOLD_RATIO = 10;         
int totalSizeArchive = 0;
int totalEntryArchive = 0;

while(entries.hasMoreElements()) { 
ZipEntry ze = entries.nextElement();
InputStream in = new BufferedInputStream(zipFile.getInputStream(ze)); 
OutputStream out = new BufferedOutputStream(new FileOutputStream("./output_onlyfortesting.txt"));

totalEntryArchive ++;

int nBytes = -1;
byte[] buffer = new byte[2048];
int totalSizeEntry = 0;

while((nBytes = in.read(buffer)) > 0) { // Compliant
  out.write(buffer, 0, nBytes);
  totalSizeEntry += nBytes;
  totalSizeArchive += nBytes; 

  double compressionRatio = totalSizeEntry / ze.getCompressedSize();
  if(compressionRatio > THRESHOLD_RATIO) {
    // ratio between compressed and uncompressed data is highly suspicious, looks like a Zip Bomb Attack
    break;
  }
}

if(totalSizeArchive > THRESHOLD_SIZE) {
  // the uncompressed data size is too much for the application resource capacity
  break;
}

if(totalEntryArchive > THRESHOLD_ENTRIES) {
  // too much entries in this archive, can lead to inodes exhaustion of the system
  break;
} 
}

An HTTP method is safe when used to perform a read-only operation, such as retrieving information. In contrast, an unsafe HTTP method is used to change the state of an application, for instance to update a user’s profile on a web application.

Common safe HTTP methods are GET, HEAD, or OPTIONS.

Common unsafe HTTP methods are POST, PUT and DELETE.

Allowing both safe and unsafe HTTP methods to perform a specific operation on a web application could impact its security, for example CSRF protections are most of the time only protecting operations performed by unsafe HTTP methods.

@RequestMapping("/delete_user", method = RequestMethod.POST)  // Compliant
public String delete1(String username) {
// state of the application will be changed here
}

@RequestMapping(path = "/delete_user", method = RequestMethod.POST) // Compliant
String delete2(@RequestParam("id") String id) {
// state of the application will be changed here
}

Nested code - blocks of code inside blocks of code - is eventually necessary, but increases complexity. This is why keeping the code as flat as possible, by avoiding unnecessary nesting, is considered a good practice.

Merging if statements when possible will decrease the nesting of the code and improve its readability.

if (condition1) {
if (condition2) {             // Noncompliant
/* ... */
}
}

An empty {operationName} is generally considered bad practice and can lead to confusion, readability, and maintenance issues. Empty {operationName}s bring no functionality and are misleading to others as they might think the {operationName} implementation fulfills a specific and identified requirement.

There are several reasons for a {operationName} not to have a body:

It is an unintentional omission, and should be fixed to prevent an unexpected behavior in production.
It is not yet, or never will be, supported. In this case an exception should be thrown.
The method is an intentionally-blank override. In this case a nested comment should explain the reason for the blank override.

public abstract class Animal {
void speak() {  // default implementation ignored
}
}

One way to test for empty lines is to use the regex `”^$”, which can be extremely handy when filtering out empty lines from collections of Strings, for instance. With regard to this, the Javadoc for Pattern (Line Terminators) states the following:

As emphasized, ^ is not going to match at the end of an input, and the end of the input is necessarily included in the empty string, which might lead to completely missing empty lines, while it would be the initial reason for using such regex.

Therefore, when searching for empty lines using a multi-line regular expression, you should also check whether the string is empty.

This rule is raising an issue every time a pattern that can match the empty string is used with MULTILINE flag and without calling isEmpty()` on the string.

static final Pattern p = Pattern.compile("^$", Pattern.MULTILINE); // Noncompliant

// Alternatively
static final Pattern p = Pattern.compile("(?m)^$"); // Noncompliant


boolean containsEmptyLines(String str) {
return p.matcher(str).find();
}

// ...
System.out.println(containsEmptyLines("a\n\nb")); // correctly prints 'true'
System.out.println(containsEmptyLines("")); // incorrectly prints 'false'

Using upper case literal suffixes removes the potential ambiguity between “1” (digit 1) and “l” (letter el) for declaring literals.

long long1 = 1l; // Noncompliant
float float1 = 1.0f; // Noncompliant
double double1 = 1.0d; // Noncompliant

Lookahead assertions are a regex feature that makes it possible to look ahead in the input without consuming it. It is often used at the end of regular expressions to make sure that substrings only match when they are followed by a specific pattern.

For example, the following pattern will match an “a” only if it is directly followed by a “b”. This does not consume the “b” in the process:

Unresolved directive in <stdin> - include::{lookahead}[]

However, lookaheads can also be used in the middle (or at the beginning) of a regex. In that case there is the possibility that what comes after the lookahead contradicts the pattern inside the lookahead. Since the lookahead does not consume input, this makes the lookahead impossible to match and is a sign that there’s a mistake in the regular expression that should be fixed.

Pattern.compile("(?=a)b"); // Noncompliant, the same character can't be equal to 'a' and 'b' at the same time

Clear-text protocols such as `ftp, telnet, or http lack encryption of transported data, as well as the capability to build an authenticated connection. It means that an attacker able to sniff traffic from the network can read, modify, or corrupt the transported content. These protocols are not secure as they expose applications to an extensive range of risks:

sensitive data exposure
traffic redirected to a malicious endpoint
malware-infected software update or installer
execution of client-side code
corruption of critical information

Even in the context of isolated networks like offline environments or segmented cloud environments, the insider threat exists. Thus, attacks involving communications being sniffed or tampered with can still happen.

For example, attackers could successfully compromise prior security layers by:

bypassing isolation mechanisms
compromising a component of the network
getting the credentials of an internal IAM account (either from a service account or an actual person)

In such cases, encrypting communications would decrease the chances of attackers to successfully leak data or steal credentials from other network components. By layering various security practices (segmentation and encryption, for example), the application will follow the defense-in-depth principle.

Note that using the http` protocol is being deprecated by major web browsers.

In the past, it has led to the following vulnerabilities:

TelnetClient telnet = new TelnetClient(); // Sensitive

FTPClient ftpClient = new FTPClient(); // Sensitive

SMTPClient smtpClient = new SMTPClient(); // Sensitive

When a cookie is configured with the HttpOnly attribute set to true, the browser guaranties that no client-side script will be able to read it. In most cases, when a cookie is created, the default value of HttpOnly is false and it’s up to the developer to decide whether or not the content of the cookie can be read by the client-side script. As a majority of Cross-Site Scripting (XSS) attacks target the theft of session-cookies, the HttpOnly attribute can help to reduce their impact as it won’t be possible to exploit the XSS vulnerability to steal session-cookies.

Cookie c = new Cookie(COOKIENAME, sensitivedata);
c.setHttpOnly(true); // Compliant: this sensitive cookie is protected against theft (HttpOnly=true)

Array designators should always be located on the type for better code readability. Otherwise, developers must look both at the type and the variable name to know whether or not a variable is an array.

int matrix[][];   // Noncompliant
int[] matrix[];   // Noncompliant

If a private field is declared but not used locally, its limited visibility makes it dead code.

This is either a sign that some logic is missing or that the code should be cleaned.

Cleaning out dead code decreases the size of the maintained codebase, making it easier to understand and preventing bugs from being introduced.

public class MyClass {
private int foo = 42; // Noncompliant: foo is unused and should be removed

public int compute(int a) {
return a * 42;
}

}

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.

However, there are instances where casting expressions are not needed. These include situations like:

casting a variable to its own type
casting a subclass to a parent class (in the case of polymorphism)
the programming language is capable of automatically converting the given type to another

These scenarios are considered unnecessary casting expressions. They can complicate the code and make it more difficult to understand, without offering any advantages.

As a result, it’s generally advised to avoid unnecessary casting expressions. Instead, rely on the language’s type system to ensure type safety and code clarity.

class A {}
class B extends A{}
class C {
void fun(A a){}
void fun(B b){}

void foo() {
B b = new B();
fun(b);
fun((A) b); // Compliant, required to call the first method so cast is not redundant.
}
}

A regular expression is a sequence of characters that specifies a match pattern in text. Among the most important concepts are:

Character classes: defines a set of characters, any one of which can occur in an input string for a match to succeed.
Quantifiers: used to specify how many instances of a character, group, or character class must be present in the input for a match.
Wildcard (.): matches all characters except line terminators (also matches them if the s flag is set).

Many of these features include shortcuts of widely used expressions, so there is more than one way to construct a regular expression to achieve the same results. For example, to match a two-digit number, one could write [0-9]{2,2} or \d{2}. The latter is not only shorter but easier to read and thus to maintain.

This rule recommends replacing some quantifiers and character classes with more concise equivalents:

\d for [0-9] and \D for [^0-9]
\w for [A-Za-z0-9_] and \W for `[^A-Za-z0-9_]
. for character classes matching everything (e.g. [\w\W], [\d\D], or [\s\S] with s flag)
x? for x{0,1}, x* for x{0,}, x+ for x{1,}, x{N} for x{N,N}`

"[0-9]"        // Noncompliant - same as "\\d"
"[^0-9]"       // Noncompliant - same as "\\D"
"[A-Za-z0-9_]" // Noncompliant - same as "\\w"
"[\\w\\W]"     // Noncompliant - same as "."
"a{0,}"        // Noncompliant - same as "a*"

Arbitrary OS command injection vulnerabilities are more likely when a shell is spawned rather than a new process, indeed shell meta-chars can be used (when parameters are user-controlled for instance) to inject OS commands.

Runtime.getRuntime().exec("/usr/bin/file.exe");  // Compliant

ProcessBuilder pb = new ProcessBuilder("/usr/bin/file.exe");  // Compliant
pb.command("/usr/bin/file.exe";  // Sensitive.

CommandLine cmdLine = CommandLine.parse("/usr/bin/file.exe"); // Compliant
DefaultExecutor executor = new DefaultExecutor();
executor.execute(cmdLine);

Even if it is legal, mixing case and non-case labels in the body of a switch statement is very confusing and can even be the result of a typing error.

switch (day) {
case MONDAY:
case TUESDAY:
WEDNESDAY:   // Noncompliant; syntactically correct, but behavior is not what's expected
doSomething();
break;
...
}

switch (day) {
case MONDAY:
break;
case TUESDAY:
foo:for(int i = 0 ; i < X ; i++) {  // Noncompliant; the code is correct and behaves as expected but is barely readable 
     /* ... */
    break foo;  // this break statement doesn't relate to the nesting case TUESDAY
     /* ... */
}
break;
/* ... */
}

Using the same value on both sides of a binary operator is a code defect. In the case of logical operators, it is either a copy/paste error and, therefore, a bug, or it is simply duplicated code and should be simplified. In the case of bitwise operators and most binary mathematical operators, having the same value on both sides of an operator yields predictable results and should be simplified as well.

if ( a == a ) { // always true
doZ();
}
if ( a != a ) { // always false
doY();
}
if ( a == b && a == b ) { // if the first one is true, the second one is too
doX();
}
if ( a == b || a == b ) { // if the first one is true, the second one is too
doW();
}

int j = 5 / 5; //always 1
int k = 5 - 5; //always 0

c.equals(c); //always true

When either the equality operator in a null test or the logical operator that follows it is reversed, the code has the appearance of safely null-testing the object before dereferencing it. Unfortunately the effect is just the opposite - the object is null-tested and then dereferenced only if it is null, leading to a guaranteed null pointer dereference.

if (str == null && str.length() == 0) {
System.out.println("String is empty");
}

if (str != null || str.length() > 0) {
System.out.println("String is not empty");
}

Rejecting requests with significant content length is a good practice to control the network traffic intensity and thus resource consumption in order to prevent DoS attacks.

@Bean(name = "multipartResolver") 
public CommonsMultipartResolver multipartResolver() {
CommonsMultipartResolver multipartResolver = new CommonsMultipartResolver();
multipartResolver.setMaxUploadSize(104857600); // Sensitive (100MB)
return multipartResolver;
}

@Bean(name = "multipartResolver") 
public CommonsMultipartResolver multipartResolver() {
CommonsMultipartResolver multipartResolver = new CommonsMultipartResolver(); // Sensitive, by default if maxUploadSize property is not defined, there is no limit and thus it's insecure
return multipartResolver;
}

@Bean
public MultipartConfigElement multipartConfigElement() {
MultipartConfigFactory factory = new MultipartConfigFactory(); // Sensitive, no limit by default
return factory.createMultipartConfig();
}

A common code smell that can hinder the clarity of source code is making assignments within sub-expressions. This practice involves assigning a value to a variable inside a larger expression, such as within a loop or a conditional statement.

This practice essentially gives a side-effect to a larger expression, thus making it less readable. This often leads to confusion and potential errors.

void processInput(BufferedReader br) {
String line;
while ((line = br.readLine()) != null) {
processLine(line);
}
}

Object foo;
if ((foo = bar()) != null) {
// do something with "foo"
}

A chain of if/else if statements is evaluated from top to bottom. At most, only one branch will be executed: the first one with a condition that evaluates to true.

Therefore, duplicating a condition automatically leads to dead code. Usually, this is due to a copy/paste error. At best, it’s simply dead code and at worst, it’s a bug that is likely to induce further bugs as the code is maintained, and obviously it could lead to unexpected behavior.

if (param == 1)
openWindow();
else if (param == 2)
closeWindow();
else if (param == 1)  // Noncompliant
moveWindowToTheBackground();
}

In regular expressions the boundaries `^ and \A can only match at the beginning of the input (or, in case of ^ in combination with the MULTILINE flag, the beginning of the line) and $, \Z and \z only at the end.

These patterns can be misused, by accidentally switching ^ and $` for example, to create a pattern that can never match.

// This can never match because $ and ^ have been switched around
Pattern.compile("$[a-z]+^"); // Noncompliant

Formatted SQL queries can be difficult to maintain, debug and can increase the risk of SQL injection when concatenating untrusted values into the query. However, this rule doesn’t detect SQL injections (unlike rule S3649), the goal is only to highlight complex/formatted queries.

public User getUser(Connection con, String user) throws SQLException {

Statement stmt1 = null;
PreparedStatement pstmt = null;
String query = "select FNAME, LNAME, SSN " +
             "from USERS where UNAME=?"
try {
stmt1 = con.createStatement();
ResultSet rs1 = stmt1.executeQuery("GETDATE()");

pstmt = con.prepareStatement(query);
pstmt.setString(1, user);  // Good; PreparedStatements escape their inputs.
ResultSet rs2 = pstmt.executeQuery();

//...
}
}

public User getUserHibernate(org.hibernate.Session session, String data) {

org.hibernate.Query query =  session.createQuery("FROM students where fname = ?");
query = query.setParameter(0,data);  // Good; Parameter binding escapes all input

org.hibernate.Query query2 =  session.createQuery("FROM students where fname = " + data); // Sensitive
// ...

A typical code smell known as unused function parameters refers to parameters declared in a function but not used anywhere within the function’s body. While this might seem harmless at first glance, it can lead to confusion and potential errors in your code. Disregarding the values passed to such parameters, the function’s behavior will be the same, but the programmer’s intention won’t be clearly expressed anymore. Therefore, removing function parameters that are not being utilized is considered best practice.

void doSomething(int a, int b) { // Noncompliant, "b" is unused
compute(a);
}

Public fields in public classes do not respect the encapsulation principle and have three main disadvantages:

Additional behavior such as validation cannot be added.
The internal representation is exposed, and cannot be changed afterwards.
Member values are subject to change from anywhere in the code and may not meet the programmer’s assumptions.

To prevent unauthorized modifications, private attributes and accessor methods (set and get) should be used.

public class MyClass {

public static final int SOME_CONSTANT = 0;     // Compliant - constants are not checked

public String firstName;                       // Noncompliant

}

Storing data locally is a common task for mobile applications. Such data includes files among other things. One convenient way to store files is to use the external file storage which usually offers a larger amount of disc space compared to internal storage.

Files created on the external storage are globally readable and writable. Therefore, a malicious application having the permissions WRITE_EXTERNAL_STORAGE or READ_EXTERNAL_STORAGE could try to read sensitive information from the files that other applications have stored on the external storage.

External storage can also be removed by the user (e.g when based on SD card) making the files unavailable to the application.

import android.content.Context;

public class AccessExternalFiles {

public void accessFiles(Context context) {
    context.getFilesDir();
}
}

When placing Unicode Grapheme Clusters (characters which require to be encoded in multiple Code Points) inside a character class of a regular expression, this will likely lead to unintended behavior.

For instance, the grapheme cluster c̈ requires two code points: one for ‘c’, followed by one for the umlaut modifier ‘\u{0308}’. If placed within a character class, such as [c̈], the regex will consider the character class being the enumeration [c\u{0308}] instead. It will, therefore, match every ‘c’ and every umlaut that isn’t expressed as a single codepoint, which is extremely unlikely to be the intended behavior.

This rule raises an issue every time Unicode Grapheme Clusters are used within a character class of a regular expression.

"cc̈d̈d".replaceAll("[c̈d̈]", "X"); // Noncompliant, print "XXXXXX" instead of expected "cXXd".

Shared naming conventions improve readability and allow teams to collaborate efficiently. This rule checks that all package names match a provided regular expression.

package org.exAmple; // Noncompliant

Constructing arguments of system commands from user input is security-sensitive. It has led in the past to the following vulnerabilities:

Arguments of system commands are processed by the executed program. The arguments are usually used to configure and influence the behavior of the programs. Control over a single argument might be enough for an attacker to trigger dangerous features like executing arbitrary commands or writing files into specific directories.

String input = request.getParameter("input");
if (allowed.contains(input)) {
String cmd[] =  new String[] { "/usr/bin/find", input };
Runtime.getRuntime().exec(cmd);
}

Hard-coding a URI makes it difficult to test a program for a variety of reasons:

path literals are not always portable across operating systems
a given absolute path may not exist in a specific test environment
a specified Internet URL may not be available when executing the tests
production environment filesystems usually differ from the development environment

In addition, hard-coded URIs can contain sensitive information, like IP addresses, and they should not be stored in the code.

For all those reasons, a URI should never be hard coded. Instead, it should be replaced by a customizable parameter.

Further, even if the elements of a URI are obtained dynamically, portability can still be limited if the path delimiters are hard-coded.

This rule raises an issue when URIs or path delimiters are hard-coded.

public class Foo {
public Collection<User> listUsers() {
File userList = new File("/home/mylogin/Dev/users.txt"); // Noncompliant
Collection<User> users = parse(userList);
return users;
}
}

This rule applies whenever an `if statement is followed by one or more else if statements; the final else if should be followed by an else statement.

The requirement for a final else statement is defensive programming.

The else statement should either take appropriate action or contain a suitable comment as to why no action is taken. This is consistent with the requirement to have a final default clause in a switch` statement.

if (x == 0) {
doSomething();
} else if (x == 1) {
doSomethingElse();
}

To reduce the risk of cross-site scripting attacks, templating systems, such as `Twig, Django, Smarty, Groovy’s template engine, allow configuration of automatic variable escaping before rendering templates. When escape occurs, characters that make sense to the browser (eg: <a>) will be transformed/replaced with escaped/sanitized values (eg: & lt;a& gt; ).

Auto-escaping is not a magic feature to annihilate all cross-site scripting attacks, it depends on the strategy applied and the context, for example a “html auto-escaping” strategy (which only transforms html characters into html entities) will not be relevant when variables are used in a html attribute because ’:’ character is not escaped and thus an attack as below is possible:

<a href=”{{ myLink }}“>link</a> // myLink = javascript:alert(document.cookie)
<a href=“javascript:alert(document.cookie)“>link</a> // JS injection (XSS attack)

Mustache.compiler().compile(template).execute(context); // Compliant, auto-escaping is enabled by default
Mustache.compiler().escapeHTML(true).compile(template).execute(context); // Compliant

Multiple spaces in a regular expression can make it hard to tell how many spaces should be matched. It’s more readable to use only one space and then indicate with a quantifier how many spaces are expected.

Pattern.compile("hello   world");

User-provided data, such as URL parameters, POST data payloads, or cookies, should always be considered untrusted and tainted. Applications constructing HTTP response headers based on tainted data could allow attackers to change security sensitive headers like Cross-Origin Resource Sharing headers.

Web application frameworks and servers might also allow attackers to inject new line characters in headers to craft malformed HTTP response. In this case the application would be vulnerable to a larger range of attacks like HTTP Response Splitting/Smuggling. Most of the time this type of attack is mitigated by default modern web application frameworks but there might be rare cases where older versions are still vulnerable.

As a best practice, applications that use user-provided data to construct the response header should always validate the data first. Validation should be based on a whitelist.

protected void doGet(HttpServletRequest req, HttpServletResponse resp) throws IOException {
String value = req.getParameter("value");
resp.addHeader("X-Header", value); // Noncompliant
}

Android applications can receive broadcasts from the system or other applications. Receiving intents is security-sensitive. For example, it has led in the past to the following vulnerabilities:

Receivers can be declared in the manifest or in the code to make them context-specific. If the receiver is declared in the manifest Android will start the application if it is not already running once a matching broadcast is received. The receiver is an entry point into the application.

Other applications can send potentially malicious broadcasts, so it is important to consider broadcasts as untrusted and to limit the applications that can send broadcasts to the receiver.

Permissions can be specified to restrict broadcasts to authorized applications. Restrictions can be enforced by both the sender and receiver of a broadcast. If permissions are specified when registering a broadcast receiver, then only broadcasters who were granted this permission can send a message to the receiver.

This rule raises an issue when a receiver is registered without specifying any broadcast permission.

import android.content.BroadcastReceiver;
import android.content.Context;
import android.content.IntentFilter;
import android.os.Build;
import android.os.Handler;
import android.support.annotation.RequiresApi;

public class MyIntentReceiver {

@RequiresApi(api = Build.VERSION_CODES.O)
public void register(Context context, BroadcastReceiver receiver,
                     IntentFilter filter,
                     String broadcastPermission,
                     Handler scheduler,
                     int flags) {

    context.registerReceiver(receiver, filter, broadcastPermission, scheduler);
    context.registerReceiver(receiver, filter, broadcastPermission, scheduler, flags);
}
}

When a cookie is protected with the secure attribute set to true it will not be send by the browser over an unencrypted HTTP request and thus cannot be observed by an unauthorized person during a man-in-the-middle attack.

Cookie c = new Cookie(COOKIENAME, sensitivedata);
c.setSecure(true); // Compliant: the sensitive cookie will not be send during an unencrypted HTTP request thanks to the secure flag set to true

If a label is declared but not used in the program, it can be considered as dead code and should therefore be removed.

This will improve maintainability as developers will not wonder what this label is used for.

void foo() {
outer: //label is not used.
for(int i = 0; i<10; i++) {
break;
}
}

Android comes with Android KeyStore, a secure container for storing key materials. It’s possible to define certain keys to be unlocked when users authenticate using biometric credentials. This way, even if the application process is compromised, the attacker cannot access keys, as presence of the authorized user is required.

These keys can be used, to encrypt, sign or create a message authentication code (MAC) as proof that the authentication result has not been tampered with. This protection defeats the scenario where an attacker with physical access to the device would try to hook into the application process and call the `onAuthenticationSucceeded method directly. Therefore he would be unable to extract the sensitive data or to perform the critical operations protected by the biometric authentication.

Ask Yourself Whether

The application contains:

Cryptographic keys / sensitive information that need to be protected using biometric authentication.

There is a risk if you answered yes to this question.

Recommended Secure Coding Practices

It’s recommended to tie the biometric authentication to a cryptographic operation by using a CryptoObject` during authentication.

// ...
BiometricPrompt biometricPrompt = new BiometricPrompt(activity, executor, callback);
// ...
biometricPrompt.authenticate(promptInfo); // Noncompliant

Why use named groups only to never use any of them later on in the code?

This rule raises issues every time named groups are:

defined but never called anywhere in the code through their name;
defined but called elsewhere in the code by their number instead;
referenced while not defined.

String date = "01/02";

Pattern datePattern = Pattern.compile("(?<month>[0-9]{2})/(?<year>[0-9]{2})"); 
Matcher dateMatcher = datePattern.matcher(date);

if (dateMatcher.matches()) {
checkValidity(dateMatcher.group(1), dateMatcher.group(2));  // Noncompliant - numbers instead of names of groups are used
checkValidity(dateMatcher.group("day")); // Noncompliant - there is no group called "day"
}

// ...

String score = "14:1";

Pattern scorePattern = Pattern.compile("(?<player1>[0-9]+):(?<player2>[0-9]+)"); // Noncompliant - named groups are never used
Matcher scoreMatcher = scorePattern.matcher(score);

if (scoreMatcher.matches()) {
checkScore(score);
}

Nested control flow statements such as if, for, while, switch, and try are often key ingredients in creating what’s known as “Spaghetti code”. This code smell can make your program difficult to understand and maintain.

When numerous control structures are placed inside one another, the code becomes a tangled, complex web. This significantly reduces the code’s readability and maintainability, and it also complicates the testing process.

if (condition1) {                  // Compliant - depth = 1
/* ... */
if (condition2) {                // Compliant - depth = 2
/* ... */
for (int i = 0; i < 10; i++) {  // Compliant - depth = 3
  /* ... */
  if (condition4) {            // Noncompliant - depth = 4, which exceeds the limit
    if (condition5) {          // Depth = 5, exceeding the limit, but issues are only reported on depth = 4
      /* ... */
    }
    return;
  }
}
}
}

There is no good reason to create a new object to not do anything with it. Most of the time, this is due to a missing piece of code and so could lead to an unexpected behavior in production.

If it was done on purpose because the constructor has side-effects, then that side-effect code should be moved into a separate method and called directly.

if (x < 0)
new IllegalArgumentException("x must be nonnegative");

A catch clause that only rethrows the caught exception has the same effect as omitting the catch altogether and letting it bubble up automatically.

Unresolved directive in <stdin> - include::{example}[]

Such clauses should either be removed or populated with the appropriate logic.

Unresolved directive in <stdin> - include::{compliant}[]

String readFirstLine(FileReader fileReader) throws IOException {
try (BufferedReader br = new BufferedReader(fileReader)) {
 return br.readLine();
} catch (IOException e) { // Noncompliant
throw e;
}

Overriding a method just to call the same method from the super class without performing any other actions is useless and misleading. The only time this is justified is in final overriding methods, where the effect is to lock in the parent class behavior. This rule ignores such overrides of equals, hashCode and toString.

public void doSomething() {                
super.doSomething();
}

@Override
public boolean isLegal(Action action) {      
return super.isLegal(action);
}

Duplicated string literals make the process of refactoring complex and error-prone, as any change would need to be propagated on all occurrences.

public void run() {
prepare("action1");                              // Noncompliant - "action1" is duplicated 3 times
execute("action1");
release("action1");
}

@SuppressWarning("all")                            // Compliant - annotations are excluded
private void method1() { /* ... */ }
@SuppressWarning("all")
private void method2() { /* ... */ }

public String printInQuotes(String a, String b) {
return "'" + a + "'" + b + "'";               // Compliant - literal "'" has less than 5 characters and is excluded
}

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.

public void makeItPublic(String methodName) throws NoSuchMethodException {

this.getClass().getMethod(methodName).setAccessible(true); // Noncompliant 
}

public void setItAnyway(String fieldName, int value) {
this.getClass().getDeclaredField(fieldName).setInt(this, value); // Noncompliant; bypasses controls in setter 
}

When the value of a private field is always assigned to in a class’ methods before being read, then it is not being used to store class information. Therefore, it should become a local variable in the relevant methods to prevent any misunderstanding.

public class Foo {
private int a;
private int b;

public void doSomething(int y) {
a = y + 5;
...
if(a == 0) {
  ...
}
...
}

public void doSomethingElse(int y) {
b = y + 3;
...
}
}

Get Started

Ask Yourself Whether

Recommended Secure Coding Practices