Qodana / Static Code Analysis Guide / Java Code Review Checklist and Best Practices

Java Code Review: Checklist and Best Practices

Code review is a common practice that developers perform before merging their code into the main branch. It helps you to catch and fix errors and potential problems before they become major issues. It also allows for other developers to provide feedback and suggestions on the code.

When doing a code review, you should keep a few things in mind. First, it is important to be respectful of the author's work. Second, you should be clear and concise in your comments. Finally, remember that the goal is to improve the code, not to find fault with the author.

One of the benefits of code reviews is that they allow developers to share responsibility for the code. By discussing the code and providing feedback, everyone can better understand how it works and how it could be improved. Reviewers have the opportunity to suggest alternative designs and discuss architectural decisions.

Adhering to best practices during the code review process helps ensure that it is valuable. This checklist outlines some of the most important things to consider when reviewing someone else's code. Ultimately your decision about which of these practices to adopt will depend largely on context and the needs of your workflow.

Agree on naming conventions

Java has well-defined conventions for naming classes, interfaces, methods, variables, and constants. These conventions are intended to make it easier to name things and to make code easier to read and maintain.

Your team may have agreed to deviate from the Java conventions in some cases. However, by enforcing these conventions code will be easier to read for developers new to the code base. According to the Java conventions:

  • Classes should be nouns and written with Pascal case, for example, BankAccount.
  • Interfaces should be adjectives also written in Pascal case, for example, Spendable.
  • Methods should be verbs and written in camel case, for example, transferPocketMoney.
  • Constants don’t change, so their names should describe what they are and be written in all caps and underscores, for example, WEEKLY_POCKET_MONEY_AMOUNT.

Paying attention to the names of classes can help ensure that each one has a single responsibility. If you're finding it hard to think of a good name for a class because it's doing too many things, then it is likely breaking that single responsibility pattern.

Enforce unified code styles

Code reviews are most efficient when the reviewer is familiar with the code's style. Not only does maintaining a consistent code style make it easier to read, but it can also make the code review process smoother.

Here are examples of some rules you may want to enforce across the board:

  • Functions should be less than 30 lines.
  • Tab indents are four spaces.
  • Curly braces start on the same line as the end of a function or class.
  • A new line is used for each parameter to a function or class.

Linting is the process of running a program that analyzes your code for errors and potential problems. With linting, you can enforce common coding style and formatting rules for your team. This helps your team stay aligned on what is acceptable stylistically and avoid disputes over formatting.

Lean on CI/CD automation

Use automated tests and linters to check code quality (this is where Qodana can help!). This will help to catch errors and inconsistent code before either becomes an issue.

If the automated tests and linters are in place, you can hold off on performing a code review until the CI/CD pipelines pass. This will save you time as a code reviewer, since the authors will be able to fix issues themselves without you having to point them out.

Consider whether classes need to be public

Java has the concept of nested classes, where one class is defined inside another. This helps organize logic while keeping implementation details hidden from outside code. The result is greater encapsulation and code that’s easier to maintain as it evolves.

When reviewing code, look for classes that should be grouped together, especially when one exists only to act as a helper to another.

A common argument for exposing a class is so it can be unit tested. But if the class is truly just an implementation detail, it doesn’t need its own public interface — its behavior will naturally be exercised when you test the main class. In other words, you test it indirectly through the main class’s interface rather than in isolation.

Concatenate strings with StringBuilder

When working with strings inside a loop, it is important to use the StringBuilder class instead of concatenating them with the + operator. StringBuilder is more efficient, as it doesn't create a new string each time you concatenate, which can lead to memory issues if the strings are large or if you're concatenating many strings together.

As an example, consider the following code:

StringBuilder sb = new StringBuilder();
String letters = “”;
for (int i = 0; i < 1000; i++) {
  sb.append("a");
  letters += “a”;
}
String s = sb.toString();

Though you should use a StringBuilder in a loop, concatenation with a + is no different outside a loop. This is because the Java compiler will automatically optimize the code and use a StringBuilder instead.

Check for nulls to avoid null pointer exceptions

Null pointer exceptions are runtime errors, meaning the compiler won't alert you to them. Because of this, you may need robust testing to prevent them from causing your code to crash. That’s why a reviewer needs to consider returned values when methods are called and variables are accessed.

If a variable is expected to be null, then a null check should be done before accessing it. Suggest Objects.requireNonNull(), Optional, or using @NotNull annotations. This operator will check whether a given reference is null before accessing it. If it is null, then it will return null instead of throwing an exception.

Here’s an example of null checking with an if condition:

if (obj != null) {
  obj.doSomething()
}

While this is what the same code looks like with the safe navigation operator:

obj?.doSomething()

You can suggest using the safe navigation operator in place of explicit if checks if doing so will make the code easier to read and require fewer lines.

Understanding the code you’re reading is important for providing valuable feedback about valid null checks. If some code depends on an external service, you must understand the possible values returned and consider the failure modes.

While null checks are important, you should make sure they aren’t used unnecessarily, as this can be confusing for a reader or even hide bugs if a null value should never occur.

Consider choosing polymorphism over conditionals

One of the key features of object-oriented programming is polymorphism — the ability for an object to take on multiple forms. For example, a shape can be a circle, square, or triangle. Each has its own properties, but all can be referred to as shapes.

Polymorphism can often replace long chains of if or switch statements. If you see code with many conditionals that check an object’s type, that’s usually a sign polymorphism would be a cleaner solution.

As an example, consider the following code:

if (shape instanceof Square) {
// do something
} else if (shape instanceof Circle) {
// do something
} else if (shape instanceof Triangle) {
// do something
}
This can be rewritten using polymorphism as follows:
shape.draw();

When reviewing code like this, you could suggest how polymorphism can reduce the need to use conditionals in multiple places.

Look out for memory leaks

Memory leaks happen when objects are no longer being used but are still referenced by the program. This means they can't be garbage collected and will stay in memory until the program exits, which can lead to OutOfMemoryError exceptions being thrown or performance degradation that is hard to debug.

Java’s garbage collector usually handles memory cleanup automatically, but leaks can still occur in some cases.

Make sure that resources are closed after using them, as in the example below. This is especially important when working with files, databases, and network connections.

FileInputStream fis = new FileInputStream("file.txt");
// do something with the file
fis.close();

Another way you may get a memory leak is by overusing static variables. These variables are initialized when the class is loaded and stay in memory until the program exits. If these variables hold onto references, a memory leak may result.

Suppose you had a class named Session that kept track of all the users who join a given session:

class Session {
  static User[] users;
}

The user array will stay in memory for the entire lifetime of the program. As the array grows you may run into memory issues.

Re-use frameworks and libraries

As your code evolves, you may encounter problems that seem beyond your framework. Rather than immediately adding a new library or swapping dependencies, first check the framework’s documentation for a built-in solution.

Frameworks like Spring have great documentation that you can read and link in code reviews.

Check automated test coverage

When changes are made to the code, it's important that tests are in place to cover the new functionality. These tests can be unit tests, integration tests, or end-to-end tests.

You can objectively measure code coverage using testing frameworks like JUnit 5 paired with static code analysis tools like the one built into IntelliJ IDEA for Java.

Test coverage shouldn't decrease with new changes. Encourage code authors to write tests while they write functional changes so the code reviewer can take them into consideration.

As a reviewer, think about the code scenarios covered by added and previously existing tests. Can you spot any gaps or repetitive testing that is unnecessary? If so, provide this additional context in the form of a suggested code flow or by explaining how a given scenario is already tested.

Implement Java logging

Java logging is a mechanism for recording messages during the execution of a program. These messages can be used to track down errors, diagnose performance issues, and give visibility into the internal workings of an application. Logs can also be a very useful form of telemetry for operational tasks.

If code paths introduced have a chance of failure, make sure they're wrapped in try…catch statements that attach additional information about why an exception was thrown and what values were used. Upstream services should then take this exception and log it appropriately.

You may even suggest info logging for complicated tasks like long-running background jobs. However, it’s important to ensure that no sensitive data or personally identifiable information is captured in these logs.

Clean up production code

When the code is merged into the main branch, it should be ready to go into production. This means all debugging log statements should be removed already.

If code is intentionally incomplete, make sure there are TODO or FIXME comments so the next person who comes along knows what needs to be finished. Don't leave code uncommented. As a reviewer, if you see code that is intentionally incomplete, suggest deleting it, as it can always be recovered using version control.

Sometimes you may decide as a team that a hack is preferred over a large refactoring or that edge cases for a feature can be ignored. This decision is called adding technical debt, and it should be documented somewhere. Encourage code authors who are creating tech debt to add a link to a resource detailing the decision being made and additional comments to provide some context.

Why use Qodana to maintain your Java code quality?

Use Qodana to bring IntelliJ IDEA inspections into your CI/CD pipeline. It will also help you run license audits and other basic security checks to ensure quality, scalability and maintainability.

You can even make code reviews simpler by automating checks, and protect your codebase by establishing quality gates.