Original	Preferred
stream.collect(counting())	stream.count()
stream.collect(maxBy(comparator))	stream.max(comparator)
stream.collect(minBy(comparator))	stream.min(comparator)
stream.collect(mapping(mapper))	stream.map(mapper).collect()
stream.collect(reducing(...))	stream.reduce(...)
stream.collect(summingInt(mapper))	stream.mapToInt(mapper).sum()
stream.collect(summingLong(mapper))	stream.mapToLong(mapper).sum()
stream.collect(summingDouble(mapper))	stream.mapToDouble(mapper).sum()\`

```java Bad theme={null} int count = stream.collect(counting()); // Noncompliant ``` ```java Fix theme={null} int count = stream.count(); ```

Synchronization can be expensive in terms of time when multiple threads need to pass through the same bottleneck /\`synchronized piece of code.

If you have a piece of code calling a synchronized method once, then it only has to wait its turn to pass through the bottleneck once. But call it in a loop, and your code has to get back in line for the bottleneck over and over.

Instead, it would be better to get into the bottleneck, and then do the looping. I.e. consider refactoring the code to perform the loop inside the synchronized method.

This rule raises an issue when a synchronized\` method is called in a loop.

```java Bad theme={null} public void doSomething(int max) { for (int i = 0; i < max; i++) { doSynchronized(i); // Noncompliant } } public synchronized void doSynchronized(int val) { // ... } ``` ```java Fix theme={null} ```

It can be useful to use in-code notation to suppress issues, but when those suppressions are no longer relevant they become a potential source of confusion and should be removed.

```java Bad theme={null} @SuppressWarnings("squid:S4174") // Noncompliant public void doSomething() { final int LOCAL = 42; // S4174 is about naming of local constants but there's nothing wrong here ``` ```java Fix theme={null} public void doSomething() { final int LOCAL = 42; // S4174 is about naming of local constants but there's nothing wrong here ```

Optional acts as a container object that may or may not contain a non-null value. It is introduced in Java 8 to help avoid NullPointerException. It provides methods to check if a value is present and retrieve the value if it is present.

Optional is used instead of null values to make the code more readable and avoid potential errors.

It is a bad practice to use null with Optional because it is unclear whether a value is present or not, leading to confusion and potential NullPointerException errors.

```java Bad theme={null} public void doSomething () { Optional optional = getOptional(); if (optional != null) { // Noncompliant // do something with optional... } Optional text = null; // Noncompliant, a variable whose type is Optional should never itself be null // ... } @Nullable // Noncompliant public Optional getOptional() { // ... return null; // Noncompliant } ``` ```java Fix theme={null} public void doSomething () { Optional optional = getOptional(); optional.ifPresent( // do something with optional... ); Optional text = Optional.empty(); // ... } public Optional getOptional() { // ... return Optional.empty(); } ```

Fields, parameters and return values marked `@NotNull, @NonNull, or @Nonnull are assumed to have non-null values and are not typically null-checked before use. Therefore setting one of these values to null, or failing to set such a class field in a constructor, could cause NullPointerException`s at runtime.

```java Bad theme={null} public class MainClass { @Nonnull private String primary; private String secondary; public MainClass(String color) { if (color != null) { secondary = null; } primary = color; // Noncompliant; "primary" is Nonnull but could be set to null here } public MainClass() { // Noncompliant; "primary" is Nonnull but is not initialized } @Nonnull public String indirectMix() { String mix = null; return mix; // Noncompliant; return value is Nonnull, but null is returned. } ``` ```java Fix theme={null} ```

When a reluctant quantifier (such as \`\*? or +?) is followed by a pattern that can match the empty string or directly by the end of the regex, it will always match the empty string when used with methods that find partial matches (such as find, replaceAll, split etc.).

Similarly, when used with methods that find full matches, a reluctant quantifier that’s followed directly by the end of the regex (or a pattern that always matches the empty string, such as ()\`) behaves indistinguishably from a greedy quantifier while being less efficient.

This is likely a sign that the regex does not work as intended.

```java Bad theme={null} "start123endstart456".replaceAll("start\\w*?(end)?", "x"); // Noncompliant. In contrast to what one would expect, the result is not "xx". str.matches("\\d*?"); // Noncompliant. Matches the same as "\d*", but will backtrack in every position. ``` ```java Fix theme={null} "start123endstart456".replaceAll("start\\w*?(end|$)", "x"); // Result is "xx". str.matches("\\d*"); ```

A non-serializable Comparator can prevent an otherwise-Serializable ordered collection from being serializable. Since the overhead to make a Comparator serializable is usually low, doing so can be considered good defensive programming.

```java Bad theme={null} public class FruitComparator implements Comparator { // Noncompliant int compare(Fruit f1, Fruit f2) {...} boolean equals(Object obj) {...} } ``` ```java Fix theme={null} public class FruitComparator implements Comparator, Serializable { private static final long serialVersionUID = 1; int compare(Fruit f1, Fruit f2) {...} boolean equals(Object obj) {...} } ```

In Java, numeric promotions happen when two operands of an arithmetic expression have different sizes. More specifically, narrower operands get promoted to the type of wider operands. For instance, an operation between a byte and an int, will trigger a promotion of the byte operand, converting it into an int.

When this happens, the sequence of 8 bits that represents the byte will need to be extended to match the 32-bit long sequence that represents the int operand. Since Java uses two’s complement notation for signed number types, the promotion will fill the missing leading bits with zeros or ones, depending on the sign of the value. For instance, the byte 0b1000\_0000 (equal to -128 in decimal notation), when promoted to int, will become 0b1111\_1111\_1111\_1111\_1111\_1111\_1000\_0000.

When performing shifting or bitwise operations without considering that bytes are signed, the bits added during the promotion may have unexpected effects on the final result of the operations.

```java Bad theme={null} public static void main(String[] args) { byte[] bytes12 = BigInteger.valueOf(12).toByteArray(); // This byte array will be simply [12] System.out.println(intFromBuffer(bytes12)); // In this case, the bytes promotion will not cause any issues, and "12" will be printed. // Here the bytes will be [2, -128] since 640 in binary is represented as 0b0000_0010_1000_0000 // which is equivalent to the concatenation of 2 bytes: 0b0000_0010 = 2, and 0b1000_0000 = -128 byte[] bytes640 = BigInteger.valueOf(640).toByteArray(); // In this case, the shifting operation combined with the bitwise OR, will produce the wrong binary string and "-128" will be printed. System.out.println(intFromBuffer(bytes640)); } static int intFromBuffer(byte[] bytes) { int originalInt = 0; for (int i = 0; i < bytes.length; i++) { // Here the right operand of the bitwise OR, which is a byte, will be promoted to an `int` // and if its value was negative, the added ones in front of the binary string will alter the value of the `originalInt` originalInt = (originalInt << 8) | bytes[i]; // Noncompliant } return originalInt; } ``` ```java Fix theme={null} public static void main(String[] args) { byte[] bytes12 = BigInteger.valueOf(12).toByteArray(); // This byte array will be simply [12] System.out.println(intFromBuffer(bytes12)); // In this case, the bytes promotion will not cause any issues, and "12" will be printed. // Here the bytes will be [2, -128] since 640 in binary is represented as 0b0000_0010_1000_0000 // which is equivalent to the concatenation of 2 bytes: 0b0000_0010 = 2, and 0b1000_0000 = -128 byte[] bytes640 = BigInteger.valueOf(640).toByteArray(); // This will correctly print "640" now. System.out.println(intFromBuffer(bytes640)); } static int intFromBuffer(byte[] bytes) { int originalInt = 0; for (int i = 0; i < bytes.length; i++) { originalInt = (originalInt << 8) | Byte.toUnsignedInt(bytes[i]); // Compliant, only the relevant 8 least significant bits will affect the bitwise OR } return originalInt; } ```

Naming conventions play a crucial role in maintaining code clarity and readability. The uniqueness of bean names in Spring configurations is vital to the clarity and readability of the code. When two beans share the same name within a configuration, it is not obvious to the reader which bean is being referred to. This leads to potential misunderstandings and errors.

```java Bad theme={null} @Configuration class Config { @Bean public User user() { return currentUser(); } @Bean public User user(AuthService auth) { // Noncompliant return auth.user(); } } ``` ```java Fix theme={null} @Configuration class Config { @Bean public User user() { return currentUser(); } @Bean public User userFromAuth(AuthService auth) { return auth.user(); } } ```

When @Overrides of synchronized methods are not themselves synchronized, the result can be improper synchronization as callers rely on the thread-safety promised by the parent class.

```java Bad theme={null} public class Parent { synchronized void foo() { //... } } public class Child extends Parent { @Override public void foo () { // Noncompliant // ... super.foo(); } } ``` ```java Fix theme={null} public class Parent { synchronized void foo() { //... } } public class Child extends Parent { @Override synchronized void foo () { // ... super.foo(); } } ```

In a multithreaded environment, the Object.wait(…), as well as Condition.await(…) and similar methods are used to pause the execution of a thread until the thread is awakened. A thread is typically awakened when it is notified, signaled, or interrupted, usually because of an event in another thread requiring some subsequent action by the waiting thread.

However, a thread may be awakened despite the desired condition not being met or the desired event not having happened. This is referred to as "spurious wakeups" and may be caused by underlying platform semantics. In other words, a thread may be awakened due to reasons that have nothing to do with the business logic. Hence, the assumption that the desired condition is met or the desired event occurred after a thread is awakened does not always hold.

According to the documentation of the Java Condition interface \[1]:

When waiting upon a Condition, a "spurious wakeup" is permitted to occur, in general, as a concession to the underlying platform semantics. This has little practical impact on most application programs as a Condition should always be waited upon in a loop, testing the state predicate that is being waited for. An implementation is free to remove the possibility of spurious wakeups but it is recommended that applications programmers always assume that they can occur and so always wait in a loop.

The same advice is also found for the Object.wait(…) method \[2]:

\[…] waits should always occur in loops, like this one:

synchronized (obj) \{
            while (\)\{
            obj.wait(timeout);
            }
            ... // Perform action appropriate to condition
            }

```java Bad theme={null} synchronized (obj) { if (!suitableCondition()){ obj.wait(timeout); // Noncompliant, the thread can be awakened even though the condition is still false } ... // Perform some logic that is appropriate for when the condition is true } ``` ```java Fix theme={null} synchronized (obj) { while (!suitableCondition()){ obj.wait(timeout); // Compliant, the condition is checked in a loop, so the action below will only occur if the condition is true } ... // Perform some logic that is appropriate for when the condition is true } ```

The use of unnecessary types makes the eye stumble, and inhibits the smooth reading of code.

```java Bad theme={null} public void doSomething() { foo("blah"); // Noncompliant; is inferred } public void foo(T t) { // ... } ``` ```java Fix theme={null} public void doSomething() { foo("blah"); } public void foo(T t) { // ... } ```

The underlying implementation of \`String::replaceAll calls the java.util.regex.Pattern.compile() method each time it is called even if the first argument is not a regular expression. This has a significant performance cost and therefore should be used with care.

When String::replaceAll is used, the first argument should be a real regular expression. If it’s not the case, String::replace does exactly the same thing as String::replaceAll without the performance drawback of the regex.

This rule raises an issue for each String::replaceAll used with a String\` as first parameter which doesn’t contains special regex character or pattern.

```java Bad theme={null} String init = "Bob is a Bird... Bob is a Plane... Bob is Superman!"; String changed = init.replaceAll("Bob is", "It's"); // Noncompliant changed = changed.replaceAll("\\.\\.\\.", ";"); // Noncompliant ``` ```java Fix theme={null} String init = "Bob is a Bird... Bob is a Plane... Bob is Superman!"; String changed = init.replace("Bob is", "It's"); changed = changed.replace("...", ";"); ```

Before it reclaims storage from an object that is no longer referenced, the garbage collector calls finalize() on the object.

But there is no guarantee that this method will be called as soon as the last references to the object are removed.

It can be few microseconds to few minutes later.

For this reason relying on overriding the finalize() method to release resources or to update the state of the program is highly discouraged.

```java Bad theme={null} public class MyClass { @Override protected void finalize() { // Noncompliant releaseSomeResources(); } } ``` ```java Fix theme={null} ```

Creating an object for the sole purpose of calling getClass on it is a waste of memory and cycles. Instead, simply use the class’s .class property.

```java Bad theme={null} MyObject myOb = new MyObject(); // Noncompliant Class c = myOb.getClass(); ``` ```java Fix theme={null} Class c = MyObject.class; ```

Thread.yield is intended to hint to the processor that the current thread is willing to suspended in favor of another thread. Unfortunately, it doesn’t have the same results across platforms, thus marring the cross-platform compatibility of any application that uses it.

```java Bad theme={null} public void doSomething() { // ... Thread.yield(); // Noncompliant // ... } ``` ```java Fix theme={null} ```

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 used instead of workaround implementations that were necessary prior to Java 21.

This rule identifies instances where a workaround is used to add or remove the first or last element of a collection where the addFirst, addLast, removeFirst or removeLast method in the java.util.SequencedCollection class should have been used instead.

```java Bad theme={null} void push(List list, String element) { list.add(list.size()-1, element); // Noncompliant } ``` ```java Fix theme={null} void push(List list, String element) { list.addLast(element); // Compliant } ```

instanceof operators that always return true or false are either useless or the result of a misunderstanding which could lead to unexpected behavior in production.

```java Bad theme={null} public boolean isSuitable(Integer param) { ... String name = null; if (name instanceof String) { // Noncompliant; always false since name is null //... } if(param instanceof Number) { // Noncompliant; always true unless param is null, because param is an Integer doSomething(); } ... } ``` ```java Fix theme={null} public boolean isSuitable(Integer param) { ... doSomething(); ... } ```

Shared coding conventions allow teams to collaborate efficiently. This rule checks that all local, final, initialized, primitive variables, have names that match a provided regular expression.

```java Bad theme={null} public void doSomething() { final int local = 42; ... } ``` ```java Fix theme={null} public void doSomething() { final int LOCAL = 42; ... } ```

A Spring @Controller that uses @SessionAttributes is designed to handle a stateful / multi-post form. Such @Controllers use the specified @SessionAttributes to store data on the server between requests. That data should be cleaned up when the session is over, but unless setComplete() is called on the SessionStatus object from a @RequestMapping method, neither Spring nor the JVM will know it’s time to do that. Note that the SessionStatus object must be passed to that method as a parameter.

```java Bad theme={null} @Controller @SessionAttributes("hello") // Noncompliant; this doesn't get cleaned up public class HelloWorld { @RequestMapping("/greet", method = GET) public String greet(String greetee) { return "Hello " + greetee; } } ``` ```java Fix theme={null} @Controller @SessionAttributes("hello") public class HelloWorld { @RequestMapping("/greet", method = GET) public String greet(String greetee) { return "Hello " + greetee; } @RequestMapping("/goodbye", method = POST) public String goodbye(SessionStatus status) { //... status.setComplete(); } } ```

Spring \`@Controllers, @Services, and @Repositorys have singleton scope by default, meaning only one instance of the class is ever instantiated in the application. Defining any other scope for one of these class types will result in needless churn as new instances are created and destroyed. In a busy web application, this could cause a significant amount of needless additional load on the server.

This rule raises an issue when the @Scope annotation is applied to a @Controller, @Service, or @Repository with any value but "singleton". @Scope("singleton")\` is redundant, but ignored.

```java Bad theme={null} @Scope("prototype") // Noncompliant @Controller public class HelloWorld { ``` ```java Fix theme={null} @Controller public class HelloWorld { ```

Java 8 introduced \`ThreadLocal.withInitial which is a simpler alternative to creating an anonymous inner class to initialise a ThreadLocal instance.

This rule raises an issue when a ThreadLocal anonymous inner class can be replaced by a call to ThreadLocal.withInitial\`.

```java Bad theme={null} ThreadLocal> myThreadLocal = new ThreadLocal>() { // Noncompliant @Override protected List initialValue() { return new ArrayList(); } }; ``` ```java Fix theme={null} ThreadLocal> myThreadLocal = ThreadLocal.withInitial(ArrayList::new); ```

java.util.concurrent.locks.Lock offers far more powerful and flexible locking operations than are available with synchronized blocks. So synchronizing on a Lock instance throws away the power of the object, as it overrides its better locking mechanisms. Instead, such objects should be locked and unlocked using one of their lock and unlock method variants.

```java Bad theme={null} Lock lock = new MyLockImpl(); synchronized(lock) { // Noncompliant // ... } ``` ```java Fix theme={null} Lock lock = new MyLockImpl(); if (lock.tryLock()) { try { // ... } finally { lock.unlock(); } } ```

Floating point math is imprecise because of the challenges of storing such values in a binary representation. Even worse, floating point math is not associative; push a \`float or a double through a series of simple mathematical operations and the answer will be different based on the order of those operation because of the rounding that takes place at each step.

Even simple floating point assignments are not simple:

float f = 0.1; // 0.100000001490116119384765625
        double d = 0.1; // 0.1000000000000000055511151231257827021181583404541015625

(Results will vary based on compiler and compiler settings);

Therefore, the use of the equality (==) and inequality (!=) operators on float or double values is almost always an error. Instead the best course is to avoid floating point comparisons altogether. When that is not possible, you should consider using one of Java’s float-handling Numbers such as BigDecimal which can properly handle floating point comparisons. A third option is to look not for equality but for whether the value is close enough. I.e. compare the absolute value of the difference between the stored value and the expected value against a margin of acceptable error. Note that this does not cover all cases (NaN and Infinity\` for instance).

This rule checks for the use of direct and indirect equality/inequailty tests on floats and doubles.

```java Bad theme={null} float myNumber = 3.146; if ( myNumber == 3.146f ) { //Noncompliant. Because of floating point imprecision, this will be false // ... } if ( myNumber != 3.146f ) { //Noncompliant. Because of floating point imprecision, this will be true // ... } if (myNumber < 4 || myNumber > 4) { // Noncompliant; indirect inequality test // ... } float zeroFloat = 0.0f; if (zeroFloat == 0) { // Noncompliant. Computations may end up with a value close but not equal to zero. } ``` ```java Fix theme={null} float f; double d; if(f != f) { // Compliant; test for NaN value System.out.println("f is NaN"); } else if (f != d) { // Noncompliant // ... } ```

Persistence annotations should be marked either on fields or on getters but not on both. Mix the two and the annotated fields will be ignored. The potential results are that your database tables are not created or not populated (and read) correctly.

```java Bad theme={null} @Entity public class Person { // Noncompliant; both fields and getters annotated @Id private Long id; private String fname; public Long getId() { ... } @Column(name="name") public String getFname() { ... } ``` ```java Fix theme={null} @Entity public class Person { @Id private Long id; @Column(name="name") private String fname; public Long getId() { ... } public String getFname() { ... } ```

AssertJ contains many assertions methods specific to common types. Both versions will test the same things, but the dedicated one will provide a better error message, simplifying the debugging process.

This rule reports an issue when an assertion can be simplified to a dedicated one.

The array below gives a non-exhaustive list of assertion reported by the rule. Code behaving similarly, or with a negation will also be reported.

Original	Dedicated
Related to Object
\`assertThat(getObject()).isEqualTo(null)	assertThat(getObject()).isNull()
assertThat(getBoolean()).isEqualTo(true)	assertThat(getBoolean()).isTrue()
assertThat(getBoolean()).isEqualTo(false)	assertThat(getBoolean()).isFalse()
assertThat(x.equals(y)).isTrue()	assertThat(x).isEqualTo(y)

Original

Dedicated

Related to Object

\`assertThat(getObject()).isEqualTo(null)

assertThat(getObject()).isNull()

assertThat(getBoolean()).isEqualTo(true)

assertThat(getBoolean()).isTrue()

assertThat(getBoolean()).isEqualTo(false)

assertThat(getBoolean()).isFalse()

assertThat(x.equals(y)).isTrue()

assertThat(x).isEqualTo(y)

```java Bad theme={null} assertThat(getObject()).isEqualTo(null); // Noncompliant assertThat(getObject()).isNotEqualTo(null); // Noncompliant - not listed above but also supported assertThat(getString().trim()).isEmpty(); assertThat(getFile().canRead()).isTrue(); assertThat(getPath().getParent()).isNull(); ``` ```java Fix theme={null} assertThat(getObject()).isNull(); assertThat(getObject()).isNotNull(); assertThat(getString()).isBlank(); assertThat(getFile()).canRead(); assertThat(getPath()).hasNoParentRaw(); ```

@Configuration is a class-level annotation indicating that an object is a source of bean definitions. @Configuration classes declare beans through @Bean-annotated methods. Calls to @Bean methods on @Configuration classes can also be used to define inter-bean dependencies. The @Bean annotation indicates that a method instantiates, configures, and initializes a new object to be managed by the Spring IoC container.

Annotating a method of a bean with @Async will make it execute in a separate thread. In other words, the caller will not wait for the completion of the called method.

The @Async annotation is not supported on methods declared within a @Configuration class. This is because @Async methods are typically used for asynchronous processing, and they require certain infrastructure to be set up, which may not be available or appropriate in a @Configuration class.

```java Bad theme={null} @EnableAsync @Configuration public class MyConfiguration { @Async // Noncompliant - This is not allowed public void asyncMethod() { // ... } } ``` ```java Fix theme={null} @EnableAsync @Configuration public class MyConfiguration { public void method() { // ... } } ```

The purpose of the hashCode method is to return a hash code based on the contents of the object. Similarly, the purpose of the toString method is to provide a textual representation of the object’s contents.

Calling hashCode() and toString() directly on array instances should be avoided because the default implementations provided by the Object class do not provide meaningful results for arrays. hashCode() returns the array’s "identity hash code", and toString() returns nearly the same value. Neither method’s output reflects the array’s contents.

```java Bad theme={null} public static void main(String[] args) { String argStr = args.toString(); // Noncompliant int argHash = args.hashCode(); // Noncompliant } ``` ```java Fix theme={null} public static void main(String[] args) { String argStr = Arrays.toString(args); // Compliant int argHash = Arrays.hashCode(args); // Compliant } ```

When boxed type java.lang.Boolean is used as an expression to determine the control flow (as described in Java Language Specification §4.2.5 The boolean Type and boolean Values) it will throw a NullPointerException if the value is null (as defined in Java Language Specification §5.1.8 Unboxing Conversion).

It is safer to avoid such conversion altogether and handle the null value explicitly.

Note, however, that no issues will be raised for Booleans that have already been null-checked.

```java Bad theme={null} Boolean b = getBoolean(); if (b) { // Noncompliant, it will throw NPE when b == null foo(); } else { bar(); } ``` ```java Fix theme={null} Boolean b = getBoolean(); if (Boolean.TRUE.equals(b)) { foo(); } else { bar(); // will be invoked for both b == false and b == null } Boolean b = getBoolean(); if(b != null){ String test = b ? "test" : ""; } ```

The java.util.Iterator.next() method must throw a NoSuchElementException when there are no more elements in the iteration. Any other behavior is non-compliant with the API contract and may cause unexpected behavior for users.

```java Bad theme={null} public class MyIterator implements Iterator { public String next() { if (!hasNext()) { return null; } // ... } } ``` ```java Fix theme={null} public class MyIterator implements Iterator { public String next() { if (!hasNext()) { throw new NoSuchElementException(); } // ... } } ```

A serialVersionUID field is required in a Serializable class. In a non-Serializable, it’s just confusing.

```java Bad theme={null} public class MyClass { private static final long serialVersionUID = -1L; // Noncompliant } ``` ```java Fix theme={null} ```

Method references are specialized lambda expressions for methods that already have a name. They take the form of objectReference::methodName or ClassName::methodName. When a lambda does nothing but call an existing method, it can be much clearer to use a method reference instead.

```java Bad theme={null} Set roster = transferElements(rosterSource, () -> { return new HashSet<>(); } // Noncompliant ); ``` ```java Fix theme={null} Set roster = transferElements(rosterSource, HashSet::new); ```

Recursion is a technique to solve a computational problem by splitting it into smaller problems. A method is recursive, if it splits its input into smaller instances and calls itself on these instances. This continues until a smallest input, a base case, is reached that can not be split further. Similarly, recursion can also occur when multiple methods invoke each other.

Recursion is a useful tool, but it must be used carefully. Recursive methods need to detect base cases and end recursion with a return statement. When this is not the case, recursion will continue until the stack overflows and the program crashes due to a StackOverflowError.

```java Bad theme={null} int myPow(int num, int exponent) { num = num * myPow(num, exponent - 1); // Noncompliant: myPow unconditionally calls itself. return num; // this is never reached } ``` ```java Fix theme={null} int myPow(int num, int exponent) { if (exponent == 0) { // <- termination condition return 1; } if (exponent < 0) { throw new IllegalArgumentException("Negative exponents are not supported."); } num = num * myPow(num, exponent - 1); return num; } ```

Just as there is little justification for writing your own String class, there is no good reason to re-define one of the existing, standard functional interfaces.

Doing so may seem tempting, since it would allow you to specify a little extra context with the name. But in the long run, it will be a source of confusion, because maintenance programmers will wonder what is different between the custom functional interface and the standard one.

```java Bad theme={null} @FunctionalInterface public interface MyInterface { // Noncompliant double toDouble(int a); } @FunctionalInterface public interface ExtendedBooleanSupplier { // Noncompliant boolean get(); default boolean isFalse() { return !get(); } } public class MyClass { private int a; public double myMethod(MyInterface instance){ return instance.toDouble(a); } } ``` ```java Fix theme={null} @FunctionalInterface public interface ExtendedBooleanSupplier extends BooleanSupplier { // Compliant, extends java.util.function.BooleanSupplier default boolean isFalse() { return !getAsBoolean(); } } public class MyClass { private int a; public double myMethod(IntToDoubleFunction instance){ return instance.applyAsDouble(a); } } ```

The use of Unicode escape sequences should be reserved for characters that would otherwise be ambiguous, such as unprintable characters.

This rule ignores sequences composed entirely of Unicode characters, but otherwise raises an issue for each Unicode character that represents a printable character.

```java Bad theme={null} String prefix = "n\u00E9e"; // Noncompliant ``` ```java Fix theme={null} String prefix = "née"; ```

The use of factory methods lets you abstract the job you need to do from the specific tool implementations needed to do it with, and helps insulate you from changes.

This rule raises an issue when instances of these are instantiated directly:

\`javax.xml.parsers.DocumentBuilder
javax.xml.parsers.SAXParser
javax.xml.transform.Transformer
org.xml.sax.XMLReader
org.xml.sax.XMLFilter
org.w3c.dom.\*\`

```java Bad theme={null} DocumentBuilder builder = new DocumentBuilderImpl(); // Noncompliant ``` ```java Fix theme={null} DocumentBuilderFactory factory = DocumentBuilderFactory.newInstance(); DocumentBuilder builder = factory.newDocumentBuilder(); ```

Empty implementations of the \`X509TrustManager interface are often created to allow connection to a host that is not signed by a root certificate authority. Such an implementation will accept any certificate, which leaves the application vulnerable to Man-in-the-middle attacks. The correct solution is to provide an appropriate trust store.

This rule raises an issue when an implementation of X509TrustManager\` never throws exception.

```java Bad theme={null} class TrustAllManager implements X509TrustManager { @Override public void checkClientTrusted(X509Certificate[] x509Certificates, String s) throws CertificateException { // Noncompliant, nothing means trust any client } @Override public void checkServerTrusted(X509Certificate[] x509Certificates, String s) throws CertificateException { // Noncompliant, this method never throws exception, it means trust any client LOG.log(Level.SEVERE, ERROR_MESSAGE); } @Override public X509Certificate[] getAcceptedIssuers() { return null; } } ``` ```java Fix theme={null} ```

The Spring dependency injection mechanism cannot identify which constructor to use for auto-wiring when multiple constructors are present in a class. This ambiguity can cause the application to crash at runtime, and it makes the code less clear to understand and more complex to extend and maintain.

```java Bad theme={null} @Component public class ExampleClass { // Noncompliant, multiple constructors present and no @Autowired annotation to specify which one to use private final DependencyClass1 dependency1; public ExampleClass() { throw new UnsupportedOperationException("Not supported yet."); } public ExampleClass(DependencyClass1 dependency1) { this.dependency1 = dependency1; } // ... } ``` ```java Fix theme={null} @Component public class ExampleClass { private final DependencyClass1 dependency1; public ExampleClass() { throw new UnsupportedOperationException("Not supported yet."); } @Autowired public ExampleClass(DependencyClass1 dependency1) { this.dependency1 = dependency1; } // ... } ```

In Java 10 Local-Variable Type Inference was introduced. It allows you to omit the expected type of a variable by declaring it with the \`var keyword.

While it is not always possible or cleaner to use this new way of declaring a variable, when the type on the left is the same as the one on the right in an assignment, using the var\` will result in a more concise code.

This rule reports an issue when the expected type of the variable is the same as the returned type of assigned expression and the type can be easily inferred by the reader, either when the type is already mentioned in the name or the initializer, or when the expression is self-explanatory.

```java Bad theme={null} MyClass myClass = new MyClass(); int i = 10; // Type is self-explanatory MyClass something = MyClass.getMyClass(); // Type is already mentioned in the initializer MyClass myClass = get(); // Type is already mentioned in the name ``` ```java Fix theme={null} var myClass = new MyClass(); var i = 10; var something = MyClass.getMyClass(); var myClass = get(); ```

It is very common to pass a collection constructor reference as an argument, for example \`Collectors.toCollection(ArrayList::new) takes the ArrayList::new constructor. When the method expects a java.util.function.Supplier it is perfectly fine. However when the method argument type is java.util.function.Function it means that an argument will be passed to the constructor.

The first argument of Collections constructors is usually an integer representing its "initial capacity". This is generally not what the developer expects, but the memory allocation is not visible at first glance.

This rule raises an issue when a collection constructor is passed by reference as a java.util.function.Function\` argument.

```java Bad theme={null} Arrays.asList(1, 2, 54000).stream().collect(Collectors.toMap(Function.identity(), ArrayList::new)); // Noncompliant, "ArrayList::new" unintentionally refers to "ArrayList(int initialCapacity)" instead of "ArrayList()" ``` ```java Fix theme={null} Arrays.asList(1, 2, 54000).stream().collect(Collectors.toMap(Function.identity(), id -> new ArrayList<>())); // Compliant, explicitly show the usage of "id -> new ArrayList<>()" or "id -> new ArrayList<>(id)" ```

Superfluous exceptions within throws clauses have negative effects on the readability and maintainability of the code. An exception in a throws clause is superfluous if it is:

listed multiple times
a subclass of another listed exception
not actually thrown by any execution path of the method

```java Bad theme={null} void foo() throws MyException, MyException {} // Noncompliant; should be listed once void bar() throws Throwable, Exception {} // Noncompliant; Exception is a subclass of Throwable void boo() throws IOException { // Noncompliant; IOException cannot be thrown System.out.println("Hi!"); } ``` ```java Fix theme={null} void foo() throws MyException {} void bar() throws Throwable {} void boo() { System.out.println("Hi!"); } ```

If the \`suite method in a JUnit 3 TestCase is not declared correctly, it will not be used. Such a method must be named "suite", have no arguments, be public static, and must return either a junit.framework.Test or a junit.framework.TestSuite.

Similarly, setUp and tearDown\` methods that aren’t properly capitalized will also be ignored.

```java Bad theme={null} Test suite() { ... } // Noncompliant; must be public static public static boolean suite() { ... } // Noncompliant; wrong return type public static Test suit() { ... } // Noncompliant; typo in method name public static Test suite(int count) { ... } // Noncompliant; must be no-arg public void setup() { ... } // Noncompliant; should be setUp public void tearDwon() { ... } // Noncompliant; should be tearDown ``` ```java Fix theme={null} public static Test suite() { ... } public void setUp() { ... } public void tearDown() { ... } ```

In Java 8 Streams were introduced to support chaining of operations over collections in a functional style. The most common way to save a result of such chains is to save them to some collection (usually List). To do so there is a terminal method collect that can be used with a library of Collectors. The key problem is that .collect(Collectors.toList()) actually returns a mutable kind of List while in the majority of cases unmodifiable lists are preferred. In Java 10 a new collector appeared to return an unmodifiable list: toUnmodifiableList(). This does the trick but results in verbose code. Since Java 16 there is now a better variant to produce an unmodifiable list directly from a stream: Stream.toList().

This rule raises an issue when "collect" is used to create a list from a stream.

```java Bad theme={null} List list1 = Stream.of("A", "B", "C") .collect(Collectors.toList()); // Noncompliant List list2 = Stream.of("A", "B", "C") .collect(Collectors.toUnmodifiableList()); // Noncompliant ``` ```java Fix theme={null} List list1 = Stream.of("A", "B", "C").toList(); // Compliant List list2 = Stream.of("A", "B", "C") .collect(Collectors.toList()); // Compliant, the list2 needs to be mutable list2.add("X"); ```

There are several reasons that a class might have a method that throws an \`UnsupportedOperationException. The method may be required by an interface or an abstract superclass but not actually needed in the class. Or it may be that the class itself is intended as a superclass, and the method may optionally be implemented by subclasses but not invoked on the superclass. Finally, it could be that the method has been stubbed into the code but not implemented yet.

Whatever the reason, methods that throw UnsupportedOperationException\` should not be called.

```java Bad theme={null} public class Callee { public int doTheThing() { throw new UnsupportedOperationException("Not implemented"); } } public class Caller { public void accomplishStuff() { //... Callee callee = new Callee(); callee.doTheThing(); // Noncompliant ``` ```java Fix theme={null} ```

The rules of operator precedence are complicated and can lead to errors. For this reason, parentheses should be used for clarification in complex statements. However, this does not mean that parentheses should be gratuitously added around every operation.

This rule raises issues when \`&& and || are used in combination, when assignment and equality or relational operators are used together in a condition, and for other operator combinations according to the following table:

	+, -, \*, /, %	\<\<, >>, >>>	&	^	\|
+, -, \*, /, %		x	x	x	x
\<\<, >>, >>>	x		x	x	x
&	x	x		x	x
^	x	x	x		x
\|\`	x	x	x	x

+, -, \*, /, %

\<\<, >>, >>>

+, -, \*, /, %

\<\<, >>, >>>

|\`

This rule also raises an issue when the "true" or "false" expression of a ternary operator is not trivial and not wrapped inside parentheses.

```java Bad theme={null} x = a + b - c; x = a + 1 << b; // Noncompliant y = a == b ? a * 2 : a + b; // Noncompliant if ( a > b || c < d || a == d) {...} if ( a > b && c < d || a == b) {...} // Noncompliant if (a = f(b,c) == 1) { ... } // Noncompliant; == evaluated first ``` ```java Fix theme={null} x = a + b - c; x = (a + 1) << b; y = a == b ? (a * 2) : (a + b); if ( a > b || c < d || a == d) {...} if ( (a > b && c < d) || a == b) {...} if ( (a = f(b,c)) == 1) { ... } ```

There’s no reason to use literal boolean values or nulls in assertions. Instead of using them with assertEquals, assertNotEquals and similar methods, you should be using assertTrue, assertFalse, assertNull or assertNotNull instead (or isNull etc. when using Fest). Using them with assertions unrelated to equality (such as assertNull) is most likely a bug.

Supported frameworks:

JUnit3
JUnit4
JUnit5
Fest assert

```java Bad theme={null} Assert.assertTrue(true); // Noncompliant assertThat(null).isNull(); // Noncompliant assertEquals(true, something()); // Noncompliant assertNotEquals(null, something()); // Noncompliant ``` ```java Fix theme={null} assertTrue(something()); assertNotNull(something()); ```

In Records serialization is not done the same way as for ordinary serializable or externalizable classes. Records serialization does not rely on the \`serialVersionUID field, because the requirement to have this field equal is waived for record classes. By default, all records will have this field equal to 0L and there is no need to specify this field with 0L value and it is possible to specify it with some custom value to support serialization/deserialization involving ordinary classes.

This rule raises an issue when there is a private static final long serialVersionUID field which is set to 0L\` in a Record class.

```java Bad theme={null} record Person(String name, int age) implements Serializable { @Serial private static final long serialVersionUID = 0L; // Noncompliant } ``` ```java Fix theme={null} record Person(String name, int age) implements Serializable {} // Compliant record Person(String name, int age) implements Serializable { @Serial private static final long serialVersionUID = 42L; // Compliant } ```

The Java Persistence API specification imposes only a conditional requirement that \`@Entity classes be Serializable:

If an entity instance is to be passed by value as a detached object (e.g., through a remote interface), the entity class must implement the Serializable interface.

But it’s best practice to make all such classes Serializable from the start. So this rule raises an issue when an @Entity does not implement Serializable\`.

```java Bad theme={null} @Entity pubic class Person { // Noncompliant private String fname; // ... ``` ```java Fix theme={null} @Entity pubic class Person implements Serializable { private String fname; // ... ```

Synchronization is a mechanism used when multithreading in Java to ensure that only one thread executes a given block of code at a time. This is done to avoid bugs that can occur when multiple threads share a given state and try to manipulate simultaneously.

Object serialization is not thread-safe by default. In a multithreaded environment, one option is to mark writeObject with synchronized to improve thread safety. It is highly suspicious, however, if writeObject is the only synchronized method in a class. It may indicate that serialization is not required, as multithreading is not used. Alternatively, it could also suggest that other methods in the same class have been forgotten to be made thread-safe.

```java Bad theme={null} public class RubberBall implements Serializable { private Color color; private int diameter; public RubberBall(Color color, int diameter) { // ... } public void bounce(float angle, float velocity) { // ... } private synchronized void writeObject(ObjectOutputStream stream) throws IOException { // Noncompliant, "writeObject" is the only synchronized method in this class // ... } } ``` ```java Fix theme={null} public class RubberBall implements Serializable { private Color color; private int diameter; public RubberBall(Color color, int diameter) { // ... } public void bounce(float angle, float velocity) { // ... } private void writeObject(ObjectOutputStream stream) throws IOException { // Compliant, no methods in this class are synchronized // ... } } ```

When a back reference in a regex refers to a capturing group that hasn’t been defined yet (or at all), it can never be matched. Named back references throw a \`PatternSyntaxException in that case; numeric back references fail silently when they can’t match, simply making the match fail.

When the group is defined before the back reference but on a different control path (like in (.)|\1\` for example), this also leads to a situation where the back reference can never match.

```java Bad theme={null} Pattern.compile("\\1(.)"); // Noncompliant, group 1 is defined after the back reference Pattern.compile("(.)\\2"); // Noncompliant, group 2 isn't defined at all Pattern.compile("(.)|\\1"); // Noncompliant, group 1 and the back reference are in different branches Pattern.compile("(?.)|\\k"); // Noncompliant, group x and the back reference are in different branches ``` ```java Fix theme={null} Pattern.compile("(.)\\1"); Pattern.compile("(?.)\\k"); ```

The compiler automatically initializes class fields to their default values before setting them with any initialization values, so there is no need to explicitly set a field to its default value. Further, under the logic that cleaner code is better code, it’s considered poor style to do so.

```java Bad theme={null} public class MyClass { int count = 0; // Noncompliant // ... } ``` ```java Fix theme={null} public class MyClass { int count; // ... } ```

All classes extend Object implicitly. Doing so explicitly is redundant.

Further, declaring the implementation of an interface and one if its parents is also redundant. If you implement the interface, you also implicitly implement its parents and there’s no need to do so explicitly.

```java Bad theme={null} public interface MyFace { // ... } public interface MyOtherFace extends MyFace { // ... } public class Foo extends Object // Noncompliant implements MyFace, MyOtherFace { // Noncompliant //... } ``` ```java Fix theme={null} public interface MyFace { // ... } public interface MyOtherFace extends MyFace { // ... } public class Foo implements MyOtherFace { //... } ```

According to its JavaDocs, the intermediate Stream operation \`java.util.Stream.peek() “exists mainly to support debugging” purposes.

A key difference with other intermediate Stream operations is that the Stream implementation is free to skip calls to peek() for optimization purpose. This can lead to peek() being unexpectedly called only for some or none of the elements in the Stream.

As a consequence, relying on peek()\` without careful consideration can lead to error-prone code.

This rule raises an issue for each use of peek() to be sure that it is challenged and validated by the team to be meant for production debugging/logging purposes.

```java Bad theme={null} Stream.of("one", "two", "three", "four") .filter(e -> e.length() > 3) .peek(e -> System.out.println("Filtered value: " + e)); // Noncompliant ``` ```java Fix theme={null} Stream.of("one", "two", "three", "four") .filter(e -> e.length() > 3) .foreach(e -> System.out.println("Filtered value: " + e)); ```

Using high power consumption modes for Bluetooth operations can drain the device battery faster and may not be suitable for scenarios where power efficiency is crucial.

This rule identifies instances where high power consumption Bluetooth operations are used, specifically when requestConnectionPriority or setAdvertiseMode methods are invoked with arguments other than those promoting low power consumption.

```java Bad theme={null} public class BluetoothExample { private final BluetoothGattCallback gattCallback = new BluetoothGattCallback() { @Override public void onConnectionStateChange(BluetoothGatt gatt, int status, int newState) { // ... } @Override public void onServicesDiscovered(BluetoothGatt gatt, int status) { if (status == BluetoothGatt.GATT_SUCCESS) { gatt.requestConnectionPriority(BluetoothGatt.CONNECTION_PRIORITY_HIGH); // Noncompliant } } }; } ``` ```java Fix theme={null} public class BluetoothExample { private final BluetoothGattCallback gattCallback = new BluetoothGattCallback() { @Override public void onConnectionStateChange(BluetoothGatt gatt, int status, int newState) { // ... } @Override public void onServicesDiscovered(BluetoothGatt gatt, int status) { if (status == BluetoothGatt.GATT_SUCCESS) { gatt.requestConnectionPriority(BluetoothGatt.CONNECTION_PRIORITY_LOW_POWER); // Compliant } } }; } ```

It is very easy to write incomplete assertions when using some test frameworks. This rule enforces complete assertions in the following cases:

Fest: \`assertThat is not followed by an assertion invocation
AssertJ: assertThat is not followed by an assertion invocation
Mockito: verify is not followed by a method invocation
Truth: assertXXX\` is not followed by an assertion invocation

In such cases, what is intended to be a test doesn’t actually verify anything

```java Bad theme={null} // Fest boolean result = performAction(); // let's now check that result value is true assertThat(result); // Noncompliant; nothing is actually checked, the test passes whether "result" is true or false // Mockito List mockedList = Mockito.mock(List.class); mockedList.add("one"); mockedList.clear(); // let's check that "add" and "clear" methods are actually called Mockito.verify(mockedList); // Noncompliant; nothing is checked here, oups no call is chained to verify() ``` ```java Fix theme={null} // Fest boolean result = performAction(); // let's now check that result value is true assertThat(result).isTrue(); // Mockito List mockedList = Mockito.mock(List.class); mockedList.add("one"); mockedList.clear(); // let's check that "add" and "clear" methods are actually called Mockito.verify(mockedList).add("one"); Mockito.verify(mockedList).clear(); ```

Regardless of the logging framework in use (logback, log4j, commons-logging, java.util.logging, …), loggers should be:

\`private: never be accessible outside of its parent class. If another class needs to log something, it should instantiate its own logger.
static: not be dependent on an instance of a class (an object). When logging something, contextual information can of course be provided in the messages but the logger should be created at class level to prevent creating a logger along with each object.
final\`: be created once and only once per class.

```java Bad theme={null} public Logger logger = LoggerFactory.getLogger(Foo.class); // Noncompliant ``` ```java Fix theme={null} private static final Logger LOGGER = LoggerFactory.getLogger(Foo.class); ```

Using a type parameter when you don’t have to simply obfuscates the code. Inserting an unnecessary type parameter in an unparameterized method call will compile, but confuse maintainers.

```java Bad theme={null} void doTheThing() { // ... } //... this.doTheThing(); // Noncompliant ``` ```java Fix theme={null} void doTheThing() { // ... } //... this.doTheThing(); ```

Java uses angular brackets (\[/code> and \) to provide a specific type (the "type argument") to a generic type. For instance, List is a generic type, so a list containing strings can be declared with List\.

Prior to Java 7, the type argument had to be provided explicitly for every occurrence where generics were used. This often caused redundancy, as the type argument would have to be provided both when a field is declared and initialized.

Java 7 introduced the diamond operator (\<>) to reduce the code’s verbosity in some situations. The type argument between the angular brackets should be omitted if the compiler can infer it.

Since the diamond operator was only introduced in Java 7, this rule is automatically disabled when the project’s sonar.java.source is lower than 7.

```java Bad theme={null} List strings = new ArrayList(); // Noncompliant, the compiler can infer the type argument of the constructor invocation Map> map = new HashMap>(); // Noncompliant, the compiler can also infer complex type arguments ``` ```java Fix theme={null} List strings = new ArrayList<>(); // Compliant, the compiler will infer the type argument Map> map = new HashMap<>(); // Compliant, the compiler will infer the type argument ```

There’s no point in having a public member in a non-public class because objects that can’t access the class will never have the chance to access the member.

This rule raises an issue when classes has methods, fields, or inner classes with higher visibility than the class itself has.

```java Bad theme={null} class MyClass { public static final float PI = 3.14; // Noncompliant public int getOne() { // Noncompliant return 1; } protected class InnerClass { // Noncompliant; outer class is package-protected public boolean flipCoin() { // Noncompliant; owning class is protected return false; } // ... } } ``` ```java Fix theme={null} public class MyClass { // Class visibility upgrade makes members compliant public static final float PI = 3.14; public int getOne() { return 1; } protected class InnerClass { protected boolean flipCoin() { // visibility changed to match class return false; } // ... } } ```

The Object.wait(…), Object.notify() and Object.notifyAll() methods are used in multithreaded environments to coordinate interdependent tasks that are performed by different threads. These methods are not thread-safe and by contract, they require the invoking Thread to own the object’s monitor. If a thread invokes one of these methods without owning the object’s monitor an IllegalMonitorStateException is thrown.

```java Bad theme={null} private void performSomeAction(Object syncValue) { while (!suitableCondition()){ syncValue.wait(); // Noncompliant, not being inside a `synchronized` block, this will raise an IllegalMonitorStateException } ... // Perform some action } ``` ```java Fix theme={null} private void performSomeAction(Object syncValue) { synchronized(syncValue) { while (!suitableCondition()){ syncValue.wait(); // Compliant, the `synchronized` block guarantees ownership of syncValue's monitor } ... // Perform some action } } ```

Instances of a Serializable class can be saved out to file and rehydrated at leisure. But that only works when all the values in the class instance are themselves Serializable (or transient). Storing a non-serializable value in a Serializable class will prevent the serialization of that class.

```java Bad theme={null} interface Fruit extends Serializable {...} class Gooseberry implements Fruit { // Nonserializable because of Thread field Thread thread; } class Bowl implements Serializable { private static final long serialVersionUID = 1; private Fruit fruit = new Gooseberry(); //Non-Compliant } ``` ```java Fix theme={null} interface Fruit implements Serializable {...} class Gooseberry implements Fruit {...} //Serializable class Bowl implements Serializable { private static final long serialVersionUID = 1; private Fruit fruit = new Gooseberry(); //Compliant, Gooseberry is serializable } ```

In Java 15 Text Blocks are official and can be used just like an ordinary String. However, when they are used to represent a big chunk of text, they should not be used directly in complex expressions, as it decreases the readability. In this case, it is better to extract the text block into a variable or a field.

This rule reports an issue when a text block longer than a number of lines given as a parameter is directly used within a lambda expression.

```java Bad theme={null} listOfString.stream() .map(str -> !""" 4.0.0 com.mycompany.app my-app 1 com.mycompany.app my-module 1 """.equals(str)); ``` ```java Fix theme={null} String myTextBlock = """ 4.0.0 com.mycompany.app my-app 1 com.mycompany.app my-module 1 """; listOfString.stream() .map(str -> !myTextBlock.equals(str)); ```

Double-checked locking can be used for lazy initialization of volatile fields, but only if field assignment is the last step in the synchronized block. Otherwise you run the risk of threads accessing a half-initialized object.

```java Bad theme={null} public class MyClass { private volatile List strings; public List getStrings() { if (strings == null) { // check#1 synchronized(MyClass.class) { if (strings == null) { strings = new ArrayList<>(); // Noncompliant strings.add("Hello"); //When threadA gets here, threadB can skip the synchronized block because check#1 is false strings.add("World"); } } } return strings; } } ``` ```java Fix theme={null} public class MyClass { private volatile List strings; public List getStrings() { if (strings == null) { // check#1 synchronized(MyClass.class) { if (strings == null) { List tmpList = new ArrayList<>(); tmpList.add("Hello"); tmpList.add("World"); strings = tmpList; } } } return strings; } } ```

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…

This is because value-based classes are intended to be wrappers for value types, which will be primitive-like collections of data (similar to \`structs in other languages) that will come in future versions of Java.

Instances of a value-based class …

do not have accessible constructors, but are instead instantiated through factory methods which make no commitment as to the identity of returned instances;

This means that you can’t be sure you’re the only one trying to lock on any given instance of a value-based class, opening your code up to contention and deadlock issues.

Under Java 8 breaking this rule may not actually break your code, but there are no guarantees of the behavior beyond that.

This rule raises an issue when a known value-based class is used for synchronization. That includes all the classes in the java.time package except Clock; the date classes for alternate calendars, HijrahDate, JapaneseDate, MinguoDate, ThaiBuddhistDate; and the optional classes: Optional, OptionalDouble, OptionalLong, OptionalInt.

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

```java Bad theme={null} Optional fOpt = doSomething(); synchronized (fOpt) { // Noncompliant // ... } ``` ```java Fix theme={null} ```

Callers of a Boolean method may be expecting to receive true or false in response. But Boolean objects can take null as a possible value. Boolean methods should not return null unless the code is annotated appropriately. With the proper annotation, the caller is aware that the returned value could be null.

```java Bad theme={null} public Boolean isUsable() { // ... return null; // Noncompliant } public void caller() { if (isUsable()) { // A NullPointerException might occur here // ... } } ``` ```java Fix theme={null} @javax.annotation.Nullable public Boolean isUsable() { // ... return null; } @javax.annotation.CheckForNull public Boolean isUsable() { // ... return null; } public void caller() { if (Boolean.True.equals(isUsable())) { // This caller knows to check and avoid ambiguity // ... } } ```

Non-encoded control characters and whitespace characters are often injected in the source code because of a bad manipulation. They are either invisible or difficult to recognize, which can result in bugs when the string is not what the developer expects. If you actually need to use a control character use their encoded version (ex: ASCII \`\n,\t,… or Unicode U+000D, U+0009,…).

This rule raises an issue when the following characters are seen in a literal string:

ASCII control character. (character index \< 32 or = 127)

Unicode whitespace characters.
Unicode C0 control characters
Unicode characters U+200B, U+200C, U+200D, U+2060, U+FEFF, U+2028, U+2029

No issue will be raised on the simple space character. Unicode U+0020\`, ASCII 32.

```java Bad theme={null} String tabInside = "A B"; // Noncompliant, contains a tabulation String zeroWidthSpaceInside = "foobar"; // Noncompliant, it contains a U+200B character inside char tab = ' '; ``` ```java Fix theme={null} String tabInside = "A\tB"; // Compliant, uses escaped value String zeroWidthSpaceInside = "foo\u200Bbar"; // Compliant, uses escaped value char tab = '\t'; ```

Serializing a non-\`static inner class will result in an attempt at serializing the outer class as well. If the outer class is actually serializable, then the serialization will succeed but possibly write out far more data than was intended.

Making the inner class static (i.e. "nested") avoids this problem, therefore inner classes should be static if possible. However, you should be aware that there are 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 Raspberry implements Serializable { // ... public class Drupelet implements Serializable { // Noncompliant; output may be too large // ... } } ``` ```java Fix theme={null} public class Raspberry implements Serializable { // ... public static class Drupelet implements Serializable { // ... } } ```

Classes without public static members cannot be used without being instantiated, but classes with only private constructors cannot be instantiated. When a class has only private constructors and no static members, it is useless and should be removed or refactored.

```java Bad theme={null} public class MyClass { // Noncompliant private MyClass () { } public double getPi(){ return 3.14; } } ``` ```java Fix theme={null} public class MyClass { public MyClass () { } public double getPi(){ return 3.14; } } ```

According to the specification:

Nonfinal \`static class fields are disallowed in EJBs because such fields make an enterprise bean difficult or impossible to distribute.

Therefore, all static fields in an EJB should also be final\`.

```java Bad theme={null} @Stateless public class MyEjb { private static String message; // Noncompliant } ``` ```java Fix theme={null} ```

When directly subclassing \`java.io.OutputStream or java.io.FilterOutputStream, the only requirement is that you implement the method write(int). However most uses for such streams don’t write a single byte at a time and the default implementation for write(byte\[],int,int) will call write(int) for every single byte in the array which can create a lot of overhead and is utterly inefficient. It is therefore strongly recommended that subclasses provide an efficient implementation of write(byte\[],int,int).

This rule raises an issue when a direct subclass of java.io.OutputStream or java.io.FilterOutputStream doesn’t provide an override of write(byte\[],int,int)\`.

```java Bad theme={null} public class MyStream extends OutputStream { // Noncompliant private FileOutputStream fout; public MyStream(File file) throws IOException { fout = new FileOutputStream(file); } @Override public void write(int b) throws IOException { fout.write(b); } @Override public void close() throws IOException { fout.write("\n\n".getBytes()); fout.close(); super.close(); } } ``` ```java Fix theme={null} public class MyStream extends OutputStream { private FileOutputStream fout; public MyStream(File file) throws IOException { fout = new FileOutputStream(file); } @Override public void write(int b) throws IOException { fout.write(b); } @Override public void write(byte[] b, int off, int len) throws IOException { fout.write(b, off, len); } @Override public void close() throws IOException { fout.write("\n\n".getBytes()); fout.close(); super.close(); } } ```

Specifying the default value for an annotation parameter is redundant. Such values should be omitted in the interests of readability.

```java Bad theme={null} @MyAnnotation(arg = "def") // Noncompliant public class MyClass { // ... } public @interface MyAnnotation { String arg() default "def"; } ``` ```java Fix theme={null} @MyAnnotation public class MyClass { // ... } public @interface MyAnnotation { String arg() default "def"; } ```

\`Throwable.printStackTrace(...) prints a Throwable and its stack trace to some stream. By default that stream System.Err, which could inadvertently expose sensitive information.

Loggers should be used instead to print Throwables, as they have many advantages:

Users are able to easily retrieve the logs.
The format of log messages is uniform and allow users to browse the logs easily.

This rule raises an issue when printStackTrace\` is used without arguments, i.e. when the stack trace is printed to the default stream.

```java Bad theme={null} try { /* ... */ } catch(Exception e) { e.printStackTrace(); // Noncompliant } ``` ```java Fix theme={null} try { /* ... */ } catch(Exception e) { LOGGER.log("context", e); } ```

PreparedStatement is an object that represents a precompiled SQL statement, that can be used to execute the statement multiple times efficiently.

ResultSet is the Java representation of the result set of a database query obtained from a Statement object. A default ResultSet object is not updatable and has a cursor that moves forward only.

The parameters in PreparedStatement and ResultSet are indexed beginning at 1, not 0. When an invalid index is passed to the PreparedStatement or ResultSet methods, an IndexOutOfBoundsException is thrown. This can cause the program to crash or behave unexpectedly, leading to a poor user experience.

This rule raises an issue for the get methods in PreparedStatement and the set methods in ResultSet.

```java Bad theme={null} PreparedStatement ps = con.prepareStatement("SELECT fname, lname FROM employees where hireDate > ? and salary < ?"); ps.setDate(0, date); // Noncompliant ps.setDouble(3, salary); // Noncompliant ResultSet rs = ps.executeQuery(); while (rs.next()) { String fname = rs.getString(0); // Noncompliant // ... } ``` ```java Fix theme={null} PreparedStatement ps = con.prepareStatement("SELECT fname, lname FROM employees where hireDate > ? and salary < ?"); ps.setDate(1, date); ps.setDouble(2, salary); ResultSet rs = ps.executeQuery(); while (rs.next()) { String fname = rs.getString(1); // ... } ```

The problem with invoking \`Thread.start() in a constructor is that you’ll have a confusing mess on your hands if the class is ever extended because the superclass' constructor will start the thread before the child class has truly been initialized.

This rule raises an issue any time start is invoked in the constructor of a non-final\` class.

```java Bad theme={null} public class MyClass { Thread thread = null; public MyClass(Runnable runnable) { thread = new Thread(runnable); thread.start(); // Noncompliant } } ``` ```java Fix theme={null} ```

The Spring framework provides the annotation Async to mark a method (or all methods of a type) as a candidate for asynchronous execution.

Asynchronous methods do not necessarily, by their nature, return the result of their calculation immediately. Hence, it is unexpected and in clear breach of the Async contract for such methods to have a return type that is neither void nor a Future type.

```java Bad theme={null} @Async public String asyncMethod() { ... } ``` ```java Fix theme={null} @Async public Future asyncMethod() { ... } ```

Spring framework 4.3 introduced variants of the @RequestMapping annotation to better represent the semantics of the annotated methods. The use of @GetMapping, @PostMapping, @PutMapping, @PatchMapping and @DeleteMapping should be preferred to the use of the raw @RequestMapping(method = RequestMethod.XYZ).

```java Bad theme={null} @RequestMapping(path = "/greeting", method = RequestMethod.GET) // Noncompliant public Greeting greeting(@RequestParam(value = "name", defaultValue = "World") String name) { ... } ``` ```java Fix theme={null} @GetMapping(path = "/greeting") // Compliant public Greeting greeting(@RequestParam(value = "name", defaultValue = "World") String name) { ... } ```

Using toLowerCase() or toUpperCase() to make case insensitive comparisons is inefficient because it requires the creation of temporary, intermediate String objects.

```java Bad theme={null} private void compareStrings(String foo, String bar){ boolean result1 = foo.toUpperCase().equals(bar); // Noncompliant boolean result2 = foo.equals(bar.toUpperCase()); // Noncompliant boolean result3 = foo.toLowerCase().equals(bar.toLowerCase()); // Noncompliant } ``` ```java Fix theme={null} private void compareStrings(String foo, String bar){ boolean result1 = foo.equalsIgnoreCase(bar); // Compliant } ```

Swing interfaces should be constructed and shown from the Swing event dispatch thread. Doing so from any other thread, such as from \`main risks deadlocks since you run the risk of multiple threads accessing things which are inherently not thread-safe.

Instead, use SwingUtilities.invokeLater or SwingUtilities.invokeAndWait to kick off a new Runnable\` that handles your GUI creation.

```java Bad theme={null} public static void main(String args[]) { makeGui(); // Noncompliant } public void makeGui() { JFrame frame = new JFrame(); // ... frame.show(); } ``` ```java Fix theme={null} public static void main(String args[]) { SwingUtilities.invokeAndWait(new Runnable() { public void run() { makeGui(); } } } public void makeGui() { JFrame frame = new JFrame(); // ... frame.show(); } ```

When creating a \`DateTimeFormatter using the WeekFields.weekBasedYear() temporal field, the resulting year number may be off by 1 at the beginning of a new year (when the date to format is in a week that is shared by two consecutive years).

Using this year number in combination with an incompatible week temporal field yields a result that may be confused with the first week of the previous year.

Instead, when paired with a week temporal field, the week-based year should only be used with the week of week-based year temporal field WeekFields.weekOfWeekBasedYear().

Alternatively the temporal field ChronoField.ALIGNED\_WEEK\_OF\_YEAR\` can be used together with a regular year (but not the week based year).

```java Bad theme={null} new DateTimeFormatterBuilder() .appendValue(ChronoField.YEAR, 4) // Noncompliant: using week of week-based year with regular year .appendLiteral('-') .appendValue(WeekFields.ISO.weekOfWeekBasedYear(), 2) .toFormatter(); new DateTimeFormatterBuilder() .appendValue(ChronoField.YEAR_OF_ERA, 4) // Noncompliant: using week of week-based year with regular year .appendLiteral('-') .appendValue(WeekFields.ISO.weekOfWeekBasedYear(), 2) .toFormatter(); new DateTimeFormatterBuilder() .appendValue(WeekFields.ISO.weekBasedYear(), 4) // Noncompliant: using aligned week of year with week-based year .appendLiteral('-') .appendValue(ChronoField.ALIGNED_WEEK_OF_YEAR, 2) .toFormatter(); ``` ```java Fix theme={null} new DateTimeFormatterBuilder() .appendValue(WeekFields.ISO.weekBasedYear(), 4) .appendLiteral('-') .appendValue(WeekFields.ISO.weekOfWeekBasedYear(), 2) .toFormatter(); new DateTimeFormatterBuilder() .appendValue(ChronoField.YEAR, 4) .appendLiteral('-') .appendValue(ChronoField.ALIGNED_WEEK_OF_YEAR, 2) .toFormatter(); new DateTimeFormatterBuilder() .appendValue(ChronoField.YEAR_OF_ERA, 4) .appendLiteral('-') .appendValue(ChronoField.ALIGNED_WEEK_OF_YEAR, 2) .toFormatter(); ```

The use of escape sequences is mostly unnecessary in text blocks.

```java Bad theme={null} String textBlock = """ \"\"\" this \nis text block! !!!! """; ``` ```java Fix theme={null} String textBlock = """ \""" this is text block! !!!! """; ```

Spring Expression Language (SpEL) is an expression language used in the Spring Framework for evaluating and manipulating objects, properties, and conditions within Spring-based applications.

org.springframework.ui.Model is an interface in the Spring Framework that represents a container for data that can be passed between a controller and a view in a Spring MVC web application, allowing for data sharing during the request-response cycle.

Attributes added to the org.springframework.ui.Model should follow the Java identifier naming convention, which means they must start with a letter a-z, A-Z, underscore \_, or a dollar sign \$ and may be followed by letters, digits, underscores, or dollar signs.

Failure to do so may result in SpEL parsing errors when using these attributes in template engines.

```java Bad theme={null} model.addAttribute(" a", 100); // Noncompliant (starts with a space) model.addAttribute("a-b", 7); // Noncompliant (contains a hyphen) model.addAttribute("1c", 42); // Noncompliant (starts with a digit) ``` ```java Fix theme={null} model.addAttribute("a", 100); model.addAttribute("b", 42); model.addAttribute("_c", 7); model.addAttribute("$d", 8); ```

Calling \`Class.newInstance invokes the class' default constructor. Unfortunately, since it’s not a direct call to the constructor, compile-time checking will be unable to detect the possibility. This means that your code will compile even if you haven’t put the invocation in a try block.

On the other hand, Construtor.newInstance handles exceptions by wrapping them in an InvocationTargetException and explicitly throwing them.

This rule raises an issue when Class.newInstance\` is used to invoke a constructor that throws checked exceptions.

```java Bad theme={null} Foo f = Foo.class.newInstance(); // Noncompliant ``` ```java Fix theme={null} Foo f = Foo.class.getConstructor().newInstance(); ```

Calling constructors for String, BigInteger, BigDecimal and the objects used to wrap primitives is less efficient and less clear than relying on autoboxing or valueOf.

Consider simplifying when possible for more efficient and cleaner code.

```java Bad theme={null} String empty = new String(); // Noncompliant; yields essentially "", so just use that. String nonempty = new String("Hello world"); // Noncompliant Double myDouble = new Double(1.1); // Noncompliant; use valueOf Integer integer = new Integer(1); // Noncompliant Boolean bool = new Boolean(true); // Noncompliant BigInteger bigInteger1 = new BigInteger("3"); // Noncompliant BigInteger bigInteger2 = new BigInteger("9223372036854775807"); // Noncompliant BigInteger bigInteger3 = new BigInteger("111222333444555666777888999"); // Compliant, greater than Long.MAX_VALUE BigDecimal bigDecimal = new BigDecimal("42.0"); // Compliant (see Exceptions section) ``` ```java Fix theme={null} String empty = ""; String nonempty = "Hello world"; Double myDouble = 1.1; Integer integer = 1; Boolean bool = true; BigInteger bigInteger1 = BigInteger.valueOf(3); BigInteger bigInteger2 = BigInteger.valueOf(9223372036854775807L); BigInteger bigInteger3 = new BigInteger("111222333444555666777888999"); BigDecimal bigDecimal = new BigDecimal("42.0"); ```

SpEL is used in Spring annotations and is parsed by the Spring framework, not by the Java compiler. This means that invalid SpEL expressions are not detected during Java compile time. They will cause exceptions during runtime instead, or even fail silently with the expression string interpreted as a simple string literal by Spring.

```java Bad theme={null} @Value("#{systemProperties['user.region'}") // Noncompliant, unclosed "[" private String region; ``` ```java Fix theme={null} @Value("#{'${listOfValues}' split(',')}") // Noncompliant, missing operator private List valuesList; ```

To ensure EJB portability, the EJB specification forbids the use of functionality in the \`java.io package. Instead of reading and writing files, EJB’s should use some other means of data storage and retrieval, such as JDBC.

This rule raises an issue for the first java.io\` method call in each method.

```java Bad theme={null} public class MyBean implements BeanInterface { private File baseline = null; private void readBaseline () { try { baseline = new File(Constants.INTEREST_RATE_FILE); // Noncompliant. if (baseline.exists()) { //... } } catch (IOException e) { //... } } private void writeBaseline() { try { FileWriter fw = new FileWriter(baseline.getAbsoluteFile()); // Noncompliant BufferedWriter bw = new BufferedWriter(fw); bw.write(content); bw.close(); } catch (IOException e) { //... } } } ``` ```java Fix theme={null} ```

Importing a class statically allows you to use its public static members without qualifying them with the class name. That can be handy, but if you import too many classes statically, your code can become confusing and difficult to maintain.

```java Bad theme={null} import static java.lang.Math.*; import static java.util.Collections.*; import static com.myco.corporate.Constants.*; import static com.myco.division.Constants.*; import static com.myco.department.Constants.*; // Noncompliant ``` ```java Fix theme={null} ```

Non-overridable methods (private or final) that don’t access instance data can be static to prevent any misunderstanding about the contract of the method.

```java Bad theme={null} class Utilities { private static String magicWord = "magic"; private String getMagicWord() { // Noncompliant return magicWord; } private void setMagicWord(String value) { // Noncompliant magicWord = value; } } ``` ```java Fix theme={null} class Utilities { private static String magicWord = "magic"; private static String getMagicWord() { return magicWord; } private static void setMagicWord(String value) { magicWord = value; } } ```

Classes with only private constructors should be marked final to prevent any mistaken extension attempts.

```java Bad theme={null} public class PrivateConstructorClass { // Noncompliant private PrivateConstructorClass() { // ... } public static int magic(){ return 42; } } ``` ```java Fix theme={null} public final class PrivateConstructorClass { // Compliant private PrivateConstructorClass() { // ... } public static int magic(){ return 42; } } ```

Because a subclass instance may be cast to and treated as an instance of the superclass, overriding methods should uphold the aspects of the superclass contract that relate to the Liskov Substitution Principle. Specifically, if the parameters or return type of the superclass method are marked with any of the following: @Nullable, @CheckForNull, @NotNull, @NonNull, and @Nonnull, then subclass parameters are not allowed to tighten the contract, and return values are not allowed to loosen it.

```java Bad theme={null} public class Fruit { private Season ripe; private String color; public void setRipe(@Nullable Season ripe) { this.ripe = ripe; } public @NotNull Integer getProtein() { return 12; } } public class Raspberry extends Fruit { public void setRipe(@NotNull Season ripe) { // Noncompliant: the ripe argument annotated as @Nullable in parent class this.ripe = ripe; } public @Nullable Integer getProtein() { // Noncompliant: the return type annotated as @NotNull in parent class return null; } } ``` ```java Fix theme={null} public class Fruit { private Season ripe; private String color; public void setRipe(@Nullable Season ripe) { this.ripe = ripe; } public @NotNull Integer getProtein() { return 12; } } public class Raspberry extends Fruit { public void setRipe(@Nullable Season ripe) { this.ripe = ripe; } public @NotNull Integer getProtein() { return 12; } } ```

Spring \`@Component, @Controller, @RestController,@Service, and @Repository classes are singletons by default, meaning only one instance of the class is ever instantiated in the application. Typically such a class might have a few static members, such as a logger, but all non-static members should be managed by Spring.

This rule raises an issue when a singleton @Component, @Controller, @RestController, @Service, or @Repository, not annotated with @ConfigurationProperties, has non-static members that are not annotated with one of:

org.springframework.beans.factory.annotation.Autowired
org.springframework.beans.factory.annotation.Value
javax.annotation.Inject
javax.annotation.Resource\`

```java Bad theme={null} @Controller public class HelloWorld { private String name = null; @RequestMapping("/greet", method = GET) public String greet(String greetee) { if (greetee != null) { this.name = greetee; } return "Hello " + this.name; // if greetee is null, you see the previous user's data } } ``` ```java Fix theme={null} ```

Java 21 introduces the new Sequenced Collections API, which applies 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, use this API instead of workaround implementations that were necessary before Java 21. One of the features of the new Sequenced Collections API is SequencedCollection.reversed() which returns a lightweight view of the original collection, in the reverse order.

This rule reports when reverse view would have been sufficient instead of a reverse copy of a sequenced collection created using a list constructor plus a Collections.reverse(collection); call.

If feasible, a view should be preferred over a copy because a view is a lightweight iterator without modification of the list itself.

```java Bad theme={null} void foo() { var list = new ArrayList(); list.add("A"); list.add("B"); Collections.reverse(list); // Noncompliant for (var e : list) { // ... } } ``` ```java Fix theme={null} void foo() { var list = new ArrayList(); list.add("A"); list.add("B"); for (var e : list.reversed()) { // Compliant // ... } } ```

Sharing some naming conventions is a key point to make it possible for a team to efficiently collaborate. This rule allows to check that all abstract class names match a provided regular expression. If a non-abstract class match the regular expression, an issue is raised to suggest to either make it abstract or to rename it.

```java Bad theme={null} abstract class MyClass { // Noncompliant } class AbstractLikeClass { // Noncompliant } ``` ```java Fix theme={null} abstract class AbstractClass { } class LikeClass { } ```

While the Singleton pattern can be useful in certain situations, overusing it can have several drawbacks:

Tight coupling: The Singleton pattern can create tight coupling between the Singleton class and other classes that use it, making the code difficult to maintain and modify.
Global state: The Singleton pattern can create a global state, making it difficult to manage the state of the application and leading to unexpected behavior.
Testing: The Singleton pattern can make it difficult to test classes that depend on the Singleton, as the Singleton cannot be easily substituted with a mock object.
Scalability: The Singleton pattern can make it difficult to scale an application, as it can create a bottleneck if multiple threads try to access the Singleton concurrently.
Dependency injection: The Singleton pattern can make it difficult to use dependency injection frameworks, as the Singleton instance is usually created statically.

In general, the Singleton pattern should be used sparingly and only in situations where it provides a clear benefit over other patterns or approaches. It is important to consider the drawbacks and tradeoffs of using the Singleton pattern before incorporating it into an application.

```java Bad theme={null} public enum EnumSingleton { INSTANCE; private EnumSingleton() { // Initialization code here... } } ``` ```java Fix theme={null} public class BillPughSingleton { private BillPughSingleton(){} private static class SingletonHelper { private static final BillPughSingleton INSTANCE = new BillPughSingleton(); } public static BillPughSingleton getInstance() { return SingletonHelper.INSTANCE; } } ```

Some API, like the AWS SDK, heavily rely on the builder pattern to create different data structures. Despite all the benefits, this pattern can become really verbose, especially when dealing with nested structures. In order to reach a more concise code, "Consumer Builders", also called "Consumer Interface" are often introduced.

The idea is to overload the methods taking others structures in a Builder with a Consumer of Builder instead. This enables to use a lambda instead of nesting another Builder, resulting in more concise and readable code.

This rule reports an issue when the Consumer Builder methods could be used instead of the classical ones.

```java Bad theme={null} SendEmailRequest.builder() .destination(Destination.builder() .toAddresses("to-email@domain.com") .bccAddresses("bcc-email@domain.com") .build()) .build(); ``` ```java Fix theme={null} SendEmailRequest.builder() .destination(d -> d.toAddresses("to-email@domain.com").bccAddresses("bcc-email@domain.com")) .build(); ```

Lambda expressions with only one argument with an inferred type (i.e., no explicit type declaration) can be written without parentheses around that single parameter. This syntax is simpler, more compact and readable than using parentheses and is therefore preferred.

This rule is automatically disabled when the project’s sonar.java.source is lower than 8, as lambda expressions were introduced in Java 8.

```java Bad theme={null} (x) -> x * 2 ``` ```java Fix theme={null} x -> x * 2 ```

This rule raises an issue when a configured Java package or class is used.

```java Bad theme={null} import java.sql.*; // Noncompliant java.util.ArrayList clients; // Noncompliant java.lang.String name // Compliant ``` ```java Fix theme={null} ```

The \`instanceof construction is a preferred way to check whether a variable can be cast to some type statically because a compile-time error will occur in case of incompatible types. The method isInstance() from java.lang.Class works differently and does type check at runtime only, incompatible types will therefore not be detected early in the development, potentially resulting in dead code. The isInstance() method should only be used in dynamic cases when the instanceof operator can’t be used.

This rule raises an issue when isInstance() is used and could be replaced with an instanceof\` check.

```java Bad theme={null} int f(Object o) { if (String.class.isInstance(o)) { // Noncompliant return 42; } return 0; } int f(Number n) { if (String.class.isInstance(n)) { // Noncompliant return 42; } return 0; } ``` ```java Fix theme={null} int f(Object o) { if (o instanceof String) { // Compliant return 42; } return 0; } int f(Number n) { if (n instanceof String) { // Compile-time error return 42; } return 0; } boolean fun(Object o, String c) throws ClassNotFoundException { return Class.forName(c).isInstance(o); // Compliant, can't use instanceof operator here } ```

The Java language authors have been quite frank that \`Optional was intended for use only as a return type, as a way to convey that a method may or may not return a value.

And for that, it’s valuable but using Optional on the input side increases the work you have to do in the method without really increasing the value. With an Optional parameter, you go from having 2 possible inputs: null and not-null, to three: null, non-null-without-value, and non-null-with-value. Add to that the fact that overloading has long been available to convey that some parameters are optional, and there’s really no reason to have Optional parameters.

The rule also checks for Guava’s Optional, as it was the inspiration for the JDK Optional. Although it is different in some aspects (serialization, being recommended for use as collection elements), using it as a parameter type causes exactly the same problems as for JDK Optional\`.

```java Bad theme={null} public String sayHello(Optional name) { // Noncompliant if (name == null || !name.isPresent()) { return "Hello World"; } else { return "Hello " + name; } } ``` ```java Fix theme={null} public String sayHello(String name) { if (name == null) { return "Hello World"; } else { return "Hello " + name; } } ```

The Java Collections framework defines interfaces such as java.util.List or java.util.Map. Several implementation classes are provided for each of those interfaces to fill different needs: some of the implementations guarantee a few given performance characteristics, some others ensure a given behavior, for example immutability.

Among the methods defined by the interfaces of the Collections framework, some are declared as "optional": an implementation class may choose to throw an UnsupportedOperationException when one of those methods is called. For example, java.util.Collections.emptyList() returns an implementation of java.util.List which is documented as "immutable". Calling the add method on this object triggers an UnsupportedOperationException.

```java Bad theme={null} List list = Collections.emptyList(); // The list implementation returned here is unmodifiable. if (someCondition) { list.add("hello"); // Noncompliant; throws an UnsupportedOperationException } return list; ``` ```java Fix theme={null} List list = new ArrayList<>(); if (someCondition) { list.add("hello"); } return list; ```

This issue is raised when Sonar considers that a method is a 'Brain Method'. A Brain Method is a method that tends to centralize its owner’s class logic and generally performs too many operations. This can include checking too many conditions, using lots of variables, and ultimately making it difficult to understand, maintain and reuse. It is characterized by high LOC number, high cyclomatic and cognitive complexity, and a large number of variables being used.

```java Bad theme={null} void farmDailyRoutine() { Crops southEastCrops = getCrops(1, -1); Crops eastCrops = getCrops(1, 0); WaterContainer waterContainer = new WaterContainer(); List bottles = new ArrayList<>(); for(int i = 0; i < 10; i++) { var bottle = new Bottle(); bottle.addWater(10L); bottle.putCap(); bottle.shake(2); bottles.add(bottle); } waterContainer.store(bottles); Truck t1 = new Truck(Truck.Type.TRANSPORT); t1.load(waterContainer); if(Weather.current != Weather.RAINY) { WaterContainer extraWaterContainer = new WaterContainer(); List extraBottles = new ArrayList<>(); if(southEastCrops.isDry()) { for(LandSlot ls : southEastCrops.lands()) { Bottle b = new Bottle(); b.addWater(10L); b.putCap(); extraBottles.add(b); } } else { extraBottles.add(new Bottle()); } if(eastCrops.isDry()) { for(LandSlot ls : southEastCrops.lands()) { Bottle b = new Bottle(); b.addWater(10L); b.putCap(); extraBottles.add(b); } } else { extraBottles.add(new Bottle()); } extraWaterContainer.store(extraBottles); t1.load(extraWaterContainer); } else { WaterContainer extraWaterContainer = WaterSource.clone(waterContainer); t1.load(extraWaterContainer) } } ``` ```java Fix theme={null} void farmDailyRoutine() { // Compliant: Simpler method, making use of extracted and distributed logic Crops southEastCrops = getCrops(1, -1); Crops eastCrops = getCrops(1, 0); WaterContainer waterContainer = new WaterContainer(); List bottles = getWaterBottles(10, 10L, true); waterContainer.store(bottles); Truck t1 = new Truck(Truck.Type.TRANSPORT); t1.load(waterContainer); if(Weather.current != Weather.RAINY) { WaterContainer extraWaterContainer = new WaterContainer(); fillContainerForCrops(extraWaterContainer, southEastCrops); fillContainerForCrops(extraWaterContainer, eastCrops); t1.load(extraWaterContainer); } else { WaterContainer extraWaterContainer = WaterSource.clone(waterContainer); t1.load(extraWaterContainer) } } private fillContainerForCrops(WaterContainer wc, Crops crops) { // Compliant: extracted readable and reusable method if(crops.isDry()) { wc.store(getWaterBottles(crops.lands().size(), 10L, false)); } else { wc.store(Collections.singleton(new Bottle())); } } private List getWaterBottles(int qt, long liquid, boolean shake){ // Compliant: extracted readable and reusable method List bottles = new ArrayList<>(); for(int i = 0; i < qt; i++) { Bottle b = new Bottle(); b.addWater(liquid); b.putCap(); if(shake) { b.shake(); } bottles.add(b); } return bottles; } ```

Two "hash" classes, Hashtable, and ConcurrentHashMap offer contains methods. One might naively assume that the contains method searches both keys and values for its argument. And one would be wrong. Because these legacy methods search only values, they are likely to mislead maintainers even if the original coder understood precisely what’s going on.

```java Bad theme={null} Hashtable ht = new Hashtable<>(); // ... if (ht.contains(foo)) { // Noncompliant // ... } ``` ```java Fix theme={null} Hashtable ht = new Hashtable<>(); // ... if (ht.containsValue(foo)) { // ... } ```

It’s a common pattern to test the result of a \`java.util.Map.get() against null or calling java.util.Map.containsKey() before proceeding with adding or changing the value in the map. However the java.util.Map API offers a significantly better alternative in the form of the computeIfPresent() and computeIfAbsent() methods. Using these instead leads to cleaner and more readable code.

Note that this rule is automatically disabled when the project’s sonar.java.source\` is not 8.

```java Bad theme={null} V value = map.get(key); if (value == null) { // Noncompliant value = V.createFor(key); if (value != null) { map.put(key, value); } } if (!map.containsKey(key)) { // Noncompliant value = V.createFor(key); if (value != null) { map.put(key, value); } } return value; ``` ```java Fix theme={null} return map.computeIfAbsent(key, k -> V.createFor(k)); ```

When iterating over an Iterable with a for loop, the iteration variable could have the same type as the type returned by the iterator (the item type of the Iterable). This rule reports when a supertype of the item type is used for the variable instead, but the variable is then explicitly downcast in the loop body.

Using explicit type casts instead of leveraging the language’s type system is a bad practice. It disables static type checking by the compiler for the cast expressions, but potential errors will throw a ClassCastException during runtime instead.

```java Bad theme={null} for (Object item : getPersons()) { // Noncompliant, iteration element is implicitly upcast here Person person = (Person) item; // Noncompliant, item is explicitly downcast here person.getAddress(); } ``` ```java Fix theme={null} for (Person person : getPersons()) { // Compliant person.getAddress(); } ```

In Java 21 the java.lang.Math class was updated with the static method Math.clamp, to clamp a numerical value between a min and a max value.

Using this built-in method is now the preferred way to restrict to a given interval, as it is more readable and less error-prone.

```java Bad theme={null} int clampedValue = value > max ? max : value < min ? min : value; // Noncompliant; Replace with "Math.clamp" ``` ```java Fix theme={null} int clampedValue = Math.max(min, Math.min(max, value)); // Noncompliant; Replace with "Math.clamp" ```

Class members that are not assigned a default value and are not initialized in a constructor will be set to null by the compiler. Even if code exists to properly set those members, there is a risk that they will be dereferenced before it is called, resulting in a NullPointerException.

Because you cannot guarantee that such classes will always be used properly, class members should always be initialized.

This rule flags members which have no default value and which are left uninitialized by at least one class constructor, but which are unconditionally dereferenced somewhere in the code.

```java Bad theme={null} public class Team { int limit = 30; List roster; // Noncompliant; no default & not initialized by constructor Person coach; public Team (Person coach) { // roster is left uninitialized this.coach = coach; } public void add(Player p) { if (roster == null) { roster = new ArrayList(); } roster.add(p); } public boolean isFull() { // NPE if called before add() return roster.size() < limit; } } ``` ```java Fix theme={null} public class Team { int limit = 30; List roster = new ArrayList(); Person coach; public Team (Person coach) { this.coach = coach; } public void add(Player p) { roster.add(p); } // ... ```

Some method calls can effectively be "no-ops", meaning that the invoked method does nothing, based on the application’s configuration (eg: debug logs in production). However, even if the method effectively does nothing, its arguments may still need to evaluated before the method is called.

Passing message arguments that require further evaluation into a Guava com.google.common.base.Preconditions check can result in a performance penalty. That is because whether or not they’re needed, each argument must be resolved before the method is actually called.

Similarly, passing concatenated strings into a logging method can also incur a needless performance hit because the concatenation will be performed every time the method is called, whether or not the log level is low enough to show the message.

Instead, you should structure your code to pass static or pre-computed values into Preconditions conditions check and logging calls.

Specifically, the built-in string formatting should be used instead of string concatenation, and if the message is the result of a method call, then Preconditions should be skipped altogether, and the relevant exception should be conditionally thrown instead.

```java Bad theme={null} logger.log(Level.DEBUG, "Something went wrong: " + message); // Noncompliant; string concatenation performed even when log level too high to show DEBUG messages logger.fine("An exception occurred with message: " + message); // Noncompliant LOG.error("Unable to open file " + csvPath, e); // Noncompliant Preconditions.checkState(a > 0, "Arg must be positive, but got " + a); // Noncompliant. String concatenation performed even when a > 0 Preconditions.checkState(condition, formatMessage()); // Noncompliant. formatMessage() invoked regardless of condition Preconditions.checkState(condition, "message: %s", formatMessage()); // Noncompliant ``` ```java Fix theme={null} logger.log(Level.DEBUG, "Something went wrong: {0} ", message); // String formatting only applied if needed logger.log(Level.SEVERE, () -> "Something went wrong: " + message); // since Java 8, we can use Supplier , which will be evaluated lazily logger.fine("An exception occurred with message: {}", message); // SLF4J, Log4j LOG.error("Unable to open file {0}", csvPath, e); if (LOG.isDebugEnabled()) { LOG.debug("Unable to open file " + csvPath, e); // this is compliant, because it will not evaluate if log level is above debug. } Preconditions.checkState(arg > 0, "Arg must be positive, but got %d", a); // String formatting only applied if needed if (!condition) { throw new IllegalStateException(formatMessage()); // formatMessage() only invoked conditionally } if (!condition) { throw new IllegalStateException("message: " + formatMessage()); } ```

"equals" has a special place as a method name: it is expected to override boolean Object.equals(Object). Using the name for a method with some other signature is a recipe for confusion.

```java Bad theme={null} public void equals(MyObject o) { // Noncompliant //... } public bool equals(MyObject left, MyObject right) { // Noncompliant // ... } ``` ```java Fix theme={null} ```

There’s no need to null test in conjunction with an instanceof test. null is not an instanceof anything, so a null check is redundant.

```java Bad theme={null} if (x != null && x instanceof MyClass) { ... } // Noncompliant if (x == null || ! x instanceof MyClass) { ... } // Noncompliant ``` ```java Fix theme={null} if (x instanceof MyClass) { ... } if (! x instanceof MyClass) { ... } ```

Before records appeared in Java 16, there was a common way to represent getters for private fields of a class: a method named "get" with a capitalized field name. For example, for a \`String field named "myField" the signature of the getter method will be: public String getMyField()

In records, getters are named differently. Getters created by default do not contain the "get" prefix. So for a record’s String field "myField" the getter method will be: public String myField()\`

This means that if you want to override the default getter behavior it is better to use the method provided by records instead of creating a new one. Otherwise, this will bring confusion to the users of the record as two getters will be available and even leads to bugs if the behavior is different from the default one.

This rule raises an issue when a record contains a getter named "get" with a capitalized field name that is not behaving the same as the default one.

```java Bad theme={null} record Person(String name, int age) { public String getName() { // Noncompliant return name.toUpperCase(Locale.ROOT); } } ``` ```java Fix theme={null} record Person(String name, int age) { @Override public String name() { // Compliant return name.toUpperCase(Locale.ROOT); } } record Person(String name, int age) { public String getNameUpperCase() { // Compliant return name.toUpperCase(Locale.ROOT); } } record Person(String name, int age) { public String getName() { // Compliant, is equivalent to 'name()' return name; } } record Person(String name, int age) { @Override public String name() { // Compliant return name.toUpperCase(Locale.ROOT); } public String getName() { // Compliant, equal to 'name()' return name.toUpperCase(Locale.ROOT); } } ```

Consistent naming of beans is important for the readability and maintainability of the code. More precisely, according to the Spring documentation:

Naming beans consistently makes your configuration easier to read and understand, and if you are using Spring AOP it helps a lot when applying advice to a set of beans related by name.

Not following accepted conventions can introduce inconsistent naming, especially when multiple developers work on the same project, leading to technical debt.

The spring documentation establishes a naming convention that consists of camel-cased names with a leading lowercase letter.

This rule raises an issue when a bean name defined in one of the following annotations does not adhere to the naming convention:

@Bean
@Configuration
@Controller
@Component
@Qualifier
@Repository
@Service

```java Bad theme={null} @Bean(name = "MyBean") // Noncompliant, the first letter of the name should be lowercase public MyBean myBean() { ... ``` ```java Fix theme={null} @Bean(name = "myBean") // Compliant public MyBean myBean() { ... ```

There is no need to declare the same dependency or plugin twice in a project. In fact, doing so is likely to cause errors in the future when maintainers try to change or upgrade the plugin or dependency.

```java Bad theme={null} com.mygroup myartifact 1.0 runtime com.mygroup myartifact 1.0 jar ``` ```java Fix theme={null} ```

Using checked exceptions forces method callers to deal with errors, either by propagating them or by handling them. Throwing exceptions makes them fully part of the API of the method.

But to keep the complexity for callers reasonable, methods should not throw more than one kind of checked exception.

```java Bad theme={null} public void delete() throws IOException, SQLException { // Noncompliant /* ... */ } ``` ```java Fix theme={null} public void delete() throws SomeApplicationLevelException { /* ... */ } ```

A cast operation allows an object to be "converted" from one type to another. The compiler raises an error if it can determine that the target type is incompatible with the declared type of the object, otherwise it accepts the cast. However, depending on the actual runtime type of the object, a cast operation may fail at runtime. When a cast operation fails, a ClassCastException is thrown.

```java Bad theme={null} String hexString(Object o) { return Integer.toHexString((Integer) o); // Noncompliant if hexString is called with a String for example } ``` ```java Fix theme={null} String hexString(Integer i) { return Integer.toHexString(i); } ```

In Java, an enum is a special data type that allows you to define a set of constants. Nested enum types, also known as inner enum types, are enum types that are defined within another class or interface.

Nested enum types are implicitly static, so there is no need to declare them static explicitly.

```java Bad theme={null} public class Flower { static enum Color { // Noncompliant; static is redundant here RED, YELLOW, BLUE, ORANGE } // ... } ``` ```java Fix theme={null} public class Flower { enum Color { // Compliant RED, YELLOW, BLUE, ORANGE } // ... } ```

JNI (Java Native Interface) code should be used only as a last resort, since part of the point of using Java is to make applications portable, and by definition the use of JNI can reduce portability.

```java Bad theme={null} System.loadLibrary("nativeStringLib"); // Noncompliant ``` ```java Fix theme={null} ```

Marking an array \`volatile means that the array itself will always be read fresh and never thread cached, but the items in the array will not be. Similarly, marking a mutable object field volatile means the object reference is volatile but the object itself is not, and other threads may not see updates to the object state.

This can be salvaged with arrays by using the relevant AtomicArray class, such as AtomicIntegerArray, instead. For mutable objects, the volatile\` should be removed, and some other method should be used to ensure thread-safety, such as synchronization, or ThreadLocal storage.

```java Bad theme={null} private volatile int [] vInts; // Noncompliant private volatile MyObj myObj; // Noncompliant ``` ```java Fix theme={null} private AtomicIntegerArray vInts; private MyObj myObj; ```

Collection classes from the \`java.util.concurrent package have their own concurrency control mechanisms. A concurrent collection is thread-safe, but not governed by a single exclusion lock, therefore using an instance of such a class for synchronization is, at best unnecessary and at worst likely to have unintended consequences.

This rule raises an issue when a synchronization lock is used on an instance of one of the collection classes from the java.util.concurrent\` package.

```java Bad theme={null} public class Foo { private ConcurrentHashMap map = new ConcurrentHashMap<>(); public void bar() { synchronized(map) { // Noncompliant map.put("foo", "bar"); } // Do something ... } } ``` ```java Fix theme={null} public class Foo { private ConcurrentHashMap map = new ConcurrentHashMap<>(); public void bar() { map.put("foo", "bar"); // Do something ... } } ```

The Collection object returned by Map.entrySet(), Map.keySet() and Map.values(), and SequencedMap.sequencedEntrySet(), SequencedMap.sequencedKeySet() and SequencedMap.sequencedValues(), do not support the .add() and .addAll() methods, and they will throw an UnsupportedOperationException when invoked.

This rule raises an issue whenever .add() or .addAll() are invoked on collections that were retrieved this way.

```java Bad theme={null} Map map = new HashMap<>(); map.keySet().add(2); // Noncompliant, will throw UnsupportedOperationException map.keySet().addAll(List.of(1, 2, 3)); // Noncompliant, will throw UnsupportedOperationException SequencedMap sequencedMap = new LinkedHashMap<>(); sequencedMap.sequencedValues().add("1"); // Noncompliant, will throw UnsupportedOperationException sequencedMap.sequencedValues().addAll(List.of("1", "2", "3")); // Noncompliant, will throw UnsupportedOperationException ``` ```java Fix theme={null} ```

Java’s `import mechanism allows the use of simple class names. Therefore, using a class' fully qualified name in a file that import`s the class is redundant and confusing.

```java Bad theme={null} import java.util.List; import java.sql.Timestamp; //... java.util.List myList; // Noncompliant java.sql.Timestamp tStamp; // Noncompliant ``` ```java Fix theme={null} import java.util.List; import java.sql.Timestamp; //... List myList; Timestamp tStamp; ```

The @Value annotation does not guarantee that the property is defined. Particularly if a field or parameter is annotated as nullable, it indicates that the developer assumes that the property may be undefined.

An undefined property may lead to runtime exceptions when the Spring framework tries to inject the autowired dependency during bean creation.

This rule raises an issue when a nullable field or parameter is annotated with @Value and no default value is provided.

```java Bad theme={null} @Nullable @Value("${my.property}") // Noncompliant, no default value is provided, even though the field is nullable private String myProperty; ``` ```java Fix theme={null} @Nullable @Value("${my.property:#{null}}") // Compliant, a default value is provided private String myProperty; ```

An Iterable should not implement the Iterator interface or return this as an Iterator. The reason is that Iterator represents the iteration process itself, while Iterable represents the object we want to iterate over.

The Iterator instance encapsulates state information of the iteration process, such as the current and next element. Consequently, distinct iterations require distinct Iterator instances, for which Iterable provides the factory method Iterable.iterator().

This rule raises an issue when the Iterable.iterator() of a class implementing both Iterable and Iterator returns this.

```java Bad theme={null} class FooIterator implements Iterator, Iterable { private Foo[] seq; private int idx = 0; public boolean hasNext() { return idx < seq.length; } public Foo next() { return seq[idx++]; } public Iterator iterator() { return this; // Noncompliant } // ... } ``` ```java Fix theme={null} class FooSequence implements Iterable { private Foo[] seq; public Iterator iterator() { return new Iterator() { // Compliant private int idx = 0; public boolean hasNext() { return idx < seq.length; } public Foo next() { return seq[idx++]; } }; } // ... } ```

Shadowing parent class static methods by creating methods in child classes with the same signatures can result in seemingly strange behavior if an instance of the child class is cast to the parent class and the static method is invoked using a reference to the child class. In such cases, the parent class' code will be executed instead of the code in the child class, confusing callers and potentially causing hard-to-find bugs. Instead the child class method should be renamed.

```java Bad theme={null} public class Fruit { public static Double getCost() { return 3.5; } } public class Raspberry extends Fruit { public static Double GetCost() // Noncompliant { return 7.5; } } // ... var r = new Raspberry(); var f = (Fruit) r; System.out.println(r.GetCost()); // prints 7.5 System.out.println(f.GetCost()); // prints 3.5; there's only one instance but different code executes depending on cast ``` ```java Fix theme={null} public class Fruit { public static DoubleGetCost() { return 3.5; } } public class Raspberry extends Fruit { public static Double GetInflatedCost() { return 7.5; } } // ... var r = new Raspberry(); var f = (Fruit) r; System.out.println(r.GetCost()); // prints 3.5, Raspberry.GetCost() would be even better System.out.println(f.GetCost()); // prints 3.5; same code executes every time System.out.println(r.GetInflatedCost()); // prints 7.5, Raspberry.GetInflatedCost() would be even better ```

In Java 16 records represent a brief notation for immutable data structures. Records have autogenerated implementations for constructors with all parameters, getters, equals, hashcode and toString. Although these methods can still be overridden inside records, there is no use to do so if no special logic is required.

This rule reports an issue on empty compact constructors, trivial canonical constructors and simple getter methods with no additional logic.

```java Bad theme={null} record Person(String name, int age) { Person(String name, int age) { // Noncompliant, already autogenerated this.name = name; this.age = age; } } record Person(String name, int age) { Person { // Noncompliant, no need for empty compact constructor } public String name() { // Noncompliant, already autogenerated return name; } } ``` ```java Fix theme={null} record Person(String name, int age) { } // Compliant record Person(String name, int age) { Person(String name, int age) { // Compliant this.name = name.toLowerCase(Locale.ROOT); this.age = age; } } record Person(String name, int age) { Person { // Compliant if (age < 0) { throw new IllegalArgumentException("Negative age"); } } public String name() { // Compliant return name.toUpperCase(Locale.ROOT); } } ```

The Advanced Encryption Standard (AES) encryption algorithm can be used with various modes. Some combinations are not secured:

Electronic Codebook (ECB) mode: Under a given key, any given plaintext block always gets encrypted to the same ciphertext block. Thus, it does not hide data patterns well. In some senses, it doesn’t provide serious message confidentiality, and it is not recommended for use in cryptographic protocols at all.
Cipher Block Chaining (CBC) with PKCS#5 padding (or PKCS#7) is susceptible to padding oracle attacks.

In both cases, Galois/Counter Mode (GCM) with no padding should be preferred.

This rule raises an issue when a Cipher instance is created with either ECB or CBC/PKCS5Padding mode.

```java Bad theme={null} Cipher c1 = Cipher.getInstance("AES/ECB/NoPadding"); // Noncompliant Cipher c2 = Cipher.getInstance("AES/CBC/PKCS5Padding"); // Noncompliant ``` ```java Fix theme={null} Cipher c = Cipher.getInstance("AES/GCM/NoPadding"); ```

Whether the valid value ranges for \`Date fields start with 0 or 1 varies by field. For instance, month starts at 0, and day of month starts at 1. Enter a date value that goes past the end of the valid range, and the date will roll without error or exception. For instance, enter 12 for month, and you’ll get January of the following year.

This rule checks for bad values used in conjunction with java.util.Date, java.sql.Date, and java.util.Calendar\`. Specifically, values outside of the valid ranges:

Field	Valid
month	0-11
date (day)	0-31
hour	0-23
minute	0-60
second	0-61

Note that this rule does not check for invalid leap years, leap seconds (second = 61), or invalid uses of the 31st day of the month.

```java Bad theme={null} Date d = new Date(); d.setDate(25); d.setYear(2014); d.setMonth(12); // Noncompliant; rolls d into the next year Calendar c = new GregorianCalendar(2014, 12, 25); // Noncompliant if (c.get(Calendar.MONTH) == 12) { // Noncompliant; invalid comparison // ... } ``` ```java Fix theme={null} Date d = new Date(); d.setDate(25); d.setYear(2014); d.setMonth(11); Calendar c = new Gregorian Calendar(2014, 11, 25); if (c.get(Calendar.MONTH) == 11) { // ... } ```

When the code under test in a unit test throws an exception, the test itself fails. Therefore, there is no need to surround the tested code with a \`try-catch structure to detect failure. Instead, you can simply move the exception type to the method signature.

This rule raises an issue when there is a fail assertion inside a catch\` block.

Supported frameworks:

JUnit3
JUnit4
JUnit5
Fest assert
AssertJ

```java Bad theme={null} @Test public void testMethod() { try { // Some code } catch (MyException e) { Assert.fail(e.getMessage()); // Noncompliant } } ``` ```java Fix theme={null} @Test public void testMethod() throws MyException { // Some code } ```

When multiple tests differ only by a few hardcoded values they should be refactored as a single "parameterized" test. This reduces the chances of adding a bug and makes them more readable. Parameterized tests exist in most test frameworks (JUnit, TestNG, etc…).

The right balance needs of course to be found. There is no point in factorizing test methods when the parameterized version is a lot more complex than initial tests.

This rule raises an issue when at least 3 tests could be refactored as one parameterized test with less than 4 parameters. Only test methods which have at least one duplicated statement are considered.

```java Bad theme={null} import static org.junit.jupiter.api.Assertions.assertEquals; import org.junit.jupiter.api.Test; public class AppTest { @Test void test_not_null1() { // Noncompliant. The 3 following tests differ only by one hardcoded number. setupTax(); assertNotNull(getTax(1)); } @Test void test_not_null2() { setupTax(); assertNotNull(getTax(2)); } @Test void test_not_nul3l() { setupTax(); assertNotNull(getTax(3)); } @Test void testLevel1() { // Noncompliant. The 3 following tests differ only by a few hardcoded numbers. setLevel(1); runGame(); assertEquals(playerHealth(), 100); } @Test void testLevel2() { // Similar test setLevel(2); runGame(); assertEquals(playerHealth(), 200); } @Test void testLevel3() { // Similar test setLevel(3); runGame(); assertEquals(playerHealth(), 300); } } ``` ```java Fix theme={null} import static org.junit.jupiter.api.Assertions.assertEquals; import org.junit.jupiter.params.ParameterizedTest; import org.junit.jupiter.params.provider.CsvSource; public class AppTest { @ParameterizedTest @ValueSource(ints = {1, 2, 3}) void test_not_null(int arg) { setupTax(); assertNotNull(getTax(arg)); } @ParameterizedTest @CsvSource({ "1, 100", "2, 200", "3, 300", }) void testLevels(int level, int health) { setLevel(level); runGame(); assertEquals(playerHealth(), health); } } ```

The Collection.toArray() method returns an Object\[] when no arguments are provided to it. This can lead to a ClassCastException at runtime if you try to cast the returned array to an array of a specific type. Instead, use this method by providing an array of the desired type as the argument.

Note that passing a new T\[0] array of length zero as the argument is more efficient than a pre-sized array new T\[size].

```java Bad theme={null} public String [] getStringArray(List strings) { return (String []) strings.toArray(); // Noncompliant, a ClassCastException will be thrown here } ``` ```java Fix theme={null} public String [] getStringArray(List strings) { return strings.toArray(new String[0]); // Compliant, the toArray method will return an array of the desired type, and we can remove the casting operation } public String [] getPresizedStringArray(List strings) { return strings.toArray(new String[strings.size()]); // Compliant, but slightly less efficient than the previous example } ```

The Java regex engine uses recursive method calls to implement backtracking. Therefore when a repetition inside a regular expression contains multiple paths (i.e. the body of the repetition contains an alternation (\`|), an optional element or another repetition), trying to match the regular expression can cause a stack overflow on large inputs. This does not happen when using a possessive quantifier (such as *+ instead of *) or when using a character class inside a repetition (e.g. \[ab]* instead of (a|b)*).

The size of the input required to overflow the stack depends on various factors, including of course the stack size of the JVM. One thing that significantly increases the size of the input that can be processed is if each iteration of the repetition goes through a chain of multiple constant characters because such consecutive characters will be matched by the regex engine without invoking any recursion.

For example, on a JVM with a stack size of 1MB, the regex (?:a|b)\* will overflow the stack after matching around 6000 characters (actual numbers may differ between JVM versions and even across multiple runs on the same JVM) whereas (?:abc|def)\* can handle around 15000 characters.

Since often times stack growth can’t easily be avoided, this rule will only report issues on regular expressions if they can cause a stack overflow on realistically sized inputs. You can adjust the maxStackConsumptionFactor\` parameter to adjust this.

```java Bad theme={null} Pattern.compile("(a|b)*"); // Noncompliant Pattern.compile("(.|\n)*"); // Noncompliant Pattern.compile("(ab?)*"); // Noncompliant ``` ```java Fix theme={null} Pattern.compile("[ab]*"); // Character classes don't cause recursion the way that '|' does Pattern.compile("(?s).*"); // Enabling the (?s) flag makes '.' match line breaks, so '|\n' isn't necessary Pattern.compile("(ab?)*+"); // Possessive quantifiers don't cause recursion because they disable backtracking ```

2 ActionSupport is security-sensitive. For example, their use has led in the past to the following vulnerabilities:

CVE-2012-1006

All classes extending com.opensymphony.xwork2.ActionSupport are potentially remotely reachable. An action class extending ActionSupport will receive all HTTP parameters sent and these parameters will be automatically mapped to the setters of the Struts 2 action class. One should review the use of the fields set by the setters, to be sure they are used safely. By default, they should be considered as untrusted inputs.

```java Bad theme={null} public class AccountBalanceAction extends ActionSupport { private static final long serialVersionUID = 1L; private Integer accountId; // this setter might be called with user input public void setAccountId(Integer accountId) { this.accountId = accountId; } @Override public String execute() throws Exception { // call a service to get the account's details and its balance [...] return SUCCESS; } } ``` ```java Fix theme={null} ```

This rule allows you to track the usage of the @SuppressWarnings mechanism.

```java Bad theme={null} @SuppressWarnings("unused") @SuppressWarnings("unchecked") // Noncompliant ``` ```java Fix theme={null} ```

Assuming that a comparator or compareTo method always returns -1 or 1 if the first operand is less than or greater than the second is incorrect.

The specifications for both methods, Comparator.compare and Comparable.compareTo, state that their return value is "a negative integer, zero, or a positive integer as this object is less than, equal to, or greater than the specified object." Even if a specific comparator always returns -1, 0, or 1, this is only an implementation detail, not the API contract developers can rely on.

```java Bad theme={null} public class Main { boolean isAGreaterThanB(Comparable a, Integer b) { return a.compareTo(b) == 1; // Noncompliant, check for constant return value } public static void main(String[] args) { ByteComparator comparator = new ByteComparator(); if (comparator.compare((byte) 23, (byte) 42) == -1) { // Noncompliant, check for constant return value System.out.println("23 < 42"); } else { System.out.println("23 >= 42"); } } static class ByteComparator implements Comparator { @Override public int compare(Byte a, Byte b) { return a - b; } } } ``` ```java Fix theme={null} public class Main { boolean isAGreaterThanB(Comparable a, Integer b) { return a.compareTo(b) > 0; // Compliant, check for positive return value } public static void main(String[] args) { ByteComparator comparator = new ByteComparator(); if (comparator.compare((byte) 23, (byte) 42) < 0) { // Compliant, check for negative return value System.out.println("23 < 42"); } else { System.out.println("23 >= 42"); } } static class ByteComparator implements Comparator { @Override public int compare(Byte a, Byte b) { return a - b; } } } ```

Correctly updating a \`static field from a non-static method is tricky to get right and could easily lead to bugs if there are multiple class instances and/or multiple threads in play. Ideally, static fields are only updated from synchronized static methods.

This rule raises an issue each time a static\` field is updated from a non-static method.

```java Bad theme={null} public class MyClass { private static int count = 0; public void doSomething() { //... count++; // Noncompliant } } ``` ```java Fix theme={null} ```

When an object is marked as static, it means that it belongs to the class rather than any class instance. This means there is only one copy of the static object in memory, regardless of how many class instances are created. Static objects are shared among all instances of the class and can be accessed using the class name rather than an instance of the class.

A data type is considered thread-safe if it can be used correctly by multiple threads, regardless of how those threads are executed, without requiring additional coordination from the calling code. In other words, a thread-safe data type can be accessed and modified by multiple threads simultaneously without causing any issues or requiring extra work from the programmer to ensure correct behavior.

Non-thread-safe objects are objects that are not designed to be used in a multi-threaded environment and can lead to race conditions and data inconsistencies when accessed by multiple threads simultaneously. Using them in a multi-threaded manner is highly likely to cause data problems or exceptions at runtime.

When a non-thread-safe object is marked as static in a multi-threaded environment, it can cause issues because the non-thread-safe object will be shared across different instances of the containing class.

This rule raises an issue when any of the following instances and their subtypes are marked as static:

java.util.Calendar,
java.text.DateFormat,
javax.xml.xpath.XPath, or
javax.xml.validation.SchemaFactory.

```java Bad theme={null} public class MyClass { private static Calendar calendar = Calendar.getInstance(); // Noncompliant private static SimpleDateFormat format = new SimpleDateFormat("HH-mm-ss"); // Noncompliant } ``` ```java Fix theme={null} public class MyClass { private Calendar calendar = Calendar.getInstance(); private SimpleDateFormat format = new SimpleDateFormat("HH-mm-ss"); } ```

There is no requirement that class names be unique, only that they be unique within a package. Therefore trying to determine an object’s type based on its class name is an exercise fraught with danger. One of those dangers is that a malicious user will send objects of the same name as the trusted class and thereby gain trusted access.

Instead, the instanceof operator or the Class.isAssignableFrom() method should be used to check the object’s underlying type.

```java Bad theme={null} package computer; class Pear extends Laptop { ... } package food; class Pear extends Fruit { ... } class Store { public boolean hasSellByDate(Object item) { if ("Pear".equals(item.getClass().getSimpleName())) { // Noncompliant return true; // Results in throwing away week-old computers } return false; } public boolean isList(Class valueClass) { if (List.class.getName().equals(valueClass.getName())) { // Noncompliant return true; } return false; } } ``` ```java Fix theme={null} class Store { public boolean hasSellByDate(Object item) { if (item instanceof food.Pear) { return true; } return false; } public boolean isList(Class valueClass) { if (valueClass.isAssignableFrom(List.class)) { return true; } return false; } } ```

"A rose by any other name would smell as sweet," but main by any other name would not. Just because a method has the name "main", that doesn’t make it the entry point to an application. It must also have the correct signature. Specifically, it must be public static void and accept a single String \[] as an argument.

```java Bad theme={null} public void main(String arg) { // Noncompliant // ... } ``` ```java Fix theme={null} public static void main(String [] args) { // ... } ```

When the modulus of a negative number is calculated, the result will either be negative or zero. Thus, comparing the modulus of a variable for equality with a positive number (or a negative one) could result in unexpected results.

```java Bad theme={null} public boolean isOdd(int x) { return x % 2 == 1; // Noncompliant; if x is an odd negative, x % 2 == -1 } ``` ```java Fix theme={null} public boolean isOdd(int x) { return x % 2 != 0; } ```

Disclosure of version information, usually overlooked by developers but disclosed by default by the systems and frameworks in use, can pose a significant security risk depending on the production environement.

Once this information is public, attackers can use it to identify potential security holes or vulnerabilities specific to that version.

Furthermore, if the published version information indicates the use of outdated or unsupported software, it becomes easier for attackers to exploit known vulnerabilities. They can search for published vulnerabilities related to that version and launch attacks that specifically target those vulnerabilities.

```java Bad theme={null} @GetMapping(value = "/example") public ResponseEntity example() { HttpHeaders responseHeaders = new HttpHeaders(); responseHeaders.set("x-powered-by", "myproduct"); // Sensitive return new ResponseEntity( "example", responseHeaders, HttpStatus.CREATED); } ``` ```java Fix theme={null} ```

In object-oriented programming, inappropriately accessing static members of a base class via derived types is considered a code smell.

Static members are associated with the class itself, not with any specific instance of the class or its children classes. Accessing through the wrong type suggests a misunderstanding of the ownership and role of this member. This can make the maintenance of the code more complicated.

Therefore, the access should be done directly through the base class to maintain clarity and avoid potential misunderstandings.

```java Bad theme={null} class Parent { public static int counter; } class Child extends Parent { public Child() { Child.counter++; // Noncompliant } } ``` ```java Fix theme={null} class Parent { public static int counter; } class Child extends Parent { public Child() { Parent.counter++; } } ```

Code is sometimes annotated as deprecated by developers maintaining libraries or APIs to indicate that the method, class, or other programming element is no longer recommended for use. This is typically due to the introduction of a newer or more effective alternative. For example, when a better solution has been identified, or when the existing code presents potential errors or security risks.

Deprecation is a good practice because it helps to phase out obsolete code in a controlled manner, without breaking existing software that may still depend on it. It is a way to warn other developers not to use the deprecated element in new code, and to replace it in existing code when possible.

Deprecated classes, interfaces, and their members should not be used, inherited or extended because they will eventually be removed. The deprecation period allows you to make a smooth transition away from the aging, soon-to-be-retired technology.

Check the documentation or the deprecation message to understand why the code was deprecated and what the recommended alternative is.

```java Bad theme={null} /** * @deprecated As of release 1.3, replaced by {@link #Foo} */ @Deprecated public class Fum { ... } public class Foo { /** * @deprecated As of release 1.7, replaced by {@link #newMethod()} */ @Deprecated public void oldMethod() { ... } public void newMethod() { ... } } public class Bar extends Foo { public void oldMethod() { ... } // Noncompliant; don't override a deprecated method } public class Baz extends Fum { // Noncompliant; Fum is deprecated public void myMethod() { Foo foo = new Foo(); foo.oldMethod(); // Noncompliant; oldMethod method is deprecated } } ``` ```java Fix theme={null} ```

User enumeration refers to the ability to guess existing usernames in a web application database. This can happen, for example, when using "sign-in/sign-on/forgot password" functionalities of a website.

When an user tries to "sign-in" to a website with an incorrect username/login, the web application should not disclose that the username doesn’t exist with a message similar to "this username is incorrect", instead a generic message should be used like "bad credentials", this way it’s not possible to guess whether the username or password was incorrect during the authentication.

If a user-management feature discloses information about the existence of a username, attackers can use brute force attacks to retrieve a large amount of valid usernames that will impact the privacy of corresponding users and facilitate other attacks (phishing, password guessing etc …).

```java Bad theme={null} public String authenticate(String username, String password) throws AuthenticationException { Details user = null; try { user = loadUserByUsername(username); } catch (UsernameNotFoundException | DataAccessException e) { // Hide this exception reason to not disclose that the username doesn't exist } if (user == null || !user.isPasswordCorrect(password)) { // User should not be able to guess if the bad credentials message is related to the username or the password throw new BadCredentialsException("Bad credentials"); } } ``` ```java Fix theme={null} DaoAuthenticationProvider daoauth = new DaoAuthenticationProvider(); daoauth.setUserDetailsService(new MyUserDetailsService()); daoauth.setPasswordEncoder(new BCryptPasswordEncoder()); daoauth.setHideUserNotFoundExceptions(true); // Compliant builder.authenticationProvider(daoauth); ```

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. In the context of a WebView, JavaScript code can exfiltrate local files that might be sensitive or even worse, access exposed functions of the application that can result in more severe vulnerabilities such as code injection. Thus JavaScript support should not be enabled for WebViews unless it is absolutely necessary and the authenticity of the web resources can be guaranteed.

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

This rule verifies that single-line comments are not located at the ends of lines of code. The main idea behind this rule is that in order to be really readable, trailing comments would have to be properly written and formatted (correct alignment, no interference with the visual structure of the code, not too long to be visible) but most often, automatic code formatters would not handle this correctly: the code would end up less readable. Comments are far better placed on the previous empty line of code, where they will always be visible and properly formatted.

```java Bad theme={null} int a1 = b + c; // This is a trailing comment that can be very very long ``` ```java Fix theme={null} // This very long comment is better placed before the line of code int a2 = b + c; ```

While it is possible to access static members from a class instance, it’s bad form, and considered by most to be misleading because it implies to the readers of your code that there’s an instance of the member per class instance.

```java Bad theme={null} public class A { public static int counter = 0; } public class B { private A first = new A(); private A second = new A(); public void runUpTheCount() { first.counter ++; // Noncompliant second.counter ++; // Noncompliant. A.counter is now 2, which is perhaps contrary to expectations } } ``` ```java Fix theme={null} public class A { public static int counter = 0; } public class B { private A first = new A(); private A second = new A(); public void runUpTheCount() { A.counter ++; // Compliant A.counter ++; // Compliant } } ```

Hardcoding IP addresses is security-sensitive. It has led in the past to the following vulnerabilities:

Today’s services have an ever-changing architecture due to their scaling and redundancy needs. It is a mistake to think that a service will always have the same IP address. When it does change, the hardcoded IP will have to be modified too. This will have an impact on the product development, delivery, and deployment:

The developers will have to do a rapid fix every time this happens, instead of having an operation team change a configuration file.
It misleads to use the same address in every environment (dev, sys, qa, prod).

Last but not least it has an effect on application security. Attackers might be able to decompile the code and thereby discover a potentially sensitive address. They can perform a Denial of Service attack on the service, try to get access to the system, or try to spoof the IP address to bypass security checks. Such attacks can always be possible, but in the case of a hardcoded IP address solving the issue will take more time, which will increase an attack’s impact.

```java Bad theme={null} String ip = System.getenv("IP_ADDRESS"); // Compliant Socket socket = new Socket(ip, 6667); ``` ```java Fix theme={null} ```

A cookie’s domain specifies which websites should be able to read it. Left blank, browsers are supposed to only send the cookie to sites that exactly match the sending domain. For example, if a cookie was set by lovely.dream.com, it should only be readable by that domain, and not by nightmare.com or even strange.dream.com. If you want to allow sub-domain access for a cookie, you can specify it by adding a dot in front of the cookie’s domain, like so: .dream.com. But cookie domains should always use at least two levels.

Cookie domains can be set either programmatically or via configuration. This rule raises an issue when any cookie domain is set with a single level, as in .com.

```java Bad theme={null} Cookie myCookie = new Cookie("name", "val"); // Compliant; by default, cookies are only returned to the server that sent them. // or Cookie myCookie = new Cookie("name", "val"); myCookie.setDomain(".myDomain.com"); // Compliant java.net.HttpCookie myOtherCookie = new java.net.HttpCookie("name", "val"); myOtherCookie.setDomain(".myDomain.com"); // Compliant ``` ```java Fix theme={null} ```

Developers often use TODO tags to mark areas in the code where additional work or improvements are needed but are not implemented immediately. However, these TODO tags sometimes get overlooked or forgotten, leading to incomplete or unfinished code. This rule aims to identify and address unattended TODO tags to ensure a clean and maintainable codebase. This description explores why this is a problem and how it can be fixed to improve the overall code quality.

```java Bad theme={null} void doSomething() { // TODO } ``` ```java Fix theme={null} ```

Storing files locally is a common task for mobile applications. Files that are stored unencrypted can be read out and modified by an attacker with physical access to the device. Access to sensitive data can be harmful for the user of the application, for example when the device gets stolen.

```java Bad theme={null} String masterKeyAlias = MasterKeys.getOrCreate(MasterKeys.AES256_GCM_SPEC); File file = new File(context.getFilesDir(), "secret_data"); EncryptedFile encryptedFile = EncryptedFile.Builder( file, context, masterKeyAlias, EncryptedFile.FileEncryptionScheme.AES256_GCM_HKDF_4KB ).build(); // write to the encrypted file FileOutputStream encryptedOutputStream = encryptedFile.openFileOutput(); ``` ```java Fix theme={null} ```

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 the same character is listed twice in the same character class or if the character class contains overlapping ranges, this has no effect.

Thus duplicate characters in a character class are either a simple oversight or a sign that a range in the character class matches more than is intended or that the author misunderstood how character classes work and wanted to match more than one character. A common example of the latter mistake is trying to use a range like \[0-99] to match numbers of up to two digits, when in fact it is equivalent to \[0-9]. Another common cause is forgetting to escape the - character, creating an unintended range that overlaps with other characters in the character class.

```java Bad theme={null} str.matches("[0-99]") // Noncompliant, this won't actually match strings with two digits str.matches("[0-9.-_]") // Noncompliant, .-_ is a range that already contains 0-9 (as well as various other characters such as capital letters) ``` ```java Fix theme={null} str.matches("[0-9]{1,2}") str.matches("[0-9.\\-_]") ```

Nested code blocks create new scopes where variables declared within are inaccessible from the outside, and their lifespan ends with the block.

Although this may appear beneficial, their usage within a function often suggests that the function is overloaded. Thus, it may violate the Single Responsibility Principle, and the function needs to be broken down into smaller functions.

The presence of nested blocks that don’t affect the control flow might suggest possible mistakes in the code.

```java Bad theme={null} class Example { private final Deque stack = new LinkedList<>(); public void evaluate(int operator) { switch (operator) { case ADD: { /* ... */ { // Noncompliant - Extract this nested code block into a method int a = stack.pop(); int b = stack.pop(); int result = a + b; stack.push(result); } /* ... */ break; } /* ... */ } } } ``` ```java Fix theme={null} class Example { private final Deque stack = new LinkedList<>(); public void evaluate(int operator) { switch (operator) { case ADD: { /* ... */ evaluateAdd(); /* ... */ break; } /* ... */ } } private void evaluateAdd() { int a = stack.pop(); int b = stack.pop(); int result = a + b; stack.push(result); } } ```

If an alternative in a regular expression only matches things that are already matched by another alternative, that alternative is redundant and serves no purpose.

In the best case this means that the offending subpattern is merely redundant and should be removed. In the worst case it’s a sign that this regex does not match what it was intended to match and should be reworked.

```java Bad theme={null} "[ab]|a" // Noncompliant: the "|a" is redundant because "[ab]" already matches "a" ".*|a" // Noncompliant: .* matches everything, so any other alternative is redundant ``` ```java Fix theme={null} "[ab]" ".*" ```

Shared naming conventions make it possible for a team to collaborate efficiently. Following the established convention of single-letter type parameter names helps users and maintainers of your code quickly see the difference between a type parameter and a poorly named class.

This rule check that all type parameter names match a provided regular expression. The following code snippets use the default regular expression.

```java Bad theme={null} public class MyClass { // Noncompliant void method(TYPE t) { // Noncompliant } } ``` ```java Fix theme={null} public class MyClass { void method(T t) { } } ```

A naming convention in software development is a set of guidelines for naming code elements like variables, functions, and classes.

The goal of a naming convention is to make the code more readable and understandable, which makes it easier to maintain and debug. It also ensures consistency in the code, especially when multiple developers are working on the same project.

This rule checks that field names match a provided regular expression.

```java Bad theme={null} class MyClass { private int my_field; } ``` ```java Fix theme={null} class MyClass { private int myField; } ```

This rule raises an issue when a \{visibility} \{operationName} is never referenced in the code.

```java Bad theme={null} public class Foo implements Serializable { public static void doSomething() { Foo foo = new Foo(); ... } private void unusedPrivateMethod() {...} private void writeObject(ObjectOutputStream s) {...} //Compliant, relates to the java serialization mechanism private void readObject(ObjectInputStream in) {...} //Compliant, relates to the java serialization mechanism } ``` ```java Fix theme={null} public class Foo implements Serializable { public static void doSomething(){ Foo foo = new Foo(); ... } private void writeObject(ObjectOutputStream s) {...} //Compliant, relates to the java serialization mechanism private void readObject(ObjectInputStream in) {...} //Compliant, relates to the java serialization mechanism } ```

There is no good excuse for an empty class. If it’s being used simply as a common extension point, it should be replaced with an interface. If it was stubbed in as a placeholder for future development it should be fleshed-out. In any other case, it should be eliminated.

```java Bad theme={null} public interface Nothing { } ``` ```java Fix theme={null} @Configuration @EnableWebMvc public final class ApplicationConfiguration { } ```

In Unix file system permissions, the "`others`" category refers to all users except the owner of the file system resource and the members of the group assigned to this resource.

Granting permissions to this category can lead to unintended access to files or directories that could allow attackers to obtain sensitive information, disrupt services or elevate privileges.

```java Bad theme={null} public void setPermissionsSafe(String filePath) throws IOException { Set perms = new HashSet(); // user permission perms.add(PosixFilePermission.OWNER_READ); perms.add(PosixFilePermission.OWNER_WRITE); perms.add(PosixFilePermission.OWNER_EXECUTE); // group permissions perms.add(PosixFilePermission.GROUP_READ); perms.add(PosixFilePermission.GROUP_EXECUTE); // others permissions removed perms.remove(PosixFilePermission.OTHERS_READ); // Compliant perms.remove(PosixFilePermission.OTHERS_WRITE); // Compliant perms.remove(PosixFilePermission.OTHERS_EXECUTE); // Compliant Files.setPosixFilePermissions(Paths.get(filePath), perms); } ``` ```java Fix theme={null} ```

There’s no point in forcing the overhead of a method call for a method that always returns the same constant value. Even worse, the fact that a method call must be made will likely mislead developers who call the method thinking that something more is done. Declare a constant instead.

This rule raises an issue if on methods that contain only one statement: the return of a constant value.

```java Bad theme={null} static final int BEST_NUMBER = 12; ``` ```java Fix theme={null} ```

Empty statements represented by a semicolon ; are statements that do not perform any operation. They are often the result of a typo or a misunderstanding of the language syntax. It is a good practice to remove empty statements since they don’t add value and lead to confusion and errors.

```java Bad theme={null} void doSomething() { ; // Noncompliant - was used as a kind of TODO marker } void doSomethingElse() { System.out.println("Hello, world!");; // Noncompliant - double ; // ... } ``` ```java Fix theme={null} void doSomething() {} void doSomethingElse() { System.out.println("Hello, world!"); // ... for (int i = 0; i < 3; i++) ; // Compliant if unique statement of a loop // ... } ```

Security through obscurity is no security at all, and the use of Base64 encoding to obscure a password will only slow an attacker down for seconds, at the most. Instead, passwords should be encrypted with private keys that are at least 128 bits in length.

This rule checks for the use of Base64 decoding on values that are then used in database and LDAP connections.

```java Bad theme={null} String password = Base64.decode(retrievePassword()); DriverManager.getConnection(url, usr, password); // Noncompliant ``` ```java Fix theme={null} Cipher cipher = Cipher.getInstance("RSA/ECB/OAEPWithMD5AndMGF1Padding"); cipher.init(Cipher.DECRYPT_MODE, privateKey); // key management out of scope for this example cipher.doFinal(retrievePassword())); ```

Having a variable with the same name in two unrelated classes is fine, but do the same thing within a class hierarchy and you’ll get confusion at best, chaos at worst.

```java Bad theme={null} public class Fruit { protected Season ripe; protected Color flesh; // ... } public class Raspberry extends Fruit { private boolean ripe; // Noncompliant private static Color FLESH; // Noncompliant } ``` ```java Fix theme={null} public class Fruit { protected Season ripe; protected Color flesh; // ... } public class Raspberry extends Fruit { private boolean ripened; private static Color FLESH_COLOR; } ```

When the execution is not explicitly terminated at the end of a switch case, it continues to execute the statements of the following case. While this is sometimes intentional, it often is a mistake which leads to unexpected behavior.

```java Bad theme={null} switch (myVariable) { case 0: // Empty case used to specify the same behavior for a group of cases. case 1: doSomething(); break; case 2: // Use of a fallthrough comment // fallthrough case 3: // Use of return statement return; case 4: // Use of throw statement throw new IllegalStateException(); case 5: // Use of continue statement continue; default: // For the last case, use of break statement is optional doSomethingElse(); } ``` ```java Fix theme={null} ```

Testing for loop termination using an equality operator (== and !=) is dangerous, because it could set up an infinite loop. Using a broader relational operator instead casts a wider net, and makes it harder (but not impossible) to accidentally write an infinite loop.

```java Bad theme={null} for (int i = 1; i != 10; i += 2) // Noncompliant. Infinite; i goes from 9 straight to 11. { //... } ``` ```java Fix theme={null} for (int i = 1; i <= 10; i += 2) // Compliant { //... } ```

Enabling runFinalizersOnExit is unsafe as it might result in erratic behavior and deadlocks on application exit.

Indeed, finalizers might be force-called on live objects while other threads are concurrently manipulating them.

Instead, if you want to execute something when the virtual machine begins its shutdown sequence, you should attach a shutdown hook.

```java Bad theme={null} public static void main(String [] args) { System.runFinalizersOnExit(true); // Noncompliant } protected void finalize(){ doShutdownOperations(); } ``` ```java Fix theme={null} public static void main(String [] args) { Thread myThread = new Thread( () -> { doShutdownOperations(); }); Runtime.getRuntime().addShutdownHook(myThread); } ```

Although they don’t affect the runtime behavior of the application after compilation, removing them will:

Improve the readability and maintainability of the code.
Help avoid potential naming conflicts.
Improve the build time, as the compiler has fewer lines to read and fewer types to resolve.
Reduce the number of items the code editor will show for auto-completion, thereby showing fewer irrelevant suggestions.

```java Bad theme={null} package myapp.helpers; import java.io.IOException; import java.nio.file.*; import java.nio.file.*; // Noncompliant - package is imported twice import java.lang.Runnable; // Noncompliant - java.lang is imported by default public class FileHelper { public static String readFirstLine(String filePath) throws IOException { return Files.readAllLines(Paths.get(filePath)).get(0); } } ``` ```java Fix theme={null} package myapp.helpers; import java.io.IOException; import java.nio.file.*; public class FileHelper { public static String readFirstLine(String filePath) throws IOException { return Files.readAllLines(Paths.get(filePath)).get(0); } } ```

Files with no lines of code clutter a project and should be removed.

```java Bad theme={null} //package org.foo; // //public class Bar {} ``` ```java Fix theme={null} ```

Cryptographic hash algorithms such as MD2, MD4, MD5, MD6, HAVAL-128, HMAC-MD5, DSA (which uses SHA-1), RIPEMD, RIPEMD-128, RIPEMD-160, HMACRIPEMD160 and SHA-1 are no longer considered secure, because it is possible to have collisions (little computational effort is enough to find two or more different inputs that produce the same hash).

```java Bad theme={null} MessageDigest md1 = MessageDigest.getInstance("SHA-512"); // Compliant ``` ```java Fix theme={null} ```

Ternary expressions, while concise, can often lead to code that is difficult to read and understand, especially when they are nested or complex. Prioritizing readability fosters maintainability and reduces the likelihood of bugs. Therefore, they should be removed in favor of more explicit control structures, such as if/else statements, to improve the clarity and readability of the code.

```java Bad theme={null} System.out.println(i>10?"yes":"no"); // Noncompliant ``` ```java Fix theme={null} if (i > 10) { System.out.println("yes"); } else { System.out.println("no"); } ```

The \`catch block of a checked exception "E" may be hidden because the corresponding try block only throws exceptions derived from E.

These derived exceptions are handled in dedicated catch blocks prior to the catch block of the base exception E.

The catch\` block of E is unreachable and should be considered dead code. It should be removed, or the entire try-catch structure should be refactored.

It is also possible that a single exception type in a multi-catch block may be hidden while the catch block itself is still reachable. In that case it is enough to only remove the hidden exception type or to replace it with another type.

```java Bad theme={null} public class HiddenCatchBlock { public static class CustomException extends Exception { } public static class CustomDerivedException extends CustomException { } public static void main(String[] args) { try { throwCustomDerivedException(); } catch(CustomDerivedException e) { // ... } catch(CustomException e) { // Noncompliant; this code is unreachable // ... } } private static void throwCustomDerivedException() throws CustomDerivedException { throw new CustomDerivedException(); } } ``` ```java Fix theme={null} public class HiddenCatchBlock { public static class CustomException extends Exception { } public static class CustomDerivedException extends CustomException { } public static void main(String[] args) { try { throwCustomDerivedException(); } catch(CustomDerivedException e) { // Compliant; try-catch block is "catching" only the Exception that can be thrown in the "try" //... } } } ```

Possessive quantifiers in Regex patterns like below improve performance by eliminating needless backtracking:

?+ , \*+ , ++ , \{n}+ , \{n,}+ , \{n,m}+

But because possessive quantifiers do not keep backtracking positions and never give back, the following sub-patterns should not match only similar characters. Otherwise, possessive quantifiers consume all characters that could have matched the following sub-patterns and nothing remains for the following sub-patterns.

```java Bad theme={null} Pattern pattern1 = Pattern.compile("a++abc"); // Noncompliant, the second 'a' never matches Pattern pattern2 = Pattern.compile("\\d*+[02468]"); // Noncompliant, the sub-pattern "[02468]" never matches ``` ```java Fix theme={null} Pattern pattern1 = Pattern.compile("aa++bc"); // Compliant, for example it can match "aaaabc" Pattern pattern2 = Pattern.compile("\\d*+(?<=[02468])"); // Compliant, for example it can match an even number like "1234" ```

Libraries used to unarchive a file (zip, bzip2, tar, …) do what they were made for: they extract the content of the archive blindly, creating on the filesystem directories and files corresponding exactly to the content of the archive. Using a specially crafted archive containing some path traversal filenames, it is possible to create directories/files outside of the dir where the archive is extracted. This can lead to overwriting an executable or a configuration file with a file containing malicious code and transform a simple archive into a way to execute arbitrary code.

```java Bad theme={null} Enumeration entries = zipFile.entries(); while (entries.hasMoreElements()) { ZipEntry entry = entries.nextElement(); File extractedFile = new File(toDir, entry.getName()); FileOutputStream fos = new FileOutputStream(extractedFile); // Noncompliant; entry.getName() that was used to created "extractedFile" may be tainted with "../../../../../../../../tmp/evil.sh" InputStream input = zipFile.getInputStream(entry); IOUtils.copy(input, fos); } ``` ```java Fix theme={null} Enumeration entries = zipFile.entries(); while (entries.hasMoreElements()) { ZipEntry zipEntry = entries.nextElement(); String fileName = zipEntry.getName(); File extractedFile = new File(toDir, fileName); String canonicalDirPath = toDir.getCanonicalPath(); String canonicalDestPath = extractedFile.getCanonicalPath(); sanitizeAgainstZipFlipVulnerability(fileName, canonicalDestPath, canonicalDirPath); // Compliant FileOutputStream fos = new FileOutputStream(extractedFile); InputStream input = zipFile.getInputStream(entry); IOUtils.copy(input, fos); } public static void sanitizeAgainstZipFlipVulnerability(String fileName, String canonicalDestPath, String canonicalDirPath) throws ArchiverException { if (fileName.indexOf("..") != -1 && !canonicalDestPath.startsWith(canonicalDirPath + File.separator)) { // Sanitizer throw new ArchiverException("The file " + fileName + " is trying to leave the target output directory."); } } ```

There are several reasons to use a group in a regular expression:

to change the precedence (e.g. do(g|or) will match 'dog' and 'door')
to remember parenthesised part of the match in the case of capturing group
to improve readability

In any case, having an empty group is most probably a mistake. Either it is a leftover after refactoring and should be removed, or the actual parentheses were intended and were not escaped.

```java Bad theme={null} "foo()" // Noncompliant, will match only 'foo' ``` ```java Fix theme={null} "foo\$\$" // Matches 'foo()' ```

The use of a non-standard algorithm is dangerous because a determined attacker may be able to break the algorithm and compromise whatever data has been protected. Standard algorithms like \`SHA-256, SHA-384, SHA-512, … should be used instead.

This rule tracks creation of java.security.MessageDigest\` subclasses.

```java Bad theme={null} MessageDigest digest = MessageDigest.getInstance("SHA-256"); ``` ```java Fix theme={null} ```

The requirement for a final default clause is defensive programming. The clause should either take appropriate action, or contain a suitable comment as to why no action is taken.

```java Bad theme={null} switch (param) { //missing default clause case 0: doSomething(); break; case 1: doSomethingElse(); break; } switch (param) { default: // default clause should be the last one error(); break; case 0: doSomething(); break; case 1: doSomethingElse(); break; } ``` ```java Fix theme={null} switch (param) { case 0: doSomething(); break; case 1: doSomethingElse(); break; default: error(); break; } ```

There are valid cases for passing a variable multiple times into the same method call, but usually doing so is a mistake, and something else was intended for one of the arguments.

```java Bad theme={null} if (compare(myPoint.x, myPoint.x) != 0) { // Noncompliant //... } if (compare(getNextValue(), getNextValue()) != 0) { // Noncompliant // ... } ``` ```java Fix theme={null} if (compare(myPoint.x, myPoint.y) != 0) { //... } Object v1 = getNextValue(); Object v2 = getNextValue(); if (compare(v1, v2) != 0) { // ... } ```

In Android applications, broadcasting intents is security-sensitive. For example, it has led in the past to the following vulnerability:

CVE-2018-9489

By default, broadcasted intents are visible to every application, exposing all sensitive information they contain.

This rule raises an issue when an intent is broadcasted without specifying any "receiver permission".

```java Bad theme={null} import android.content.BroadcastReceiver; import android.content.Context; import android.content.Intent; import android.os.Build; import android.os.Bundle; import android.os.Handler; import android.os.UserHandle; import android.support.annotation.RequiresApi; public class MyIntentBroadcast { @RequiresApi(api = Build.VERSION_CODES.JELLY_BEAN_MR1) public void broadcast(Intent intent, Context context, UserHandle user, BroadcastReceiver resultReceiver, Handler scheduler, int initialCode, String initialData, Bundle initialExtras, String broadcastPermission) { context.sendBroadcast(intent, broadcastPermission); context.sendBroadcastAsUser(intent, user, broadcastPermission); context.sendOrderedBroadcast(intent, broadcastPermission); context.sendOrderedBroadcastAsUser(intent, user,broadcastPermission, resultReceiver, scheduler, initialCode, initialData, initialExtras); } } ``` ```java Fix theme={null} ```

Android KeyStore is a secure container for storing key materials, in particular it prevents key materials extraction, i.e. when the application process is compromised, the attacker cannot extract keys but may still be able to use them. It’s possible to enable an Android security feature, user authentication, to restrict usage of keys to only authenticated users. The lock screen has to be unlocked with defined credentials (pattern/PIN/password, biometric).

```java Bad theme={null} KeyGenerator keyGenerator = KeyGenerator.getInstance(KeyProperties.KEY_ALGORITHM_AES, "AndroidKeyStore"); KeyGenParameterSpec builder = new KeyGenParameterSpec.Builder("test_secret_key_noncompliant", KeyProperties.PURPOSE_ENCRYPT | KeyProperties.PURPOSE_DECRYPT) // Noncompliant .setBlockModes(KeyProperties.BLOCK_MODE_GCM) .setEncryptionPaddings(KeyProperties.ENCRYPTION_PADDING_NONE) .build(); keyGenerator.init(builder); ``` ```java Fix theme={null} KeyGenerator keyGenerator = KeyGenerator.getInstance(KeyProperties.KEY_ALGORITHM_AES, "AndroidKeyStore"); KeyGenParameterSpec builder = new KeyGenParameterSpec.Builder("test_secret_key", KeyProperties.PURPOSE_ENCRYPT | KeyProperties.PURPOSE_DECRYPT) .setBlockModes(KeyProperties.BLOCK_MODE_GCM) .setEncryptionPaddings(KeyProperties.ENCRYPTION_PADDING_NONE) .setUserAuthenticationRequired(true) .setUserAuthenticationParameters (60, KeyProperties.AUTH_DEVICE_CREDENTIAL) .build(); keyGenerator.init(builder) ```

By default XML processors attempt to load all XML schemas and DTD (their locations are defined with xsi:schemaLocation attributes and DOCTYPE declarations), potentially from an external storage such as file system or network, which may lead, if no restrictions are put in place, to server-side request forgery (SSRF) vulnerabilities.

```java Bad theme={null} DocumentBuilderFactory factory = DocumentBuilderFactory.newInstance(); factory.setValidating(true); // Noncompliant factory.setFeature("http://apache.org/xml/features/nonvalidating/load-external-dtd", true); // Noncompliant SAXParserFactory factory = SAXParserFactory.newInstance(); factory.setValidating(true); // Noncompliant factory.setFeature("http://apache.org/xml/features/nonvalidating/load-external-dtd", true); // Noncompliant SchemaFactory schemaFactory = SchemaFactory.newInstance(XMLConstants.W3C_XML_SCHEMA_NS_URI); schemaFactory.setFeature("http://apache.org/xml/features/nonvalidating/load-external-dtd", true); // Noncompliant ``` ```java Fix theme={null} SAXReader xmlReader = new SAXReader(); // Noncompliant xmlReader.setFeature("http://apache.org/xml/features/nonvalidating/load-external-dtd", true); // Noncompliant ```

Content security policy (CSP) (fetch directives) is a W3C standard which is used by a server to specify, via a http header, the origins from where the browser is allowed to load resources. It can help to mitigate the risk of cross site scripting (XSS) attacks and reduce privileges used by an application. If the website doesn’t define CSP header the browser will apply same-origin policy by default.

Content-Security-Policy: default-src 'self'; script-src ‘self ‘ [http://www.example.com](http://www.example.com)

In the above example, all resources are allowed from the website where this header is set and script resources fetched from example.com are also authorized:

\  \
        \ \
        \