> ## Documentation Index > Fetch the complete documentation index at: https://docs.codeant.ai/llms.txt > Use this file to discover all available pages before exploring further. # Java - 3 > Learn about Java Anti-Patterns and How they help you write better code, and avoid common pitfalls.

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.

```java Bad theme={null} public void doSomething(String str) { if (Math.abs(str.hashCode()) > 0) { // Noncompliant // ... } } ``` ```java Fix theme={null} public void doSomething(String str) { if (str.hashCode() != 0) { // ... } } ```

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.

```java Bad theme={null} @CheckForNull boolean isFoo() { ... } ``` ```java Fix theme={null} 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().

```java Bad theme={null} 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 } } ``` ```java Fix theme={null} class MyClass { private int foo = 1; @Override public boolean equals(Object o) { // Compliant if (this == o) { return true; } if (o == null || getClass() != o.getClass()) { return false; } MyClass other = (MyClass)o; return this.foo == other.foo; } } ```

According to the documentation,

A program may produce unpredictable results if it attempts to distinguish two references to equal values of a value-based class, whether directly via reference equality or indirectly via an appeal to synchronization, identity hashing, serialization…​

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

```java Bad theme={null} class MyClass implements Serializable { private HijrahDate date; // Noncompliant; mark this transient // ... } ``` ```java Fix theme={null} class MyClass implements Serializable { private transient HijrahDate date; // ... } ```

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

```java Bad theme={null} int myInt = 4; String myIntString = (new Integer(myInt)).toString(); // Noncompliant; creates & discards an Integer object myIntString = Integer.valueOf(myInt).toString(); // Noncompliant ``` ```java Fix theme={null} int myInt = 4; String myIntString = Integer.toString(myInt); ```

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

returns true if the specified Object argument is an instance of the represented class (or of any of its subclasses); it returns false otherwise

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.

```java Bad theme={null} Class num = Number.class; Class 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 ``` ```java Fix theme={null} Class num = Number.class; Class bi = BigInteger.class; System.out.println(num.isAssignableFrom(bi)); // true System.out.println(bi.isAssignableFrom(Class.class)); // false System.out.println(Class.class.isAssignableFrom(bi)); // false ```

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.

```java Bad theme={null} String str1 = "Now is the time for all good people"; String str2 = str1.toString(); ``` ```java Fix theme={null} String str1 = "Now is the time for all good people"; String str2 = str1; ```

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.

```java Bad theme={null} 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) { // ... } } ``` ```java Fix theme={null} public class Highlander implements Immortal { public static final Highlander INSTANCE = new Highlander; private Highlander() { } public void eliminateRival(Immortal immortal) { // ... } } public class Kurgan implements Immortal { public static final Kurgan INSTANCE = new Kurgan; private Kurgan() { } 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.

```java Bad theme={null} 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 // ... } //... ``` ```java Fix theme={null} public class Fruit extends Food { private Season ripe; public boolean equals(Object obj) { if (obj == this) { return true; } if (obj == null) { return false; } if (this.getClass() == obj.getClass()) { return ripe.equals(((Fruit)obj).getRipe()); } return false; } ```

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.

```java Bad theme={null} map.computeIfAbsent(key, k -> null); // Noncompliant, the map will not contain an entry key->null. map.computeIfPresent(key, (k, oldValue) -> null); // Noncompliant ``` ```java Fix theme={null} if (!map.containsKey(key)) { map.put(key, null); } if (map.containsKey(key)) { map.put(key, null); } ```

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.

```java Bad theme={null} myCollection.stream().map(new Function() { // Noncompliant, use a lambda expression instead @Override public String apply(String input) { return new StringBuilder(input).reverse().toString(); } }) ... ``` ```java Fix theme={null} myCollection.stream() .map(input -> new StringBuilder(input).reverse().toString()) // Compliant ... ```

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.

```java Bad theme={null} S3Client.builder() .credentialsProvider(EnvironmentVariableCredentialsProvider.create()) .build(); ``` ```java Fix theme={null} S3Client.builder() .region(Region.of(System.getenv(SdkSystemSetting.AWS_REGION.environmentVariable())) .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.

```java Bad theme={null} public class Student { // no hashCode() method; not hash-able // ... public boolean equals(Object o) { // ... } } public class School { private Map studentBody = // okay so far new HashTable(); // Noncompliant // ... ``` ```java Fix theme={null} public class Student { // has hashCode() method; hash-able // ... public boolean equals(Object o) { // ... } public int hashCode() { // ... } } public class School { private Map studentBody = new HashTable(); // ... ```

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.

```java Bad theme={null} 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 } ``` ```java Fix theme={null} public class NonSerializableClassWithoutConstructor { // after deserialization, "field1" will always be set to 42 private int field1 = 42; // this non-serializable class has an implicit no-argument constructor } public class NonSerializableClass extends NonSerializableClassWithoutConstructor { // after deserialization, "field2" will always be set to 12 by the no-argument constructor private int field2; // this non-serializable class has an explicit no-argument constructor public NonSerializableClass() { field2 = 12; } public NonSerializableClass(int field2) { this.field2 = field2; } } public class SerializableClass extends NonSerializableClass implements Serializable { // after deserialization, "field3" will have the previously serialized value. private int field3; // deserialization does not use declared constructors public SerializableClass(int field3) { super(field3 * 2); this.field3 = field3; } } ```

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.

```java Bad theme={null} myString.toLowerCase() ``` ```java Fix theme={null} myString.toLowerCase(Locale.TR) ```

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.

```java Bad theme={null} public enum Color { RED, GREEN, BLUE, ORANGE; } Map colorMap = new HashMap<>(); // Noncompliant ``` ```java Fix theme={null} public enum Color { RED, GREEN, BLUE, ORANGE; } Map colorMap = new EnumMap<>(Color.class); // Compliant ```

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\`

```java Bad theme={null} Statement stmt = con.createStatement(ResultSet.TYPE_FORWARD_ONLY); stmt.executeQuery("SELECT name, address FROM PERSON"); ResultSet rs = stmt.getResultSet(); if (rs.isBeforeFirst()) { // Noncompliant } ``` ```java Fix theme={null} ```

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.

```java Bad theme={null} boolean foo(Object param) { if (expression) { // Noncompliant bar(param, true, "qix"); } else { bar(param, false, "qix"); } if (expression) { // Noncompliant return true; } else { return false; } } ``` ```java Fix theme={null} boolean foo(Object param) { bar(param, expression, "qix"); return expression; } ```

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.

```java Bad theme={null} class ParentClass { public boolean doSomething(){/*...*/} } class FirstChildClass extends ParentClass { public boolean doSomething(){/*...*/} // Noncompliant } ``` ```java Fix theme={null} class ParentClass { public boolean doSomething(){/*...*/} } class FirstChildClass extends ParentClass { @Override public boolean doSomething(){/*...*/} // Compliant } ```

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.

```java Bad theme={null} public int shift(int a) { int x = a >> 32; // Noncompliant return a << 48; // Noncompliant } ``` ```java Fix theme={null} public int shift(int a) { int x = a >> 31; return a << 16; } ```

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.

```java Bad theme={null} new Thread(() -> { try { Thread.sleep(1000); // Noncompliant blocking operation in platform thread } catch (InterruptedException e) { throw new RuntimeException(e); } }); ``` ```java Fix theme={null} Thread.ofVirtual().start(() -> { try { Thread.sleep(1000); // Compliant } 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.

```java Bad theme={null} String message = "Hello " + "world" + "!"; // Noncompliant StringBuilder sb = new StringBuilder(); sb.append("I'm pleased").append(" to meet you."); //Noncompliant ``` ```java Fix theme={null} String message = "Hello world!"; StringBuilder sb = new StringBuilder(); sb.append("I'm pleased to meet you."); ```

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.

```java Bad theme={null} @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 Fix theme={null} @Component @Scope(value = WebApplicationContext.SCOPE_REQUEST, proxyMode = ScopedProxyMode.TARGET_CLASS) public class RequestBean { //... } public class SingletonBean { private final ObjectFactory requestBeanFactory; @Autowired public SingletonBean(ObjectFactory requestBeanFactory) { this.requestBeanFactory = requestBeanFactory; } public RequestBean getRequestBean() { return requestBeanFactory.getObject(); } } ```

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.

```java Bad theme={null} void printLastToFirst(List list) { for (var it = list.listIterator(list.size()); it.hasPrevious();) { var element = it.previous(); System.out.println(element); } } ``` ```java Fix theme={null} void printLastToFirst(List list) { for (var element: list.reversed()) { 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.

```java Bad theme={null} 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(); } ``` ```java Fix theme={null} ```

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

```java Bad theme={null} 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()); } } ``` ```java Fix theme={null} public class Tomato implements Externalizable { public Color color; // Compliant; deserialization will invoke this public no-argument constructor public Tomato() { this.color = Color.UNKNOWN; } public Tomato(Color color) { this.color = color; } @Override public void writeExternal(ObjectOutput out) throws IOException { out.writeUTF(color.name()); } @Override public void readExternal(ObjectInput in) throws IOException { color = Color.valueOf(in.readUTF()); } } ```

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.

```java Bad theme={null} class A { @Deprecated void foo(){} } class B extends A { @Override void foo(){ // Noncompliant } } ``` ```java Fix theme={null} ```

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.

```java Bad theme={null} int f(Object o) { if (o instanceof String) {  // Noncompliant String string = (String) o; return string.length(); } return 0; } ``` ```java Fix theme={null} int f(Object o) {   if (o instanceof String string) {  // Compliant     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.

```java Bad theme={null} public class Myclass { public final int THRESHOLD = 3; } ``` ```java Fix theme={null} public class Myclass { public static final int THRESHOLD = 3; // Compliant } ```

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.

```java Bad theme={null} String str = ""; for (int i = 0; i < arrayOfStrings.length ; ++i) { str = str + arrayOfStrings[i]; } ``` ```java Fix theme={null} StringBuilder bld = new StringBuilder(); for (int i = 0; i < arrayOfStrings.length; ++i) { bld.append(arrayOfStrings[i]); } String str = bld.toString(); ```

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

```java Bad theme={null} ClassLoader cl = this.getClass().getClassLoader(); // Noncompliant ``` ```java Fix theme={null} ClassLoader cl = Thread.currentThread().getContextClassLoader(); ```

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.

```java Bad theme={null} 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! ``` ```java Fix theme={null} List integers = new ArrayList<>(); // The program does not compile anymore with this mistake: // integers.add("Hello World!"); integers.add(42); Integer a = integers.get(0); // No need to cast anymore. ```

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.

```java Bad theme={null} 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(); } ... } ``` ```java Fix theme={null} public void doSomething(){ synchronized(monitor) { while(notReady()){ monitor.wait(200); // Compliant, the current monitor is released. } 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.

```java Bad theme={null} class MyClass { public void doSomethingCommon() { Random random = new Random(); // Noncompliant - new instance created with each invocation int rValue = random.nextInt(); } } ``` ```java Fix theme={null} class MyClass { private Random random = new Random(); // Compliant public void doSomethingCommon() { int rValue = this.random.nextInt(); } } ```

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

JUnit provides a gentle migration path via a JUnit Vintage test engine which allows existing tests based on JUnit 3 and JUnit 4 to be executed using the JUnit Platform infrastructure. Since all classes and annotations specific to JUnit Jupiter reside under a new org.junit.jupiter base package, having both JUnit 4 and JUnit Jupiter in the classpath does not lead to any conflicts.

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

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\`
```java Bad theme={null} 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"); } } ``` ```java Fix theme={null} package org.foo; import org.junit.jupiter.api.AfterAll; import org.junit.jupiter.api.AfterEach; import org.junit.jupiter.api.BeforeAll; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Disabled; import org.junit.jupiter.api.Tag; import org.junit.jupiter.api.Test; import org.junit.jupiter.api.extension.ExtendWith; @ExtendWith(MyJUnit5Extension.class) class MyJUnit5Test { @BeforeAll static void beforeAll() { System.out.println("beforeAll"); } @AfterAll static void afterAll() { System.out.println("afterAll"); } @BeforeEach void beforeEach() { System.out.println("beforeEach"); } @AfterEach void afterEach() { System.out.println("afterEach"); } @Test void test1() { System.out.println("test1"); } @Test @Tag("SomeTests") void test2() { System.out.println("test2"); } @Test @Disabled("Requires fix of #42") void disabled() { 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.

```java Bad theme={null} 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 } } ``` ```java Fix theme={null} class MyComponent { private final MyCollaborator collaborator; @Inject public MyComponent(MyCollaborator collaborator) { Assert.notNull(collaborator, "MyCollaborator must not be null!"); this.collaborator = collaborator; } public void myBusinessMethod() { collaborator.doSomething(); } } ```

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.

```java Bad theme={null} String textBlock = """ this is text block! !!!! """; ``` ```java Fix theme={null} String textBlock = """ this is 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.

```java Bad theme={null} public abstract class Car { public abstract void start(Environment c); public void stop(Environment c) { c.freeze(this); } } ``` ```java Fix theme={null} public interface Car { public void start(Environment c); public default 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.

```java Bad theme={null} 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 ``` ```java Fix theme={null} String myNum = "42.0"; float f = Float.parseFloat(myNum); int myInteger = Integer.parseInt(myNum); ```

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.

```java Bad theme={null} 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 ``` ```java Fix theme={null} ```

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.

```java Bad theme={null} 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; } } ``` ```java Fix theme={null} class Bar extends Foo { // Noncompliant, `equals` ignores the value of `b` final int b; } ```

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.

```java Bad theme={null} public int hashcode() { /* ... */ } // Noncompliant public String tostring() { /* ... */ } // Noncompliant public boolean equal(Object obj) { /* ... */ } // Noncompliant ``` ```java Fix theme={null} @Override public int hashCode() { /* ... */ } // Compliant @Override public String toString() { /* ... */ } // Compliant @Override public boolean equals(Object obj) { /* ... */ } // Compliant ```

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.

```java Bad theme={null} public class SomeService { @Autowired private SomeDependency someDependency; // Noncompliant private String name = someDependency.getName(); // Will throw a NullPointerException } ``` ```java Fix theme={null} public class SomeService { private final SomeDependency someDependency; private final String name; @Autowired public SomeService(SomeDependency someDependency) { this.someDependency = someDependency; name = someDependency.getName(); } } ```

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.

Test classes, test methods, and lifecycle methods are not required to be public, but they must not be private.

It is generally recommended to omit the public modifier for test classes, test methods, and lifecycle methods unless there is a technical reason for doing so – for example, when a test class is extended by a test class in another package. Another technical reason for making classes and methods public is to simplify testing on the module path when using the Java Module System.

— JUnit5 User Guide
```java Bad theme={null} import org.junit.jupiter.api.Test; public class MyClassTest { // Noncompliant - modifier can be removed @Test protected void test() { // Noncompliant - modifier can be removed // ... } } ``` ```java Fix theme={null} import org.junit.jupiter.api.Test; class MyClassTest { @Test void test() { // ... } } ```

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.

```java Bad theme={null} @Service public class AsyncNotificationProcessor implements NotificationProcessor { @Override public void process(Notification notification) { processAsync(notification); // Noncompliant, call bypasses proxy } @Async public processAsync(Notification notification) { // ... } } ``` ```java Fix theme={null} @Service public class AsyncNotificationProcessor implements NotificationProcessor { @Resource private AsyncNotificationProcessor @Override public void process(Notification notification) { asyncNotificationProcessor.processAsync(notification); // Compliant, call via injected 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.

```java Bad theme={null} for (;;) { // Noncompliant; end condition omitted // ... } ``` ```java Fix theme={null} int j; while (true) { // Noncompliant; end condition omitted j++; } ```

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.

```java Bad theme={null} @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(); } } ``` ```java Fix theme={null} @Configuration public class MyConfiguration { @Bean public MyService myService() { // ... return new MyService(); } @Bean(name="betterService") public MyService aBetterService() { // ... return new MyService(); } @Bean(name="evenBetterService") public MyService anEvenBetterService() { // ... return new MyService(); } @Bean(name="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.

```java Bad theme={null} class MyKeyType { // ... } class Program { Map data = new HashMap<>(); // Noncompliant Map buildMap() { // Noncompliant //... } } ``` ```java Fix theme={null} class MyKeyType implements Comparable { // ... } class MyChildKeyType extends MyKeyType { // ... } class Program { Map data = new HashMap<>(); Map data = new HashMap<>(); Map buildMap() { //... } } ```

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.

```java Bad theme={null} public void doSomething() { try { anObject.notify(); } catch(IllegalMonitorStateException e) { // Noncompliant } } ``` ```java Fix theme={null} public void doSomething() { synchronized(anObject) { anObject.notify(); } } ```

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.

```java Bad theme={null} int userAge = new Random().nextInt(42); // Noncompliant UUID userID = UUID.randomUUID(); // Noncompliant ``` ```java Fix theme={null} int userAge = 31; UUID userID = UUID.fromString("00000000-000-0000-0000-000000000001"); ```

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.

```java Bad theme={null} 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; } } ``` ```java Fix theme={null} public class ResourceFactory { private static Resource resource; public static synchronized Resource getInstance() { // Compliant, the entire method is synchronized and hence thread-safe 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

```java Bad theme={null} if (MyClass.class.isAssignableFrom(x.getClass())) { // Noncompliant MyClass mc = (MyClass)x; } ``` ```java Fix theme={null} if (x instanceof MyClass.class) { 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.

```java Bad theme={null} 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 } ``` ```java Fix theme={null} ```

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

```java Bad theme={null} 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 //... } } ``` ```java Fix theme={null} class Fruit implements Serializable { private transient Seed seed; public Fruit (Seed seed) { this.seed = seed; // 1st reference } public void setSeed (Seed seed) this.seed = seed; // 2nd reference } public Seed getSeed() { return seed; // not counted; read, not write. } public void sprout () { this.seed = new GerminatedSeed(); // Noncompliant; 3rd write //... } protected Object readResolve() throws ObjectStreamException { // ... } } ```

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.

```java Bad theme={null} 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; } } } ``` ```java Fix theme={null} public class Person { String name; int age; public synchronized void setName(String name) { this.name = name; } public synchronized String getName() { return this.name; } public void setAge(int age) { synchronized (this) { 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\`

```java Bad theme={null} 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); } } ``` ```java Fix theme={null} import java.sql.Connection; import java.sql.DriverManager; import java.sql.SQLException; public class Demo { private final Connection connection; public Demo(String serverURI) throws SQLException { 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.

```java Bad theme={null} 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")); ``` ```java Fix theme={null} ModelAndView mav = getMyModelAndView(); ModelAndViewAssert.assertViewName(mav, "register"); ModelAndViewAssert.assertModelAttributeValue(mav, "myAttribute", Boolean.TRUE); ModelAndViewAssert.assertModelAttributeValue(mav, "myAttribute", Boolean.FALSE); ModelAndViewAssert.assertModelAttributeValue(mav, "myAttribute", myObject); ```

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.

```java Bad theme={null} 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); } } ``` ```java Fix theme={null} ```

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.

```java Bad theme={null} Session session = sessionFactory.openSession(); session.setFlushMode(FlushMode.NEVER); // Noncompliant ``` ```java Fix theme={null} ```

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.

```java Bad theme={null} Thread virtualThread = Thread.ofVirtual().start(task); // Noncompliant `Thread.startVirtualThread` should be used instead ``` ```java Fix theme={null} Thread virtualThread = Thread.startVirtualThread(task); // Compliant Thread unstartedVirtualThread = Thread.ofVirtual().unstarted(task); // Compliant, the thread is unstarted Thread namedVirtualThread = Thread.ofVirtual().name("MyThread").start(); // Compliant, the builder pattern is being used to set the name Thread platformThread = Thread.ofPlatform().start(task); // Compliant, there is no `Thread.startPlatformThread` method ```

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.

```java Bad theme={null} @Deprecated ``` ```java Fix theme={null} @Deprecated(since="4.2", forRemoval=true) ```

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

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.

```java Bad theme={null} // 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); } } ``` ```java Fix theme={null} ```

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.

```java Bad theme={null} String sayHello() [] { // Noncompliant return new String[] {"hello", "world"}; } ``` ```java Fix theme={null} String [] sayHello() { 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.

```java Bad theme={null} public class MyItr implements Iterator { //... public boolean hasNext() { if (next() != null) { // Noncompliant return true; } return false; } ``` ```java Fix theme={null} public class MyItr implements Iterator { private List list; private int index = 0; //... public boolean hasNext() { return index < list.size(); } ```

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.

```java Bad theme={null} String username = "steve"; String password = "blue"; Connection conn = DriverManager.getConnection("jdbc:mysql://localhost/test?" + "user=" + username + "&password=" + password); // Sensitive ``` ```java Fix theme={null} String username = getEncryptedUser(); String password = getEncryptedPassword(); Connection conn = DriverManager.getConnection("jdbc:mysql://localhost/test?" + "user=" + username + "&password=" + password); ```

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.

```java Bad theme={null} Map source = new HashMap(){{ // Noncompliant put("firstName", "John"); put("lastName", "Smith"); }}; ``` ```java Fix theme={null} Map source = new HashMap(); // ... source.put("firstName", "John"); source.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.

```java Bad theme={null} record Person(String[] names, int age) {} // Noncompliant ``` ```java Fix theme={null} record Person(String[] names, int age) { @Override public boolean equals(Object o) { if (this == o) return true; if (o == null || getClass() != o.getClass()) return false; Person person = (Person) o; return age == person.age && Arrays.equals(names, person.names); } @Override public int hashCode() { int result = Objects.hash(age); result = 31 * result + Arrays.hashCode(names); return result; } @Override public String toString() { return "Person{" + "names=" + Arrays.toString(names) + ", age=" + age + '}'; } } ```

According to Hibernate’s documentation:

Hibernate’s own connection pooling algorithm is, …​ quite rudimentary. It is intended to help you get started and is not intended for use in a production system, or even for performance testing. You should use a third party pool for best performance and stability.

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

```java Bad theme={null} 10 ``` ```java Fix theme={null} // ... hibernate.connection.pool_size=10 // Noncompliant ```

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.

```java Bad theme={null} void foo(int n, int m) { switch (n) { case 0: switch (m) { // Noncompliant; nested switch // ... } case 1: // ... default: // ... } } ``` ```java Fix theme={null} void foo(int n, int m) { switch (n) { case 0: bar(m); case 1: // ... default: // ... } } void bar(int m){ switch(m) { // ... } } ```

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.

```java Bad theme={null} InvokeRequest invokeRequest = new InvokeRequest() .withFunctionName("myFunction"); AWSLambda awsLambda = AWSLambdaClientBuilder.standard() .withCredentials(new ProfileCredentialsProvider()) .withRegion(Regions.US_WEST_2).build(); awsLambda.invoke(invokeRequest); // Noncompliant ``` ```java Fix theme={null} ```

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.

```java Bad theme={null} class MyClass { private static final Map MY_MAP = new HashMap() { { put("a", "b"); } }; // Noncompliant - HashMap should be extended only to add behavior, not for initialization } ``` ```java Fix theme={null} class MyClass { private static final Map MY_MAP = new HashMap<>(); static { MY_MAP.put("a", "b"); // Compliant } } ```

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.

```java Bad theme={null} for (int i = 0; i < 10; ) { // Noncompliant, i not updated in increment clause // ... i++; } ``` ```java Fix theme={null} int sum = 0 for (int i = 0; i < 10; sum++) { // 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.

```java Bad theme={null} try (InputStream input = Files.newInputStream(path)) { // "input" will be closed after the execution of this block } ``` ```java Fix theme={null} try (/* resources declarations */) { // resources usage ... } ```

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.

```java Bad theme={null} @Ignore // Noncompliant @Test public void testDoTheThing() { // ... ``` ```java Fix theme={null} @Test public void testDoTheThing() { Assume.assumeFalse(true); // Noncompliant // ... ```

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

```java Bad theme={null} 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() { // ... } } } ``` ```java Fix theme={null} import org.junit.jupiter.api.Test; class MyClassTest { @Test void test1() { // ... } @Test void test2() { // ... } @Test void test3() { // ... } @Nested class MyNestedClass { @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 \#\, where MethodName can end with the wildcard character. For constructors, the pattern should be \#\.

Example: com.company.CompareToTester#compare\*,com.company.CustomAssert#customAssertMethod,com.company.CheckVerifier#\\`.

```java Bad theme={null} @Test public void testDoSomething() { // Noncompliant MyClass myClass = new MyClass(); myClass.doSomething(); } ``` ```java Fix theme={null} import com.company.CompareToTester; @Test public void testDoSomething() { MyClass myClass = new MyClass(); assertNull(myClass.doSomething()); // JUnit assertion assertThat(myClass.doSomething()).isNull(); // Fest assertion } @Test public void testDoSomethingElse() { MyClass myClass = new MyClass(); new CompareToTester().compareWith(myClass); // Compliant - custom assertion method defined as rule parameter CompareToTester.compareStatic(myClass); // Compliant } ```

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.

```java Bad theme={null} public class Pomegranate { // ... public class Seed implements Serializable { // Noncompliant, serialization will fail due to the outer class not being serializable // ... } } ``` ```java Fix theme={null} public class Pomegranate { // ... public static class Seed implements Serializable { // Compliant, the outer class will not be serialized and hence cannot be the cause for a failure at runtime // ... } } ```

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)

```java Bad theme={null} void removeFromMap(Map 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 list, Integer integer) { if (list.contains(integer)) { // Noncompliant; always false as the list only contains Strings, not integers. // ... } } ``` ```java Fix theme={null} void removeFromMap(Map map, String strKey) { map.remove(Integer.parseInt(strKey)); // Compliant, strKey is parsed into an Integer before trying to remove it from the map. } void listContains(List list, Integer integer) { if (list.contains(integer.toString())) { // Compliant, 'integer' is converted to a String before checking if the list contains it. // ... } } ```

When implementing the \`Comparable\.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\ is not same as the one used in the Comparable\` declaration.

```java Bad theme={null} public class Foo { static class Bar implements Comparable { 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; } } } ``` ```java Fix theme={null} public class Foo { static class Bar implements Comparable { public int compareTo(Bar rhs) { return -1; } } static class FooBar extends Bar { public int compareTo(Bar rhs) { 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.

```java Bad theme={null} public class Employee { // Nonserializable } @Remote public interface EmployeeServiceRemote { public Employee getEmployee(String id); // Noncompliant } ``` ```java Fix theme={null} public class Employee implements Serializable{ } @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.

```java Bad theme={null} 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 } } ``` ```java Fix theme={null} public static void main(String[] args) { try { doSomethingWhichThrowsException(); System.out.println("OK"); } catch (RuntimeException e) { System.out.println("ERROR"); // "ERROR" is printed as expected } } public static void doSomethingWhichThrowsException() { try { throw new RuntimeException(); } finally { for (int i = 0; i < 10; i ++) { //... if (q == i) { break; // ignored } } /* ... */ } } ```

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.

```java Bad theme={null} 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() { /* ... */ } } } ``` ```java Fix theme={null} import org.junit.jupiter.api.Test; import org.junit.jupiter.api.Nested; class MyJunit5Test { @Test void test() { /* ... */ } @Nested class InnerClassTest { @Test void test() { /* ... */ } } static class StaticNestedClassTest { @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.

```java Bad theme={null} 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"); } } ``` ```java Fix theme={null} enum Color{RED,GREEN,YELLOW} String name(Color c){ if(c == Color.RED){ // Noncompliant; should be replaced by a switch expression return "red"; }else if(c == Color.GREEN){ return "green"; }else if(c == Color.YELLOW){ return "yellow"; }else{ throw new IllegalArgumentException("Unknown color"); } } ```

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.

```java Bad theme={null} public class MyClass { private static String foo_bar; // Noncompliant } ``` ```java Fix theme={null} public class MyClass { private static String fooBar; } ```

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.

```java Bad theme={null} int i = 0x80003800; Double.longBitsToDouble(i); // Noncompliant - NaN ``` ```java Fix theme={null} long i = 0x80003800L; Double.longBitsToDouble(i); // Compliant - 1.0610049784E-314 ```

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

```java Bad theme={null} @SpringBootApplication public class MyApplication { ... } ``` ```java Fix theme={null} @Configuration @EnableAutoConfiguration 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.

```java Bad theme={null} protected void finalize() { // Noncompliant; no call to super.finalize(); releaseSomeResources(); } protected void finalize() { super.finalize(); // Noncompliant; this call should come last releaseSomeResources(); } ``` ```java Fix theme={null} protected void finalize() { releaseSomeResources(); super.finalize(); } ```

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.

```java Bad theme={null} public class MyClass { public void doSomething() { Lock lock = new Lock(); lock.lock(); // Noncompliant if (isInitialized()) { // ... lock.unlock(); } } } ``` ```java Fix theme={null} public class MyClass { public void doSomething() { Lock lock = new Lock(); if (isInitialized()) { lock.lock(); // ... 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.

```java Bad theme={null} @RequestMapping(path = "/profile", method = RequestMethod.GET) // Noncompliant public UserProfile getUserProfile(String name) { ... } ``` ```java Fix theme={null} ```

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.

```java Bad theme={null} 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); } ``` ```java Fix theme={null} if (dateString.matches("^\\d{1,2}([-/.])\\d{1,2}\\1\\d{1,4}$")) { String dateParts[] = dateString.split("[-/.]"); int day = Integer.parseInt(dateParts[0]); int month = Integer.parseInt(dateParts[1]); int year = Integer.parseInt(dateParts[2]); // Put logic to validate and process the date based on its integer parts here } ```

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.

```java Bad theme={null} public void foo() { while (condition1) { if (condition2) { continue; // Noncompliant } else { doTheThing(); } } return; // Noncompliant; this is a void method } ``` ```java Fix theme={null} public void foo() { while (condition1) { if (!condition2) { doTheThing(); } } } ```

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.

```java Bad theme={null} public class S1944 { public static void main(String[] args) { List list = (List) getAttributes(); // Noncompliant; List return by getAttributes() is not be casted to List String s = list.get(0); // java.lang.ClassCastException will be raised here } private static List getAttributes() { List result = new ArrayList<>(); result.add(0); return result; } } ``` ```java Fix theme={null} public class S1944 { public static void main(String[] args) { List list = (List) getAttributes(); // Compliant String s = String.valueOf(list.get(0)); } private static List getAttributes() { List 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.

```java Bad theme={null} SQLiteDatabase db = SQLiteDatabase.openOrCreateDatabase("test.db", getKey(), null); ``` ```java Fix theme={null} String masterKeyAlias = new MasterKeys.getOrCreate(MasterKeys.AES256_GCM_SPEC); EncryptedSharedPreferences.create( "secret", masterKeyAlias, context, EncryptedSharedPreferences.PrefKeyEncryptionScheme.AES256_SIV, EncryptedSharedPreferences.PrefValueEncryptionScheme.AES256_GCM ); ```

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.

```java Bad theme={null} Pattern.compile("(["); str.matches("(["); str.replaceAll("([", "{"); str.matches("(\\w+-(\\d+)"); ``` ```java Fix theme={null} Pattern.compile("\\(\\["); Pattern.compile("([", Pattern.LITERAL); str.equals("(["); str.replace("([", "{"); 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\`

```java Bad theme={null} new File("/myDirectory/myfile.txt"); // Compliant File.createTempFile("prefix", "suffix", new File("/mySecureDirectory")); // Compliant if(SystemUtils.IS_OS_UNIX) { FileAttribute> 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); } ``` ```java Fix theme={null} ```

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.

```java Bad theme={null} class Foo { // Noncompliant @Test void check() { } } class Bar { // Noncompliant @Nested class PositiveCase { @Test void check() { } } } ``` ```java Fix theme={null} class FooTest { @Test void check() { } } class BarIT { @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.

```java Bad theme={null} import android.webkit.WebView; WebView webView = (WebView) findViewById(R.id.webview); webView.getSettings().setAllowFileAccess(true); // Sensitive webView.getSettings().setAllowContentAccess(true); // Sensitive ``` ```java Fix theme={null} import android.webkit.WebView; WebView webView = (WebView) findViewById(R.id.webview); webView.getSettings().setAllowFileAccess(false); webView.getSettings().setAllowContentAccess(false); ```

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.

```java Bad theme={null} "(?:number)\\d{2}" ``` ```java Fix theme={null} "number\\d{2}" // it is anyway required "(?:number)?\\d{2}" // it is in fact optional ```

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.

```java Bad theme={null} "ab{1,1}c" "ab{1}c" "ab{0,0}c" "ab{0}c" ``` ```java Fix theme={null} "abc" "ac" ```

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 Bad theme={null} 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 ``` ```java Fix theme={null} ```

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.

```java Bad theme={null} if (((condition1 && condition2) || (condition3 && condition4)) && condition5) { ... } ``` ```java Fix theme={null} if ( (myFirstCondition() || mySecondCondition()) && myLastCondition()) { ... } ```

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.

```java Bad theme={null} @EnableWebSecurity public class WebSecurityConfig extends WebSecurityConfigurerAdapter { @Override protected void configure(HttpSecurity http) throws Exception { // http.csrf().disable(); // Compliant } } ``` ```java Fix theme={null} ```

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.

```java Bad theme={null} int Add(int a, int b) // Noncompliant; is ignored { return a + b; } ``` ```java Fix theme={null} int Add(int a, int b) { 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.

```java Bad theme={null} 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 ``` ```java Fix theme={null} ```

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.

```java Bad theme={null} 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 } ``` ```java Fix theme={null} float twoThirds = 2f/3; // 2 promoted to float. Yields 0.6666667 long millisInYear = 1_000L*3_600*24*365; // 1000 promoted to long. Yields 31_536_000_000 long bigNum = Integer.MAX_VALUE + 2L; // 2 promoted to long. Yields 2_147_483_649 long bigNegNum = Integer.MIN_VALUE-1L; // Yields -2_147_483_649 Date myDate = new Date(seconds * 1_000L); ... public long compute(int factor){ return factor * 10_000L; } public float compute2(long factor){ return factor / 123f; } ```

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.

```java Bad theme={null} @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"); } ``` ```java Fix theme={null} @CrossOrigin // Sensitive @RequestMapping("") public class TestController { public String home(ModelMap model) { model.addAttribute("message", "ok "); return "view"; } } ```

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.

```java Bad theme={null} for (int i = 1; i <= 10; i++) { // Noncompliant; two "continue" statements if (i % 2 == 0) { continue; } if (i % 3 == 0) { continue; } // ... } ``` ```java Fix theme={null} for (int i = 1; i <= 10; i++) { if (i % 2 == 0 || 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.

```java Bad theme={null} "a[b]c" "[\\^]" ``` ```java Fix theme={null} "abc" "\\^" "a[*]c" // Compliant, see Exceptions ```

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.

```java Bad theme={null} public long computeDurationInMilliseconds() { long duration = (((hours * 60) + minutes) * 60 + seconds) * 1000; return duration; } ``` ```java Fix theme={null} public long computeDurationInMilliseconds() { return (((hours * 60) + minutes) * 60 + seconds) * 1000; } ```

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.

```java Bad theme={null} 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; } ``` ```java Fix theme={null} private void called(int foo) { foo=1; 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.

```java Bad theme={null} try { /* ... */ } catch(Exception e) { e.printStackTrace(); // Sensitive } ``` ```java Fix theme={null} import org.springframework.context.annotation.Configuration; import org.springframework.security.config.annotation.web.configuration.EnableWebSecurity; @Configuration @EnableWebSecurity(debug = true) // Sensitive public class WebSecurityConfig extends WebSecurityConfigurerAdapter { // ... } ```

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.

```java Bad theme={null} class MyClass { public void doSomething() { System.out.println("My Message"); // Noncompliant, output directly to System.out without a logger } } ``` ```java Fix theme={null} import java.util.logging.Logger; class MyClass { Logger logger = Logger.getLogger(getClass().getName()); public void doSomething() { // ... logger.info("My Message"); // Compliant, output via 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.

```java Bad theme={null} public class TypeParameterHidesAnotherType { public class Inner { // Noncompliant //... } private T method() { // Noncompliant return null; } } ``` ```java Fix theme={null} public class NoTypeParameterHiding { public class Inner { // Compliant List listOfS; } private V method() { // Compliant 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.

```java Bad theme={null} public static void doSomething() { for (int i = 0; i < 4; i++) { // Noncompliant, 4 is a magic number ... } } ``` ```java Fix theme={null} public static final int NUMBER_OF_CYCLES = 4; public static void doSomething() { for (int i = 0; i < NUMBER_OF_CYCLES ; i++) { // Compliant ... } } ```

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.

```java Bad theme={null} class Foo { /** * @deprecated */ public void foo() { // Noncompliant } @Deprecated // Noncompliant public void bar() { } public void baz() { // Compliant } } ``` ```java Fix theme={null} ```

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.

```java Bad theme={null} int myNumber = 010; // Noncompliant. myNumber will hold 8, not 10 - was this really expected? ``` ```java Fix theme={null} int myNumber = 8; ```

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.

```java Bad theme={null} SecureRandom random = new SecureRandom(); // Compliant for security-sensitive use cases byte bytes[] = new byte[20]; random.nextBytes(bytes); ``` ```java Fix theme={null} ```

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.

```java Bad theme={null} private static final String MY_SECRET = "47828a8dd77ee1eb9dde2d5e93cb221ce8c32b37"; public static void main(String[] args) { MyClass.callMyService(MY_SECRET); } ``` ```java Fix theme={null} import software.amazon.awssdk.services.secretsmanager.model.GetSecretValueRequest; import software.amazon.awssdk.services.secretsmanager.model.GetSecretValueResponse; public static void main(String[] args) { SecretsManagerClient secretsClient = ... MyClass.doSomething(secretsClient, "MY_SERVICE_SECRET"); } public static void doSomething(SecretsManagerClient secretsClient, String secretName) { GetSecretValueRequest valueRequest = GetSecretValueRequest.builder() .secretId(secretName) .build(); GetSecretValueResponse valueResponse = secretsClient.getSecretValue(valueRequest); String secret = valueResponse.secretString(); // do something with the secret MyClass.callMyService(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.

```java Bad theme={null} public void doTheThing() { try { // ... catch (IOException e) { // Noncompliant } } ``` ```java Fix theme={null} public void doTheThing() throws IOException { // ... } ```

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.

```java Bad theme={null} class StringUtils { // Noncompliant public static String concatenate(String s1, String s2) { return s1 + s2; } } ``` ```java Fix theme={null} class StringUtils { // Compliant private StringUtils() { throw new IllegalStateException("Utility class"); } 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).

```java Bad theme={null} File f = new File("ZipBomb.zip"); ZipFile zipFile = new ZipFile(f); Enumeration 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; } } ``` ```java Fix theme={null} ```

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.

```java Bad theme={null} @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 } ``` ```java Fix theme={null} ```

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.

```java Bad theme={null} if (condition1) { if (condition2) { // Noncompliant /* ... */ } } ``` ```java Fix theme={null} if (condition1 && condition2) { // Compliant /* ... */ } ```

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.

```java Bad theme={null} public abstract class Animal { void speak() { // default implementation ignored } } ``` ```java Fix theme={null} public void shouldNotBeEmpty() { // Noncompliant - method is empty } public void notImplemented() { // Noncompliant - method is empty } @Override public void emptyOnPurpose() { // Noncompliant - method is empty } ```

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:

By default, the regular expressions ^ and $ignore line terminators and only match at the beginning and the end, respectively, of the entire input sequence. If MULTILINE mode is activated then ^ matches at the beginning of input and after any line terminator except at the end of input. When in MULTILINE mode$ matches just before a line terminator or the end of the input sequence.

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.

```java Bad theme={null} 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' ``` ```java Fix theme={null} static final Pattern p = Pattern.compile("^$", Pattern.MULTILINE); boolean containsEmptyLines(String str) { return p.matcher(str).find() || str.isEmpty(); } // ... System.out.println(containsEmptyLines("a\n\nb")); // correctly prints 'true' System.out.println(containsEmptyLines("")); // also correctly prints 'true' ```

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

```java Bad theme={null} long long1 = 1l; // Noncompliant float float1 = 1.0f; // Noncompliant double double1 = 1.0d; // Noncompliant ``` ```java Fix theme={null} long long1 = 1L; float float1 = 1.0F; double double1 = 1.0D; ```

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

```java Bad theme={null} Pattern.compile("(?=a)b"); // Noncompliant, the same character can't be equal to 'a' and 'b' at the same time ``` ```java Fix theme={null} Pattern.compile("(?<=a)b"); Pattern.compile("a(?=b)"); ```

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:

```java Bad theme={null} TelnetClient telnet = new TelnetClient(); // Sensitive FTPClient ftpClient = new FTPClient(); // Sensitive SMTPClient smtpClient = new SMTPClient(); // Sensitive ``` ```java Fix theme={null} ConnectionSpec spec = new ConnectionSpec.Builder(ConnectionSpec.CLEARTEXT) // Sensitive .build(); ```

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.

```java Bad theme={null} Cookie c = new Cookie(COOKIENAME, sensitivedata); c.setHttpOnly(true); // Compliant: this sensitive cookie is protected against theft (HttpOnly=true) ``` ```java Fix theme={null} ```

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.

```java Bad theme={null} int matrix[][]; // Noncompliant int[] matrix[]; // Noncompliant ``` ```java Fix theme={null} int[][] matrix; // Compliant ```

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.

```java Bad theme={null} public class MyClass { private int foo = 42; // Noncompliant: foo is unused and should be removed public int compute(int a) { return a * 42; } } ``` ```java Fix theme={null} public class MyClass implements java.io.Serializable { private static final long serialVersionUID = 42L; // Compliant by exception } ```

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.

```java Bad theme={null} 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. } } ``` ```java Fix theme={null} class Example { public void example(List list) { for (String item: (List) list) { // Noncompliant, Remove this unnecessary cast to "List". //... } } } ```

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}\`

```java Bad theme={null} "[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*" ``` ```java Fix theme={null} "\\d" "\\D" "\\w" "." "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.

```java Bad theme={null} 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); ``` ```java Fix theme={null} ```

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.

```java Bad theme={null} 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; /* ... */ } ``` ```java Fix theme={null} switch (day) { case MONDAY: case TUESDAY: case WEDNESDAY: doSomething(); break; ... } switch (day) { case MONDAY: break; case TUESDAY: compute(args); // put the content of the labelled "for" statement in a dedicated method 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.

```java Bad theme={null} 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 ``` ```java Fix theme={null} float f; if(f != f) { //test for NaN value System.out.println("f is NaN"); } int i = 1 << 1; // Compliant int j = a << a; // Noncompliant ```

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.

```java Bad theme={null} 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"); } ``` ```java Fix theme={null} 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.

```java Bad theme={null} @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(); } ``` ```java Fix theme={null} @Bean(name = "multipartResolver") public CommonsMultipartResolver multipartResolver() { multipartResolver.setMaxUploadSize(8388608); // Compliant (8 MB) return multipartResolver; } ```

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.

```java Bad theme={null} void processInput(BufferedReader br) { String line; while ((line = br.readLine()) != null) { processLine(line); } } Object foo; if ((foo = bar()) != null) { // do something with "foo" } ``` ```java Fix theme={null} int j, i = j = 0; int k = (j += 1); byte[] result, bresult; result = (bresult = new byte[len]); ```

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.

```java Bad theme={null} if (param == 1) openWindow(); else if (param == 2) closeWindow(); else if (param == 1) // Noncompliant moveWindowToTheBackground(); } ``` ```java Fix theme={null} if (param == 1) openWindow(); else if (param == 2) closeWindow(); else if (param == 3) 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.

```java Bad theme={null} // This can never match because $ and ^ have been switched around Pattern.compile("$[a-z]+^"); // Noncompliant ``` ```java Fix theme={null} Pattern.compile("^[a-z]+$"); ```

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.

```java Bad theme={null} 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 // ... ``` ```java Fix theme={null} ```

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.

```java Bad theme={null} void doSomething(int a, int b) { // Noncompliant, "b" is unused compute(a); } ``` ```java Fix theme={null} void doSomething(int a) { 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.

```java Bad theme={null} public class MyClass { public static final int SOME_CONSTANT = 0; // Compliant - constants are not checked public String firstName; // Noncompliant } ``` ```java Fix theme={null} public class MyClass { public static final int SOME_CONSTANT = 0; // Compliant - constants are not checked private String firstName; public String getFirstName() { return firstName; } public void setFirstName(String firstName) { this.firstName = firstName; } } ```

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.

```java Bad theme={null} import android.content.Context; public class AccessExternalFiles { public void accessFiles(Context context) { context.getFilesDir(); } } ``` ```java Fix theme={null} ```

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.

```java Bad theme={null} "cc̈d̈d".replaceAll("[c̈d̈]", "X"); // Noncompliant, print "XXXXXX" instead of expected "cXXd". ``` ```java Fix theme={null} "cc̈d̈d".replaceAll("c̈|d̈", "X"); // print "cXXd" ```

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

```java Bad theme={null} package org.exAmple; // Noncompliant ``` ```java Fix theme={null} package org.example; ```

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.

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

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.

```java Bad theme={null} public class Foo { public Collection listUsers() { File userList = new File("/home/mylogin/Dev/users.txt"); // Noncompliant Collection users = parse(userList); return users; } } ``` ```java Fix theme={null} public class Foo { // Configuration is a class that returns customizable properties: it can be mocked to be injected during tests. private Configuration config; public Foo(Configuration myConfig) { this.config = myConfig; } public Collection listUsers() { // Find here the way to get the correct folder, in this case using the Configuration object String listingFolder = config.getProperty("myApplication.listingFolder"); // and use this parameter instead of the hard coded path File userList = new File(listingFolder, "users.txt"); // Compliant Collection 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.

```java Bad theme={null} if (x == 0) { doSomething(); } else if (x == 1) { doSomethingElse(); } ``` ```java Fix theme={null} if (x == 0) { doSomething(); } else if (x == 1) { doSomethingElse(); } else { throw new IllegalStateException(); } ```

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: \) 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:

\link\ // myLink = javascript:alert(document.cookie)
        \link\ // JS injection (XSS attack)
```java Bad theme={null} Mustache.compiler().compile(template).execute(context); // Compliant, auto-escaping is enabled by default Mustache.compiler().escapeHTML(true).compile(template).execute(context); // Compliant ``` ```java Fix theme={null} freemarker.template.Configuration configuration = new freemarker.template.Configuration(); configuration.setAutoEscapingPolicy(ENABLE_IF_DEFAULT_AUTO_ESCAPING_POLICY); // 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.

```java Bad theme={null} Pattern.compile("hello world"); ``` ```java Fix theme={null} Pattern.compile("hello {3}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.

```java Bad theme={null} protected void doGet(HttpServletRequest req, HttpServletResponse resp) throws IOException { String value = req.getParameter("value"); resp.addHeader("X-Header", value); // Noncompliant } ``` ```java Fix theme={null} protected void doGet(HttpServletRequest req, HttpServletResponse resp) throws IOException { String value = req.getParameter("value"); String whitelist = "safevalue1 safevalue2"; if (!whitelist.contains(value)) throw new IOException(); resp.addHeader("X-Header", value); // Compliant } ```

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.

```java Bad theme={null} 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); } } ``` ```java Fix theme={null} ```

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.

```java Bad theme={null} 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 ``` ```java Fix theme={null} ```

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.

```java Bad theme={null} void foo() { outer: //label is not used. for(int i = 0; i<10; i++) { break; } } ``` ```java Fix theme={null} void foo() { 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.

```java Bad theme={null} // ... BiometricPrompt biometricPrompt = new BiometricPrompt(activity, executor, callback); // ... biometricPrompt.authenticate(promptInfo); // Noncompliant ``` ```java Fix theme={null} // ... BiometricPrompt biometricPrompt = new BiometricPrompt(activity, executor, callback); // ... biometricPrompt.authenticate(promptInfo, new BiometricPrompt.CryptoObject(cipher)); // Compliant ```

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.

```java Bad theme={null} String date = "01/02"; Pattern datePattern = Pattern.compile("(?[0-9]{2})/(?[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("(?[0-9]+):(?[0-9]+)"); // Noncompliant - named groups are never used Matcher scoreMatcher = scorePattern.matcher(score); if (scoreMatcher.matches()) { checkScore(score); } ``` ```java Fix theme={null} String date = "01/02"; Pattern datePattern = Pattern.compile("(?[0-9]{2})/(?[0-9]{2})"); Matcher dateMatcher = datePattern.matcher(date); if (dateMatcher.matches()) { checkValidity(dateMatcher.group("month"), dateMatcher.group("year")); } // ... String score = "14:1"; Pattern scorePattern = Pattern.compile("(?[0-9]+):(?[0-9]+)"); Matcher scoreMatcher = scorePattern.matcher(score); if (scoreMatcher.matches()) { checkScore(scoreMatcher.group("player1")); checkScore(scoreMatcher.group("player2")); } ```

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.

```java Bad theme={null} 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; } } } } ``` ```java Fix theme={null} if (!condition1) { return; } /* ... */ if (!condition2) { return; } for (int i = 0; i < 10; i++) { /* ... */ if (condition4) { if (condition5) { /* ... */ } 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.

```java Bad theme={null} if (x < 0) new IllegalArgumentException("x must be nonnegative"); ``` ```java Fix theme={null} if (x < 0) throw 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 \ - include::\{example}\[]

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

Unresolved directive in \ - include::\{compliant}\[]

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

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.

```java Bad theme={null} public void doSomething() { super.doSomething(); } @Override public boolean isLegal(Action action) { return super.isLegal(action); } ``` ```java Fix theme={null} @Override public boolean isLegal(Action action) { // Compliant - not simply forwarding the call return super.isLegal(new Action(/* ... */)); } @Id @Override public int getId() { // Compliant - there is annotation different from @Override return super.getId(); } ```

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

```java Bad theme={null} 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 } ``` ```java Fix theme={null} private static final String ACTION_1 = "action1"; // Compliant public void run() { prepare(ACTION_1); // Compliant execute(ACTION_1); release(ACTION_1); } ```

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.

```java Bad theme={null} 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 } ``` ```java Fix theme={null} ```

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.

```java Bad theme={null} 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; ... } } ``` ```java Fix theme={null} public class Foo { public void doSomething(int y) { int a = y + 5; ... if(a == 0) { ... } } public void doSomethingElse(int y) { int b = y + 3; ... } } ```