Unfortunately, in Java, there is no such structure and ++Map.Entry++ or ++Object[]++ of fixed size are used as a workaround for returning multiple values from a method.
Get Started
- CodeAnt AI
- Control Center
- Pull Request Review
- IDE
- Compliance
- Anti-Patterns
- Code Governance
- Infrastructure Security Database
- Application Security Database
Java - 1
Learn about Java Anti-Patterns and How they help you write better code, and avoid common pitfalls.
Any extensible class might have subclasses located in a different package. When that happens, the use of `this.getClass().getResource with a relative path would mean that the resource isn’t found for the child class.
Instead, use an absolute path or make the class final`.
Method or constructor references are more readable than lambda expressions in many situations, and may therefore be preferred.
However, method references are sometimes less concise than lambdas. In such cases, it might be preferrable to keep the lambda expression for better readability. Therefore, this rule only raises issues on lambda expressions where the replacement method reference is shorter.
This rule is automatically disabled when the project’s sonar.java.source is lower than 8, as lambda expressions were introduced in Java 8.
The semantics of Thread and Runnable are different, and while it is technically correct to use Thread where a Runnable is expected, it is a bad practice to do so.
The crux of the issue is that Thread is a larger concept than Runnable. A Runnable represents a task. A Thread represents a task and its execution management (ie: how it should behave when started, stopped, resumed, …). It is both a task and a lifecycle management.
There are two types of stream operations: intermediate operations, which return another stream, and terminal operations, which return something other than a stream. Intermediate operations are lazy, meaning they aren’t actually executed until and unless a terminal stream operation is performed on their results. Consequently, if the result of an intermediate stream operation is not fed to a terminal operation, it serves no purpose, which is almost certainly an error.
When directly subclassing `java.io.InputStream or java.io.FilterInputStream, the only requirement is that you implement the method read(). However most uses for such streams don’t read a single byte at a time and the default implementation for read(byte[],int,int) will call read(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 read(byte[],int,int).
This rule raises an issue when a direct subclass of java.io.InputStream or java.io.FilterInputStream doesn’t provide an override of read(byte[],int,int)`.
It’s slightly more efficient to append single characters to StringBuffer and StringBuilder instances as chars, than as Strings. That is, it’s more efficient to put a single char
in single quotes, rather than double quotes.
Early classes of the Java API, such as Vector, Hashtable and StringBuffer, were synchronized to make them thread-safe. However, synchronization has a significant negative impact on performance, even when using these collections from a single thread.
It is often best to use their non-synchronized counterparts:
ArrayList or LinkedList instead of Vector
Deque instead of Stack
HashMap instead of Hashtable
StringBuilder instead of StringBuffer
Even when used in synchronized contexts, you should think twice before using their synchronized counterparts, since their usage can be costly. If you are confident the usage is legitimate, you can safely ignore this warning.
When java.io.File#delete fails, this boolean method simply returns false with no indication of the cause. On the other hand, when java.nio.file.Files#delete fails, this void method returns one of a series of exception types to better indicate the cause of the failure. And since more information is generally better in a debugging situation, java.nio.file.Files#delete
is the preferred option.
Describing, setting error message or adding a comparator in AssertJ must be done before calling the assertion, otherwise, settings will not be taken into account.
This rule raises an issue when one of the method (with all similar methods):
`as
describedAs
withFailMessage
overridingErrorMessage
usingComparator
usingElementComparator
extracting
filteredOn`
is called without calling an AssertJ assertion afterward.
There is potential for confusion if an octal or hexadecimal escape sequence is immediately followed by other characters. Instead, such sequences should be terminated by either:
The start of another escape sequence.
The end of the string.
Placing the array designators [] after the type helps maintain backward compatibility with older versions of the Java SE platform. This syntax contributes to better readability as it becomes easier to distinguish between array types and non-array types. It helps convey the intention of the method to both the developer implementing it and the developer using it.
Clear and communicative error messages help people understand what went wrong and how to correct the problem. However, care must be taken with `Servlet error messages because they could expose sensitive information to an attacker. Even sending the user’s own data back to him in an error message could be risky; you never know who might catch a glimpse of the screen.
This rule checks that the strings used in servlet responses made from catch blocks don’t change from call to call. Ideally, such strings would be private static final`, but that is not enforced by this rule. Logging messages are ignored by this rule.
Regular expressions are powerful but tricky, and even those long used to using them can make mistakes.
The following should not be used as regular expressions:
`. - matches any single character. Used in replaceAll, it matches everything
| - normally used as an option delimiter. Used stand-alone, it matches the space between characters
File.separator` - matches the platform-specific file path delimiter. On Windows, this will be taken as an escape character
Spring `@Controller, @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 and supplied via constructor injection rather than by field injection.
This rule raise an issue when any non-static` member of a Spring component has an injection annotation.
`Object.finalize() is called by the Garbage Collector at some point after the object becomes unreferenced.
In general, overloading Object.finalize() is a bad idea because:
The overload may not be called by the Garbage Collector.
Users are not expected to call Object.finalize() and will get confused.
But beyond that it’s a terrible idea to name a method “finalize” if it doesn’t actually override Object.finalize()`.
The ThreadGroup class contains many deprecated methods like allowThreadSuspension, resume, stop, and suspend. Also, some of the non-deprecated methods are obsolete or not thread-safe, and still others are insecure (activeCount, enumerate). For these reasons, any use of ThreadGroup is suspicious and should be avoided.
The default implementation of java.lang.Thread ‘s run will only perform a task passed as a Runnable. If no Runnable has been provided at construction time, then the thread will not perform any action.
When extending java.lang.Thread, you should override the run method or pass a Runnable target to the constructor of java.lang.Thread.
Perhaps counter-intuitively, a compareTo method is expected to throw a NullPointerException if passed a null argument, and a ClassCastException
if the argument is of the wrong type. So there’s no need to null-test or type-test the argument.
Looking for a given substring starting from a specified offset can be achieved by such code: `str.substring(beginIndex).indexOf(char1). This works well, but it creates a new String for each call to the substring method. When this is done in a loop, a lot of Strings are created for nothing, which can lead to performance problems if str is large.
To avoid performance problems, String.substring(beginIndex) should not be chained with the following methods:
indexOf(int ch)
indexOf(String str)
lastIndexOf(int ch)
lastIndexOf(String str)
startsWith(String prefix)
For each of these methods, another method with an additional parameter is available to specify an offset.
Using these methods will avoid the creation of additional String` instances. For indexOf methods, adjust the returned value by subtracting the substring index parameter to obtain the same result.
Stream operations are divided into intermediate and terminal operations, and are combined to form stream pipelines. After the terminal operation is performed, the stream pipeline is considered consumed, and cannot be used again. Such a reuse will yield unexpected results.
Adding messages to JUnit, FEST and AssertJ assertions is an investment in your future productivity. Spend a few seconds writing them now, and you’ll save a lot of time on the other end when either the tests fail and you need to quickly diagnose the problem, or when you need to maintain the tests and the assertion messages work as a sort of documentation.
`@ComponentScan is used to find which Spring @Component beans (@Service or @Repository or Controller) are available in the classpath so they can be used in the application context. This is a convenient feature especially when you begin a new project but it comes with the drawback of slowing down the application start-up time especially when the application becomes bigger (ie: it references a large JAR file, or it references a significant number of JAR files, or the base-package refers to a large amount of .class files).
@ComponentScan should be replaced by an explicit list of Spring beans loaded by @Import.
The interface @SpringBootApplication is also considered by this rule because it is annotated with @ComponentScan`.
When you need to perform a complicated initialization of a static member, it should be done in a static initializer block. That’s because such blocks are only executed when the class is loaded into the JVM. That is, they run only once, and that happens before any instances are created. Non-static blocks, on the other hand, run once for each instance that’s created, so any static
members “initialized” in such a block will be re-set for each new instance.
In Java 15 Text Blocks are now official and can be used. The most common pattern for multiline strings in Java < 15 was to write String concatenation. Now it’s possible to do it in a more natural way using Text Blocks.
When testing exception via org.junit.rules.ExpectedException
any code after the raised exception will not be executed, so adding subsequent assertions is wrong and misleading. This rule raises an issue when an assertion is done after the “expect(…)” invocation, only the code throwing the expected exception should be after “expect(…)”.
You should consider using org.junit.Assert.assertThrows instead, it’s available since JUnit 4.13 and it allows additional subsequent assertions.
Alternatively, you could use try-catch idiom for JUnit version < 4.13 or if your project does not support lambdas.
Spring provides two options to mark a REST parameter as optional:
Use required = false in the @PathVariable or @RequestParam annotation of the respective method parameter or
Use type java.util.Optional<T> for the method parameter
When using 1., the absence of the parameter, when the REST function is called, is encoded by null, which can only be used for object types. If required = false is used for a parameter with a primitive and the REST function is called without the parameter, a runtime exception occurs because the Spring data mapper cannot map the null value to the parameter.
Assertion methods are throwing a “java.lang.AssertionError
”. If this call is done within the try block of a try-catch cathing a similar error, you should make sure to test some properties of the exception. Otherwise, the assertion will never fail.
If the credentials provider is not specified when creating a new AwsClient with an AwsClientBuilder, the AWS SDK will execute some logic to identify it automatically.
While it will probably identify the correct one, this extra logic will slow down startup time, already known to be a hotspot.
You should therefore always define the logic to set the credentials provider yourself. This is typically done by retrieving it from the Lambda provided environment variable.
This will make the code more explicit and spare initialization time.
This rule reports an issue when the credentials provider is not set when creating an AwsClient.
Standard applications don’t require a display refresh rate above 60Hz, hence it is advisable to avoid higher frequencies to avoid unnecessary energy consumption.
The rule flags an issue when setFrameRate() is invoked with a frameRate higher than 60Hz for android.view.Surface and android.view.SurfaceControl.Transaction.
It’s important to note that the scheduler considers several factors when determining the display refresh rate. Therefore, using setFrameRate() doesn’t guarantee your app will achieve the requested frame rate.
Once set, the value of a Hibernate @Entity’s @Id field/column should never be updated. Therefore, setters for such fields should always be private
.
According to the API documentation of the HttpServletRequest.getRequestedSessionId() method:
The session ID it returns is either transmitted through a cookie or a URL parameter. This allows an end user to manually update the value of this session ID in an HTTP request.
Due to the ability of the end-user to manually change the value, the session ID in the request should only be used by a servlet container (e.g. Tomcat or Jetty) to see if the value matches the ID of an existing session. If it does not, the user should be considered unauthenticated.
The use of a “RESOURCE_LOCAL” persistence-unit
makes you responsible for your own entity management, which involves a lot of extra boilerplate code to get right. Instead, set this to “JPA” in a JavaSE environment or omit it altogether in a JavaEE environment, where “JPA” is the default.
The PreparedStatement is frequently used in loops because it allows to conveniently set parameters. A small optimization is possible by setting constant parameters outside the loop or hard-coding them in the query whenever possible.
Operations performed on a string with predictable outcomes should be avoided. For example:
checking if a string contains itself
comparing a string with itself
matching a string against itself
creating a substring from 0 to the end of the string
creating a substring from the end of the string
replacing a string with itself
replacing a substring with the exact substring
The fields in an HTTP request are putty in the hands of an attacker, and you cannot rely on them to tell you the truth about anything. While it may be safe to store such values after they have been neutralized, decisions should never be made based on their contents.
This rule flags uses of the referer header field.
In java 7 to 9, FileInputStream and FileOutputStream rely on finalization to perform final closes if the stream is not already closed. Whether or not the stream is already closed, the finalizer will be called, resulting in extra work for the garbage collector. This can easily be avoided using the Files
API.
Two classes can have the same simple name if they are in two different packages.
The Comparable.compareTo method returns a negative integer, zero, or a positive integer to indicate whether the object is less than, equal to, or greater than the parameter. The sign of the return value or whether it is zero is what matters, not its magnitude.
Returning a positive or negative constant value other than the basic ones (-1, 0, or 1) provides no additional information to the caller. Moreover, it could potentially confuse code readers who are trying to understand its purpose.
There is no good reason to declare a field “public” and “static” without also declaring it “final”. Most of the time this is a kludge to share a state among several objects. But with this approach, any object can do whatever it wants with the shared state, such as setting it to null
.
A try-catch block is used to handle exceptions or errors that may occur during the execution of a block of code. It allows you to catch and handle exceptions gracefully, preventing your program from terminating abruptly.
The code that may throw an exception is enclosed within the try block, while each catch block specifies the type of exception it can handle. The corresponding catch block is executed if the exception matches the type specified in any catch block. It is unnecessary to manually check the types using instanceof because Java automatically matches the exception type to the appropriate catch block based on the declared exception type in the catch clauses.
For optimal code readability, annotation arguments should be specified in the same order that they were declared in the annotation definition.
Assertions comparing incompatible types always fail, and negative assertions always pass. At best, negative assertions are useless. At worst, the developer loses time trying to fix his code logic before noticing wrong assertions.
Dissimilar types are:
comparing a primitive with null
comparing an object with an unrelated primitive (E.G. a string with an int)
comparing unrelated classes
comparing an array to a non-array
comparing two arrays of dissimilar types
This rule also raises issues for unrelated class and interface or unrelated interface
types in negative assertions. Because except in some corner cases, those types are more likely to be dissimilar. And inside a negative assertion, there is no test failure to inform the developer about this unusual comparison.
Supported test frameworks:
JUnit4
JUnit5
AssertJ
The location awareness feature can significantly drain the device’s battery.
The recommended way to maximize the battery life is to use the fused location provider which combines signals from GPS, Wi-Fi, and cell networks, as well as accelerometer, gyroscope, magnetometer and other sensors. The FusedLocationProviderClient automatically chooses the best method to retrieve a device’s location based on the device’s context.
The rule flags an issue when android.location.LocationManager or com.google.android.gms.location.LocationClient is used instead of com.google.android.gms.location.FusedLocationProviderClient.
Creating temporary primitive wrapper objects only for String conversion or the use of the compareTo() method is inefficient.
Instead, the static toString() or compare() method of the primitive wrapper class should be used.
Referencing a static member of a subclass from its parent during class initialization, makes the code more fragile and prone to future bugs. The execution of the program will rely heavily on the order of initialization of classes and their static members.
An indexOf or lastIndexOf call with a single letter String can be made more performant by switching to a call with a char
argument.
This rule allows you to track the use of the PMD suppression comment mechanism.
s keys will be valid until you manually revoke them. This makes them highly sensitive as any exposure can have serious consequences and should be used with care.
This rule will trigger when encountering an instantiation of com.amazonaws.auth.BasicAWSCredentials.
Using boxed type suggests that null
is a possible value for the variable. Use of the primitive type should be preferred if this is not the case to avoid any confusion about possible values variable can contain.
Appending String.valueOf() to a String decreases the code readability.
The argument passed to String.valueOf() should be directly appended instead.
Programming languages evolve over time, and new versions of Java introduce additional keywords. If future keywords are used in the current code, it can create compatibility issues when transitioning to newer versions of Java. The code may fail to compile or behave unexpectedly due to conflicts with newly introduced keywords.
The following keywords are marked as invalid identifiers:
Keyword | Added in version |
---|---|
_ | 9 |
enum | 5.0 |
assert and strictfp are another example of valid identifiers which became keywords in later versions, but are not supported by this rule.
Boxing is the process of putting a primitive value into a wrapper object, such as creating an Integer to hold an int value. Unboxing is the process of retrieving the primitive value from such an object. Since the original value is unchanged during boxing and unboxing, there is no point in doing either when not needed.
Instead, you should rely on Java’s implicit boxing/unboxing to convert from the primitive type to the wrapper type and vice versa, for better readability.
As stated per effective java :
According to the Java Language Specification, there is a contract between `equals(Object) and hashCode():
In order to comply with this contract, those methods should be either both inherited, or both overridden.
Each constructor must first invoke a parent class constructor, but it doesn’t always have to be done explicitly. If the parent class has a reachable, no-args constructor, a call to it will be inserted automatically by the compiler. Thus, calls to super()
can be omitted.
The `setUp() and tearDown() methods (initially introduced with JUnit3 to execute a block of code before and after each test) need to be correctly annotated with the equivalent annotation in order to preserve the same behavior when migrating from JUnit3 to JUnit4 or JUnit5.
This rule consequently raise issues on setUp() and tearDown()` methods which are not annotated in test classes.
A cleanly coded web application will have a clear separation of concerns, with business logic in the `@Service layer, and communication with other systems in the data access layer.
To help enforce such a separation of concerns, this rule raises an issue when a @Service class has RestTemplate, JmsTemplate, WebServiceTemplate, JdbcTemplate, or DataSource` member.
The equals and hashCode methods of java.net.URL may trigger a name service lookup (typically DNS) to resolve the hostname or IP address. Depending on the configuration, and network status, this lookup can be time-consuming.
On the other hand, the URI class does not perform such lookups and is a better choice unless you specifically require the functionality provided by URL.
In general, it is better to use the URI class until access to the resource is actually needed, at which point you can convert the URI to a URL using URI.toURL().
This rule checks for uses of URL ‘s in Map and Set , and for explicit calls to the equals and hashCode methods. It suggests reconsidering the use of URL in such scenarios to avoid potential performance issues related to name service lookups.
It is preferable to place string literals on the left-hand side of an equals() or equalsIgnoreCase()
method call.
This prevents null pointer exceptions from being raised, as a string literal can never be null by definition.
It is convention to name each class’s logger for the class itself. Doing so allows you to set up clear, communicative logger configuration. Naming loggers by some other convention confuses configuration, and using the same class name for multiple class loggers prevents the granular configuration of each class’ logger. Some libraries, such as SLF4J warn about this, but not all do.
This rule raises an issue when a logger is not named for its enclosing class.
If an InterruptedException or a ThreadDeath error is not handled properly, the information that the thread was interrupted will be lost. Handling this exception means either to re-throw it or manually re-interrupt the current thread by calling Thread.interrupt(). Simply logging the exception is not sufficient and counts as ignoring it. Between the moment the exception is caught and handled, is the right time to perform cleanup operations on the method’s state, if needed.
Using a `type=“timestamp” column as the primary key of a table is slightly risky. Two threads could create new objects in the table close enough in sequence for them to both have the same timestamp. Alternately, this could happen during a daylight savings time change. Instead, use a numeric value as the @Id.
This rule raises an issue when a time or date-related class is annotated with @Id`.
The class `java.util.zip.GZIPInputStream is already buffering its input while reading. Thus passing a java.io.BufferedInputStream to a java.util.zip.GZIPInputStream is redundant. It is more efficient to directly pass the original input stream to java.util.zip.GZIPInputStream.
Note that the default buffer size of GZIPInputStream is not the same as the one in BufferedInputStream. Configure it if need be.
This rule raises an issue when a java.util.zip.GZIPInputStream reads from a java.io.BufferedInputStream`.
The JDK provides a set of built-in methods to copy the contents of an array into another array. Using a loop to perform the same operation is less clear, more verbose and should be avoided.
There are several reasons to avoid using this method:
It is optionally available only for result sets of type ResultSet.TYPE_FORWARD_ONLY. Database drivers will throw an exception if not supported.
The method can be expensive to execute as the database driver may need to fetch ahead one row to determine whether the current row is the last in the result set. The documentation of the method explicitly mentions this fact.
What “the cursor is on the last row” means for an empty ResultSet is unclear. Database drivers may return true or false in this case .
ResultSet.next() is a good alternative to ResultSet.isLast() as it does not have the mentioned issues. It is always supported and, as per specification, returns false for empty result sets.
A return type containing wildcards cannot be narrowed down in any context. This indicates that the developer’s intention was likely something else.
The core problem lies in type variance. Expressions at an input position, such as arguments passed to a method, can have a more specific type than the type expected by the method, which is called covariance. Expressions at an output position, such as a variable that receives the return result from a method, can have a more general type than the method’s return type, which is called contravariance. This can be traced back to the Liskov substitution principle.
In Java, type parameters of a generic type are invariant by default due to their potential occurrence in both input and output positions at the same time. A classic example of this is the methods T get() (output position) and add(T element) (input position) in interface java.util.List. We could construct cases with invalid typing in List if T were not invariant.
Wildcards can be employed to achieve covariance or contravariance in situations where the type parameter appears in one position only:
<? extends Foo> for covariance (input positions)
<? super Foo> for contravariance (output positions)
However, covariance is ineffective for the return type of a method since it is not an input position. Making it contravariant also has no effect since it is the receiver of the return value which must be contravariant (use-site variance in Java). Consequently, a return type containing wildcards is generally a mistake.
It’s almost always a mistake to compare two instances of java.lang.String or boxed types like java.lang.Integer using reference equality == or !=
, because it is not comparing actual value but locations in memory.
When verifying that code raises an exception, a good practice is to avoid having multiple method calls inside the tested code, to be explicit about what is exactly tested.
When two of the methods can raise the same checked exception, not respecting this good practice is a bug, since it is not possible to know what is really tested.
You should make sure that only one method can raise the expected checked exception in the tested code.
Map is an object that maps keys to values. A map cannot contain duplicate keys, which means each key can map to at most one value.
When both the key and the value are needed, it is more efficient to iterate the entrySet(), which will give access to both instead of iterating over the keySet() and then getting the value.
If the entrySet() method is not iterated when both the key and value are needed, it can lead to unnecessary lookups. This is because each lookup requires two operations: one to retrieve the key and another to retrieve the value. By iterating the entrySet() method, the key-value pair can be retrieved in a single operation, which can improve performance.
The use of exact alarms triggers the device to wake up at precise times that can lead several wake-ups in a short period of time. The wake-up mechanism is a significant battery drain because it requires powering up the main processor and pulling it out of a low-power state.
It’s highly recommended to create an inexact alarm whenever possible.
It is also recommended for normal timing operations, such as ticks and timeouts, using the Handler, and for long-running operations, such as network downloads, using WorkManager or JobScheduler.
`Bean Validation as per defined by JSR 380 can be triggered programmatically or also executed by the Bean Validation providers. However something should tell the Bean Validation provider that a variable must be validated otherwise no validation will happen. This can be achieved by annotating a variable with javax.validation.Valid and unfortunally it’s easy to forget to add this annotation on complex Beans.
Not annotating a variable with @Valid means Bean Validation will not be triggered for this variable, but readers may overlook this omission and assume the variable will be validated.
This rule will run by default on all Class’es and therefore can generate a lot of noise. This rule should be restricted to run only on certain layers. For this reason, the “Restrict Scope of Coding Rules” feature should be used to check for missing @Valid` annotations only on some packages of the application.
Using FetchType.EAGER can lead to inefficient data loading and potential performance issues. Eager Loading initializes associated data on the spot, potentially fetching more data than needed.
Synchronizing on a class field synchronizes not on the field itself, but on the object assigned to it. So synchronizing on a non-final
field makes it possible for the field’s value to change while a thread is in a block synchronized on the old value. That would allow a second thread, synchronized on the new value, to enter the block at the same time.
The story is very similar for synchronizing on parameters; two different threads running the method in parallel could pass two different object instances in to the method as parameters, completely undermining the synchronization.
According to the JDBC specification:
Without OAEP in RSA encryption, it takes less work for an attacker to decrypt the data or infer patterns from the ciphertext. This rule logs an issue as soon as a literal value starts with RSA/NONE
.
Java 21 introduces a new SequencedCollection interface that provides a uniform API for accessing its first and last elements. The new getFirst() and getLast() methods offer a consistent way to access elements across SortedSet, NavigableSet, LinkedHashSet, List and Deque collections. Because those methods are more concise and readable, they should be used instead of more complex workarounds that recreate the same behavior.
For example, list.get(list.size() - 1) can be replaced by list.getLast().
This rule identifies code that can be simplified by using the new getFirst() and getLast() methods.
Proper synchronization and thread management can be tricky under the best of circumstances, but it’s particularly difficult in JEE application, and is even forbidden under some circumstances by the JEE standard.
This rule raises an issue for each Runnable, and use of the synchronized
keyword.
NullPointerException should be avoided, not caught. Any situation in which NullPointerException is explicitly caught can easily be converted to a null
test, and any behavior being carried out in the catch block can easily be moved to the “is null” branch of the conditional.
Testing equality of an enum value with `equals is perfectly valid because an enum is an Object and every Java developer knows ”==” should not be used to compare the content of an Object. At the same time, using ”==” on enums:
provides the same expected comparison (content) as equals
is more null-safe than equals()
provides compile-time (static) checking rather than runtime checking
For these reasons, use of ”==” should be preferred to equals`.
The difference between `private and protected visibility is that child classes can see and use protected members, but they cannot see private ones. Since a final class will have no children, marking the members of a final class protected is confusingly pointless.
Note that the protected` members of a class can also be seen and used by other classes that are placed within the same package, this could lead to accidental, unintended access to otherwise private members.
Shared coding conventions allow teams to collaborate effectively. While types for lambda arguments are optional, specifying them anyway makes the code clearer and easier to read.
An ObjectOutputStream writes primitive data types and graphs of Java objects to an OutputStream. The objects can be read (reconstituted) using an ObjectInputStream.
When ObjectOutputStream is used with files opened in append mode, it can cause data corruption and unexpected behavior. This is because when ObjectOutputStream is created, it writes metadata to the output stream, which can conflict with the existing metadata when the file is opened in append mode. This can lead to errors and data loss.
When used with serialization, an ObjectOutputStream first writes the serialization stream header. This header should appear once per file at the beginning. When you’re trying to read your object(s) back from the file, only the first one will be read successfully, and a StreamCorruptedException will be thrown after that.
There is no good reason to have a mutable object as the `public (by default), static member of an interface. Such variables should be moved into classes and their visibility lowered.
Similarly, mutable static members of classes and enumerations which are accessed directly, rather than through getters and setters, should be protected to the degree possible. That can be done by reducing visibility or making the field final if appropriate.
Note that making a mutable field, such as an array, final will keep the variable from being reassigned, but doing so has no effect on the mutability of the internal state of the array (i.e. it doesn’t accomplish the goal).
This rule raises issues for public static array, Collection, Date, and awt.Point` members.
The order in which you close database-releated resources is crucial. Close a Connection first, and depending on the database pooling in use, you may no longer be able to truly reach its Statements and ResultSet
s to close them, even though the calls are made and execute without error.
When an SWT `Image accesses a file directly, it holds the file handle for the life of the image. Do this many times, and the OS may run out of available file handles. At minimum, SWT Images which directly access files should not be static. At best, they should access their files through ImageDescriptors, which do not hold open file handles.
This rule looks for org.eclipse.swt.graphics.Images which both directly access a file on the file path and are static`.
ialization of objects from LDAP directories, which can lead to remote code execution.
This rule raises an issue when an LDAP search query is executed with SearchControls
configured to allow deserialization.
Transactional methods have a propagation type parameter in the @Transaction annotation that specifies the requirements about the transactional context in which the method can be called and how it creates, appends, or suspends an ongoing transaction.
When an instance that contains transactional methods is injected, Spring uses proxy objects to wrap these methods with the actual transaction code.
However, if a transactional method is called from another method in the same class, the this argument is used as the receiver instance instead of the injected proxy object, which bypasses the wrapper code. This results in specific transitions from one transactional method to another, which are not allowed:
From | To |
---|---|
non-`@Transactional | MANDATORY, NESTED, REQUIRED, REQUIRES_NEW |
MANDATORY | NESTED, NEVER, NOT_SUPPORTED, REQUIRES_NEW |
NESTED | NESTED, NEVER, NOT_SUPPORTED, REQUIRES_NEW |
NEVER | MANDATORY, NESTED, REQUIRED, REQUIRES_NEW |
NOT_SUPPORTED | MANDATORY, NESTED, REQUIRED, REQUIRES_NEW |
REQUIRED or @Transactional` | NESTED, NEVER, NOT_SUPPORTED, REQUIRES_NEW |
REQUIRES_NEW | NESTED, NEVER, NOT_SUPPORTED, REQUIRES_NEW |
SUPPORTS | MANDATORY, NESTED, NEVER, NOT_SUPPORTED, REQUIRED, REQUIRES_NEW |
The equals method in AtomicInteger and AtomicLong returns true only if two instances are identical, not if they represent the same number value.
This is because equals is not part of the API contract of these classes, and they do not override the method inherited from java.lang.Object. Although both classes implement the Number interface, assertions about equals comparing number values are not part of that interface either. Only the API contract of implementing classes like Integer, Long, Float, BigInteger, etc., provides such assertions.
A method with a `@RequestMapping annotation part of a class annotated with @Controller (directly or indirectly through a meta annotation - @RestController from Spring Boot is a good example) will be called to handle matching web requests. That will happen even if the method is private, because Spring invokes such methods via reflection, without checking visibility.
So marking a sensitive method private may seem like a good way to control how such code is called. Unfortunately, not all Spring frameworks ignore visibility in this way. For instance, if you’ve tried to control web access to your sensitive, private, @RequestMapping method by marking it @Secured … it will still be called, whether or not the user is authorized to access it. That’s because AOP proxies are not applied to private methods.
In addition to @RequestMapping, this rule also considers the annotations introduced in Spring Framework 4.3: @GetMapping, @PostMapping, @PutMapping, @DeleteMapping, @PatchMapping`.
Java 21 enhances Pattern Matching, introduced in Java 16, with a record pattern that decomposes records into local variables. This form should be used when all fields of a record are accessed within a block for improved readability. Nested record patterns are also allowed and should be used when a record field is another record, and all its fields are accessed.
A loop with at most one iteration is equivalent to an if statement. This can confuse developers and make the code less readable since loops are not meant to replace if statements.
If the intention was to conditionally execute the block only once, an if statement should be used instead. Otherwise, the loop should be fixed so the loop block can be executed multiple times.
A loop statement with at most one iteration can happen when a statement that unconditionally transfers control, such as a jump or throw statement, is misplaced inside the loop block.
This rule arises when the following statements are misplaced:
break
return
throw
With Java 8, there’s no need to write `Comparators that compare primitive values or other Comparables; they can be generated for you using the Comparator.comparing* functions: comparing, comparingDouble, comparingInt, comparingLong.
Note that this rule is automatically disabled when the project’s sonar.java.source is lower than 8`.
Monster Classes become monolithic entities, with numerous responsibilities and functionalities packed into a single class. This is problematic because it violates the Single Responsibility Principle, which states that a class should have only one reason to change.
When a class has too many responsibilities and functionalities, it becomes difficult to maintain. Changes to one part of the class can unintentionally affect other parts, leading to bugs. Additionally, it can be difficult to test the class, as there may be many different interactions between different parts of the class that need to be considered.
Needing to cast from an interface to a concrete type indicates that something is wrong with the abstractions in use, likely that something is missing from the interface. Instead of casting to a discrete type, the missing functionality should be added to the interface
. Otherwise there is the risk of runtime exceptions.
Optimizing resource usage and preventing unnecessary battery drain are critical considerations in Android development. Failing to release sensor resources when they are no longer needed can lead to prolonged device activity, negatively impacting battery life. Common Android sensors, such as cameras, GPS, and microphones, provide a method to release resources after they are not in use anymore.
This rule identifies situations where a sensor is not released after being utilized, helping developers maintain efficient and battery-friendly applications.
Missing call to release() method:
android.os.PowerManager.WakeLock
android.net.wifi.WifiManager$MulticastLock
android.hardware.Camera
android.media.MediaPlayer
android.media.MediaRecorder
android.media.SoundPool
android.media.audiofx.Visualizer
android.hardware.display.VirtualDisplay
Missing call to close() method
android.hardware.camera2.CameraDevice
Missing call to removeUpdates() method:
android.location.LocationManager
Missing call to unregisterListener() method:
android.hardware.SensorManager
enums are generally thought of as constant, but an enum with a public field or public setter is non-constant. Ideally fields in an enum are private
and set in the constructor, but if that’s not possible, their visibility should be reduced as much as possible.
When all the elements in a Set are values from the same enum, the Set can be replaced with an EnumSet, which can be much more efficient than other sets because the underlying data structure is a simple bitmap.
Returning null
when something goes wrong instead of throwing an exception leaves callers with no understanding of what went wrong. Instead, an exception should be thrown.
The JEE standard forbids the direct management of connections in JEE applications. The application code should not directly create, manage, or close database connections. Instead, the application code should use the connection pool managed by the container via DataSource objects.
The container is responsible for creating and managing the connection pool, as well as monitoring the usage of connections and releasing them when they are no longer needed. By delegating connection management to the container, JEE applications can avoid connection leaks and resource exhaustion and ensure that database connections are used efficiently and securely.
When an application manages connections directly, connection leaks may arise. These leaks occur when an application fails to release a database connection after it has finished using it. Another risk is vulnerability to SQL injection attacks, which occur when an attacker is able to inject malicious SQL code into an application’s database queries, allowing them to access or modify sensitive data. Finally, these applications have difficulty managing and monitoring database connections. Without a centralized connection pool, tracking the usage of database connections and ensuring they are used efficiently and securely can be challenging.
This rule raises an issue for using a DriverManager in a servlet class.
`sealed classes were introduced in Java 17. This feature is very useful if there is a need to define a strict hierarchy and restrict the possibility of extending classes. In order to mention all the allowed subclasses, there is a keyword permits, which should be followed by subclasses’ names.
This notation is quite useful if subclasses of a given sealed class can be found in different files, packages, or even modules. In case when all subclasses are declared in the same file there is no need to mention the explicitly and permits part of a declaration can be omitted.
This rule reports an issue if all subclasses of a sealed` class are declared in the same file as their superclass.
Mockito provides argument matchers for flexibly stubbing or verifying method calls.
`Mockito.verify(), Mockito.when(), Stubber.when() and BDDMockito.given() each have overloads with and without argument matchers.
However, the default matching behavior (i.e. without argument matchers) uses equals(). If only the matcher org.mockito.ArgumentMatchers.eq() is used, the call is equivalent to the call without matchers, i.e. the eq()` is not necessary and can be omitted. The resulting code is shorter and easier to read.
Marking a non-public method @Async or @Transactional is misleading because Spring does not recognize non-public methods, and so makes no provision for their proper invocation. Nor does Spring make provision for the methods invoked by the method it called.
Therefore marking a private method, for instance, @Transactional can only result in a runtime error or exception if the method is annotated as @Transactional.
Under the reasoning that cleaner code is better code, the semicolon at the end of a try-with-resources construct should be omitted because it can be omitted.
The java.util.concurrent.locks.Condition interface provides an alternative to the Object monitor methods (wait, notify and notifyAll). Hence, the purpose of implementing said interface is to gain access to its more nuanced await methods.
Consequently, calling the method Object.wait on a class implementing the Condition interface is contradictory and should be avoided. Use Condition.await instead.
The Java Language Specification recommends listing modifiers in the following order:
Annotations
public
protected
private
abstract
static
final
transient
volatile
synchronized
native
default
strictfp
Not following this convention has no technical impact, but will reduce the code’s readability because most developers are used to the standard order.
Assembling a StringBuilder or StringBuffer into a String merely to see if it’s empty is a waste of cycles. Instead, jump right to the heart of the matter and get its .length()
instead.
An exception in a `throws declaration in Java is superfluous if it is:
listed multiple times
a subclass of another listed exception
a RuntimeException`, or one of its descendants
completely unnecessary because the declared exception type cannot actually be thrown
When a developer uses the StringBuilder or StringBuffer constructor with a single character as an argument, the likely intention is to create an instance with the character as the initial string value.
However, this is not what happens because of the absence of a dedicated StringBuilder(char) or StringBuffer(char) constructor. Instead, StringBuilder(int) or StringBuffer(int) is invoked, which results in an instance with the provided int value as the initial capacity of the StringBuilder or StringBuffer.
The reason behind this behavior lies in the automatic widening of char expressions to int when required. Consequently, the UTF-16 code point value of the character (for example, 65 for the character ‘A’) is interpreted as an int to specify the initial capacity.
The @PathVariable annotation in Spring extracts values from the URI path and binds them to method parameters in a Spring MVC controller. It is commonly used with @GetMapping, @PostMapping, @PutMapping, and @DeleteMapping to capture path variables from the URI. These annotations map HTTP requests to specific handler methods in a controller. They are part of the Spring Web module and are commonly used to define the routes for different HTTP operations in a RESTful API.
If a method has a path template containing a placeholder, like “/api/resource/{id}”, and there’s no @PathVariable annotation on a method parameter to capture the id path variable, Spring will disregard the id variable.
When creating an instance of HashMap or HashSet, the developer can pick a constructor with known capacity. However, the requested capacity is not fully allocated by default. Indeed, when the collection reaches the load factor of the collection (default: 0.75), the collection is resized on the fly, leading to unexpected performance issues.
Sometimes when implementing a method, there is a need to return more than one value. To reduce the boilerplate of describing another class, other programming languages introduced such structures as `Pair, Tuple, Vector, etc.
Java 16 introduced records to represent immutable data structures and they can be used for grouping different values in one entity. By using records, developers will have meaningful names and result in a more readable code. Furthermore, when using Object[], there is a risk of getting ClassCastException or ArrayIndexOutOfBoundsException if not used carefully. It means that using records is not only more readable but is definitely safer.
This rule should report an issue when Object[] of a fixed size (< 7) or Map.Entry` are returned from a private method.
The Object.clone / java.lang.Cloneable mechanism in Java should be considered broken for the following reasons and should, consequently, not be used:
Cloneable is a marker interface without API but with a contract about class behavior that the compiler cannot enforce. This is a bad practice.
Classes are instantiated without calling their constructor, so possible preconditions cannot be enforced.
There are implementation flaws by design when overriding Object.clone, like type casts or the handling of CloneNotSupportedException exceptions.
`PreparedStatements and CallableStatements (for stored procedures) are safer and more efficient than Statements and should always be preferred.
This rule raises an issue each time a Statement` is declared.
The Java Collections API offers a well-structured hierarchy of interfaces designed to hide collection implementation details. For the various collection data structures like lists, sets, and maps, specific interfaces (java.util.List, java.util.Set, java.util.Map) cover the essential features.
When passing collections as method parameters, return values, or when exposing fields, it is generally recommended to use these interfaces instead of the implementing classes. The implementing classes, such as java.util.LinkedList, java.util.ArrayList, and java.util.HasMap, should only be used for collection instantiation. They provide finer control over the performance characteristics of those structures, and developers choose them depending on their use case.
For example, if fast random element access is essential, java.util.ArrayList should be instantiated. If inserting elements at a random position into a list is crucial, a java.util.LinkedList should be preferred. However, this is an implementation detail your API should not expose.
There are many ways to implement the Singleton pattern in Java, but none of them is as clean, compact and close to fool-proof as using an enum. Without an enum, the implementer must take care to properly handle thread-safety, serialization, and classloaders, but those things come for free with an enum
.
According to the EJB specification:
Since EJB’s may be passivated (temporarily serialized at the discretion of the container), using sockets in an EJB could cause resource leaks. Instead, you should work at a higher level and let the container handle such resources.
This rule raises an issue each time a socket is created or or retrieved from another class in a servlet class or EJB.
This rule raises an issue when required properties are not included in a project’s pom.
Java 21 has introduced enhancements to switch statements and expressions, allowing them to operate on any type, not just specific ones, as in previous versions. Furthermore, case labels have been upgraded to support patterns, providing an alternative to the previous restriction of only accepting constants.
It is equivalent to use the equality `== operator and the equals method to compare two objects if the equals method inherited from Object has not been overridden. In this case both checks compare the object references.
But as soon as equals is overridden, two objects not having the same reference but having the same value can be equal. This rule spots suspicious uses of == and != operators on objects whose equals` methods are overridden.
The creation of a JAXBContext.newInstance is a costly operation, and should only be performed once per context and stored - preferably in a static
member - for reuse.
In fact, according to the JAXB 2.2 Specification:
This rule raises an issue when multiple instances are created for the same context path.
A common good practice is to write test methods targeting only one logical concept, that can only fail for one reason.
While it might make sense to have more than one assertion to test one concept, having too many is a sign that a test became too complex and should be refactored to multiples ones.
This rule will report any test method containing more than a given number of assertion.
A non-static inner class has a reference to its outer class, and access to the outer class’ fields and methods. That class reference makes the inner class larger and could cause the outer class instance to live in memory longer than necessary.
If the reference to the outer class isn’t used, it is more efficient to make the inner class `static (also called nested). If the reference is used only in the class constructor, then explicitly pass a class reference to the constructor. If the inner class is anonymous, it will also be necessary to name it.
However, while a nested/static class would be more efficient, it’s worth noting 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.
Cloneable is a marker interface that defines the contract of the Object.clone method, which is to create a consistent copy of the instance. The clone method is not defined by the interface though, but by class Objects.
The general problem with marker interfaces is that their definitions cannot be enforced by the compiler because they have no own API. When a class implements Cloneable but does not override Object.clone, it is highly likely that it violates the contract for Cloneable.
Annotating unit tests with more than one test-related annotation is not only useless but could also result in unexpected behavior like failing tests or unwanted side-effects.
This rule reports an issue when a test method is annotated with more than one of the following competing annotation:
@Test
@RepeatedTest
@ParameterizedTest
@TestFactory
@TestTemplate
Java 8 adds `Comparator.comparing to allow the creation of a single-value comparator to be shorthanded into a single call. This cleaner syntax should be preferred.
Note that this rule is automatically disabled when the project’s sonar.java.source is lower than 8`.
Including any logic other than a simple return of the field in a persistence-annotated method can result in odd behavior, including for example, the default construction of empty members which are annotated to be lazy
.
Shared naming conventions allow teams to collaborate efficiently. This rule raises an issue when a test method name does not match the provided regular expression.
AssertJ assertions taking `Consumer objects as arguments are expected to contain “requirements”, which should themselves be expressed as assertions. This concerns the following methods: allSatisfy, anySatisfy, hasOnlyOneElementSatisfying, isInstanceOfSatisfying, noneSatisfy, satisfies, satisfiesAnyOf, zipSatisfy.
These methods are assuming the Consumer will do the assertions itself. If you do not do any assertion in the Consumer, it probably means that you are inadvertently only partially testing your object.
This rule raises an issue when a Consumer` argument of any of the above methods does not contain any assertion.
Serialization is a platform-independent mechanism for writing the state of an object into a byte-stream. For serializing the object, we call the writeObject() method of java.io.ObjectOutputStream class. Only classes that implement Serializable or extend a class that does it can successfully be serialized (or de-serialized).
Attempting to write a class with the writeObject method of the ObjectOutputStream class that does not implement Serializable or extends a class that implements it, will throw an IOException.
Providing a `serialVersionUID field on Serializable classes is strongly recommended by the Serializable documentation but blindly following that recommendation can be harmful.
serialVersionUID value is stored with the serialized data and this field is verified when deserializing the data to ensure that the code reading the data is compatible with the serialized data. In case of failure, it means the serialized data and the code are not in sync and this fine because you know what’s wrong.
When the serialVersionUID is generated by an IDE or blindly hard-coded, there is a high probability that one will forget to update the serialVersionUID value when the Serializable class is later enriched with additional fields. As a consequence, old serialized data will incorrectly be considered compatible with the newer version of the code creating situations which are hard to debug.
Therefore, defining serialVersionUID should be done with care. This rule raises an issue on each serialVersionUID field declared on classes implementing Serializable to be sure the presence and the value of the serialVersionUID` field is challenged and validated by the team.
The `Files.exists method has noticeably poor performance in JDK 8, and can slow an application significantly when used to check files that don’t actually exist.
The same goes for Files.notExists, Files.isDirectory and Files.isRegularFile from java.nio.file package.
Note that this rule is automatically disabled when the project’s sonar.java.source` is not 8.
Catching `Exception seems like an efficient way to handle multiple possible exceptions. Unfortunately, it traps all exception types, both checked and runtime exceptions, thereby casting too broad a net. Indeed, was it really the intention of developers to also catch runtime exceptions? To prevent any misunderstanding, if both checked and runtime exceptions are really expected to be caught, they should be explicitly listed in the catch clause.
This rule raises an issue if Exception is caught when it is not explicitly thrown by a method in the try` block.
Mockito provides argument matchers and argument captors for flexibly stubbing or verifying method calls.
Mockito.verify(), Mockito.when(), Stubber.when() and BDDMockito.given() each have overloads with and without argument matchers.
However, if argument matchers or captors are used only on some of the parameters, all the parameters need to have matchers as well, otherwise an InvalidUseOfMatchersException will be thrown.
This rule consequently raises an issue every time matchers are not used on all the parameters of a stubbed/verified method.
The Thread class has some methods that are used to monitor and manage its execution. With the introduction of virtual threads in Java 21, there are three of these methods that behave differently between the standard platform threads and the virtual ones.
For virtual threads:
Thread.setDaemon(boolean) will throw an IllegalArgumentException if false is passed as an argument as a virtual thread daemon status is always true.
Thread.setPriority(int priority) will never change the actual priority of a virtual thread, which is always equal to Thread.NORM_PRIORITY
Thread.getThreadGroup() will return a dummy “VirtualThreads” group that is empty and should not be used
This rule reports an issue when one of these methods is invoked on a virtual thread.
The likely intention of a user calling Thread.run() is to start the execution of code within a new thread. This, however, is not what happens when this method is called.
The purpose of Thread.run() is to provide a method that users can overwrite to specify the code to be executed. The actual thread is then started by calling Thread.start(). When Thread.run() is called directly, it will be executed as a regular method within the current thread.
To prevent URL spoofing, HostnameVerifier.verify() methods should do more than simply return true
. Doing so may get you quickly past an exception, but that comes at the cost of opening a security hole in your application.
Non-abstract classes and enums with non-static, private
members should explicitly initialize those members, either in a constructor or with a default value.
Naming a thread won’t make it run faster or more reliably, but it will make it easier to deal with if you need to debug the application.
In Java, value-based classes are those for which instances are final and immutable, like String, Integer and so on, and their identity relies on their value and not their reference. When a variable of one of these types is instantiated, the JVM caches its value, and the variable is just a reference to that value. For example, multiple String variables with the same value “Hello world!” will refer to the same cached string literal in memory.
The synchronized keyword tells the JVM to only allow the execution of the code contained in the following block to one Thread at a time. This mechanism relies on the identity of the object that is being synchronized between threads, to prevent that if object X is locked, it will still be possible to lock another object Y.
It means that the JVM will fail to correctly synchronize threads on instances of the aforementioned value-based classes, for instance:
In asynchronous testing, the test code is written in a way that allows it to wait for the asynchronous operation to complete before continuing with the test.
Using Thread.sleep in this case can cause flaky tests, slow test execution, and inaccurate test results. It creates brittle tests that can fail unpredictably depending on the environment or load.
Use mocks or libraries such as Awaitility instead. These tools provide features such as timeouts, assertions, and error handling to make it easier to write and manage asynchronous tests.
Before it reclaims storage from an object that is no longer referenced, the garbage collector calls finalize() on the object.
This is a good time to release resources held by the object.
Because the general contract is that the finalize method should only be called once per object, calling this method explicitly is misleading and does not respect this contract.
When two locks are held simultaneously, a wait
call only releases one of them. The other will be held until some other thread requests a lock on the awaited object. If no unrelated code tries to lock on that object, then all other threads will be locked out, resulting in a deadlock.
Resources that can be reused across multiple invocations of the Lambda function should be initialized at construction time. For example in the constructor of the class, or in field initializers. This way, when the same container is reused for multiple function invocations, the existing instance can be reused, along with all resources stored in its fields. It is a good practice to reuse SDK clients and database connections by initializing them at class construction time, to avoid recreating them on every lambda invocation. Failing to do so can lead to performance degradation, and when not closed properly, even out of memory errors.
This rule reports an issue when the SDK client or the database connection is initialized locally inside a Lambda function.
Java 21 adds new String.indexOf methods that accept ranges (beginIndex, to endIndex) rather than just a start index. A StringIndexOutOfBounds can be thrown when indicating an invalid range, namely when:
beginIndex > endIndex (eg: beginIndex and endIndex arguments are mistakenly reversed)
beginIndex < 0 (eg: because the older String.indexOf(what, fromIndex) accepts negative values)
An inner class that extends another type can call methods from both the outer class and parent type directly, without prepending super. or Outer.this..
When both the outer and parent classes contain a method with the same name, the compiler will resolve an unqualified call to the parent type’s implementation. The maintainer or a future reader may confuse the method call as calling the outer class’s implementation, even though it really calls the super type’s.
To make matters worse, the maintainer sees the outer class’s implementation in the same file as the call in the inner class, while the parent type is often declared in another file. The maintainer may not even be aware of the ambiguity present, as they do not see the parent’s implementation.
An XML External Entity or XSLT External Entity (XXE) vulnerability can occur when a `javax.xml.transform.Transformer is created without enabling “Secure Processing” or when one is created without disabling resolving of both external DTDs and DTD entities. If that external data is being controlled by an attacker it may lead to the disclosure of confidential data, denial of service, server side request forgery, port scanning from the perspective of the machine where the parser is located, and other system impacts.
This rule raises an issue when a Transformer` is created without either of these settings.
There is no need to declare a type parameter when naming a type constraint is not required. Using wildcards makes it easier to read.
Overriding a parent class method prevents that method from being called unless an explicit super call is made in the overriding method. In some cases, not calling the parent method is fine. However, setUp and tearDown provide some shared logic that is called before all test cases. This logic may change over the lifetime of your codebase. To make sure that your test cases are set up and cleaned up consistently, your overriding implementations of setUp and tearDown should call the parent implementations explicitly.
If you end up mocking every non-private method of a class in order to write tests, it is a strong sign that your test became too complex, or that you misunderstood the way you are supposed to use the mocking mechanism.
You should either refactor the test code into multiple units, or consider using the class itself, by either directly instantiating it, or creating a new one inheriting from it, with the expected behavior.
This rule reports an issue when every member of a given class are mocked.
AssertJ assertions `allMatch and doesNotContains on an empty list always returns true whatever the content of the predicate. Despite being correct, you should make explicit if you expect an empty list or not, by adding isEmpty()/isNotEmpty() in addition to calling the assertion, or by testing the list’s content further. It will justify the useless predicate to improve clarity or increase the reliability of the test.
This rule raises an issue when any of the methods listed are used without asserting that the list is empty or not and without testing the content.
Targetted methods:
allMatch
allSatisfy
doesNotContain
doesNotContainSequence
doesNotContainSubsequence
doesNotContainAnyElementsOf`
In Spring applications, application components that expose interfaces should be package protected at most, not public
. Such reduced visibility helps ensure that the interface is only accessed through the container and not directly.
No matter whether the optional value is present or not, `Optional::orElse’s argument will always be executed. This is usually not what the developer intended when the content of the orElse() call has side effects. Even when no side effect is involved, the unnecessary computation of the orElse() clause might be a waste of resources.
Calls to Optional::orElse should be replaced with Optional::orElseGet whenever the alternative value is not a constant.
This rule raises an issue when Optional::orElse` is called with an argument that doesn’t evaluate to a constant value.
Characters like ‘é’ can be expressed either as a single code point or as a cluster of the letter ‘e’ and a combining accent mark. Without the CANON_EQ
flag, a regex will only match a string in which the characters are expressed in the same way.
Calling Iterator.hasNext() is not supposed to have any side effects and hence should not change the iterator’s state. Iterator.next() advances the iterator by one item. So calling it inside Iterator.hasNext() breaks the hasNext() contract and will lead to unexpected behavior in production.
While you can use either forEach(list::add) or collect with a Stream, collect
is by far the better choice because it’s automatically thread-safe and parallellizable.
Constructors should not access the values of fields that haven’t yet been initialized.
The contract of the Object.finalize()
method is clear: only the Garbage Collector is supposed to call this method.
Making this method public is misleading, because it implies that any caller can use it.
`getClass should not be used for synchronization in non-final classes because child classes will synchronize on a different object than the parent or each other, allowing multiple threads into the code block at once, despite the synchronized keyword.
Instead, hard code the name of the class on which to synchronize or make the class final`.
Since assert
statements aren’t executed by default (they must be enabled with JVM flags) developers should never rely on their execution the evaluation of any logic required for correct program function.
A for loop with a counter moving away from the end of the specified range is likely a programming mistake.
If the intention is to iterate over the specified range, this differs from what the loop does because the counter moves in the wrong direction.
If the intention is to have an infinite loop or a loop terminated only by a break statement, there are two problems:
The loop condition is not infinite because the counter variable will eventually overflow and fulfill the condition. This can take a long time, depending on the data type of the counter.
An infinite loop terminated by a break statement should be implemented using a while or do while loop to make the developer’s intention clear to the reader.
This rule allows banning usage of certain constructors.
The methods declared in an `interface are public and abstract by default. Any variables are automatically public static final. Finally, class and interface are automatically public static. There is no need to explicitly declare them so.
Since annotations are implicitly interfaces, the same holds true for them as well.
Similarly, the final modifier is redundant on any method of a final class, private is redundant on the constructor of an Enum, and static is redundant for interface nested into a class or enum`.
When testing exception via @Test
annotation, having additional assertions inside that test method can be problematic because any code after the raised exception will not be executed. It will prevent you to test the state of the program after the raised exception and, at worst, make you misleadingly think that it is executed.
You should consider moving any assertions into a separate test method where possible, or using org.junit.Assert.assertThrows instead.
Alternatively, you could use try-catch idiom for JUnit version < 4.13 or if your project does not support lambdas.
AssertJ assertions methods targeting the same object can be chained instead of using multiple `assertThat. It avoids duplication and increases the clarity of the code.
This rule raises an issue when multiples assertThat` target the same tested value.
Calling toString() or clone() on an object should always return a string or an object. Returning null
instead contravenes the method’s implicit contract.
`switch can contain a default clause for various reasons: to handle unexpected values, to show that all the cases were properly considered, etc.
For readability purposes, to help a developer quickly spot the default behavior of a switch statement, it is recommended to put the default clause at the end of the switch statement.
This rule raises an issue if the default clause is not the last one of the switch’s cases.
Java 7 introduced the ability to use a digit separator (_
) to split a literal number into groups of digits for better readability.
To ensure that readability is really improved by using digit separators, this rule verifies:
Homogeneity
Except for the left-most group, which can be smaller, all groups in a number should contain the same number of digits. Mixing group sizes is at best confusing for maintainers, and at worst a typographical error that is potentially a bug.
Standardization
It is also confusing to regroup digits using a size that is not standard. This rule enforce the following standards:
Decimal numbers should be separated using groups of 3 digits.
Hexadecimal numbers should be separated using groups of 2 or 4 digits.
Octal and Binary should be separated using groups of 2, 3 or 4 digits.
Furthermore, using groups with more than 4 consecutive digits is not allowed because they are difficult for maintainers to read.
By contract, fields in a Serializable class must themselves be either Serializable or transient. Even if the class is never explicitly serialized or deserialized, it is not safe to assume that this cannot happen. For instance, under load, most J2EE application frameworks flush objects to disk.
An object that implements Serializable but contains non-transient, non-serializable data members (and thus violates the contract) could cause application crashes and open the door to attackers. In general, a Serializable class is expected to fulfil its contract and not exhibit unexpected behaviour when an instance is serialized.
This rule raises an issue on:
Non-Serializable fields.
When a field is assigned a non-Serializable type within the class.
Collection fields when they are not private. Values that are not serializable could be added to these collections externally. Due to type erasure, it cannot be guaranteed that the collection will only contain serializable objects at runtime despite being declared as a collection of serializable types.
Using certain features of regular expressions, it is possible to create regular expressions that can never match or contain subpatterns that can never match. Since a pattern or sub-pattern that can never match any input is pointless, this is a sign that the pattern does not work as intended and needs to be fixed.
This rule finds some such regular expressions and subpatterns, specifically ones that meet one of the following conditions:
Beginning- and end-of-line/input boundaries appearing in a position where they can never match (e.g. an end-of-input marker being followed by other characters)
A back reference refers to a capturing group that will never be matched before the back reference
A `serialVersionUID field is strongly recommended in all Serializable classes. If you do not provide one, one will be calculated for you by the compiler. The danger in not explicitly choosing the value is that when the class changes, the compiler will generate an entirely new id, and you will be suddenly unable to deserialize (read from file) objects that were serialized with the previous version of the class.
serialVersionUID’s should be declared with all of these modifiers: static final long`.
Abstract classes should not have public constructors. Constructors of abstract classes can only be called in constructors of their subclasses. So there is no point in making them public. The protected
modifier should be enough.
A conditional operator is sometimes cluttering readability, if one of the operand is a boolean literal it can be simplified in a boolean expression :
Cloneable is the marker Interface that indicates that clone() may be called on an object. Overriding clone() without implementing Cloneable
can be useful if you want to control how subclasses clone themselves, but otherwise, it’s probably a mistake.
Different formatters use different formatting symbols, and it can be easy to confuse one for the other. But get it wrong, and your output may be useless.
This rule logs an issue when the wrong type of format string is used for Guava, slf4j, logback or MessageFormat
strings.