This content originally appeared on Level Up Coding - Medium and was authored by Lucas Fernandes
Ultimate Guideline For a Good Code Review
This Guideline Must Be Your Developer Teams Bible
Introduction
In software development, code quality is one of the fundamental pillars for the success of any project. One of the most effective practices to ensure this quality is code review.
Although it is a well-known and widely adopted practice, there is no magic formula for how to do it. In many places I’ve worked, it became a mere “formality,” without the development team conducting a thorough analysis of code quality.
Over my years of experience, I’ve compiled a set of best practices based on my knowledge, learning from my colleagues, and experience in corporate projects.
Without further ado, I would like to present the “Bible” for a good Code Review.
1. SOLID Principles
SOLID is an acronym for Single Responsibility, Open/Closed, Liskov Substitution, Interface Segregation, and Dependency Inversion. These are five software design principles that help create more robust, scalable, and maintainable systems. They were popularized by Robert C. Martin (also known as Uncle Bob) and are widely used in object-oriented software development.
I won’t delve into the theoretical aspects of each principle, as you, dear reader, have likely heard this term thousands of times. Instead, I will show you how to apply them practically during code review throughout this article.
1.1 Use of Design Patterns
A solid understanding and application of Design Patterns is a great way to gauge a developer’s seniority and the quality of their code. Although they are also widely known in theory, many developers fail to recognize their use during development.
Here are some scenarios with examples of code without and with the use of Design Patterns.
Strategy
I must confess that this is my favorite design pattern. Its use is relatively simple, and applying it often helps us clean up our code from “if-else.” Here’s an example scenario where the Strategy Pattern fits perfectly.
Scenario: Imagine an application responsible for a Bank Points Program, where the use of each type of card generates points. Our bank, LucasBank, has 3 types of cards: Basic, Silver, and Gold. Each card type has its multiplier based on the amount spent on each purchase.
Solution 1 (Sem Strategy):
public class CalculationService {
public Integer calculatePoints(CardType cardType, BigDecimal amount) {
if (CardType.BASIC.equals(cardType)) {
// give 10% points
return amount.multiply(new BigDecimal("0.10")).setScale(0, RoundingMode.UP).intValue();
} else if (CardType.SILVER.equals(cardType)) {
// give 20% points
return amount.multiply(new BigDecimal("0.20")).setScale(0, RoundingMode.UP).intValue();
} else {
// give 30% points
return amount.multiply(new BigDecimal("0.30")).setScale(0, RoundingMode.UP).intValue();
}
}
}
public enum CardType {
BASIC, SILVER, GOLD;
}💡Insights:
- Note that the CalculationService class is responsible for knowing all the calculation rules. (❌ Violates the Single Responsibility Principle)
- In the case of new card types, another “if” will always be added, making our code extremely verbose. Any change to a specific card affects the class responsible for all calculations. (❌ Violates the Open/Closed Principle)
Solution 2 (Using Strategy + Template Method)
public class CalculationService {
public Integer calculatePoints(CardType cardType, BigDecimal amount) {
return cardType.calculatePoints(amount);
}
public enum CardType {
BASIC {
@Override
public BigDecimal multiplyFactor() {
return BASIC_MULTIPLY_FACTOR;
}
}, SILVER {
@Override
public BigDecimal multiplyFactor() {
return SILVER_MULTIPLY_FACTOR;
}
}, GOLD {
@Override
public BigDecimal multiplyFactor() {
return GOLD_MULTIPLY_FACTOR;
}
};
static final BigDecimal BASIC_MULTIPLY_FACTOR = new BigDecimal("0.10");
static final BigDecimal SILVER_MULTIPLY_FACTOR = new BigDecimal("0.20");
static final BigDecimal GOLD_MULTIPLY_FACTOR = new BigDecimal("0.30");
abstract BigDecimal multiplyFactor();
public Integer calculatePoints(BigDecimal amount) {
return amount.multiply(multiplyFactor()).setScale(0, RoundingMode.UP).intValue();
}
}💡Insights:
- Note that now our CardType Enum is the sole responsible for knowing the calculation for each card type! Instead of having multiple “if-else” statements, each instance of our Enum owns its own rule. (✅ Single Responsibility)
- Note that the main calculation method is executed by the same line of code: return amount.multiply(multiplyFactor()).setScale(0, RoundingMode.UP).intValue();. However, there is a "gap" that is filled by each type of instance, using polymorphism and applying another design pattern: Template Method. (✅ Open/Closed)
Chain of Responsability
Although it is very useful, even somewhat experienced developers often struggle to recognize the use of the Chain of Responsibility.
Essentially, its idea revolves around passing a request through a chain of handlers. At each step of the chain, the link decides whether to process the request and respond or pass it to the next link in the chain. It always follows a specific order. Confused? Let’s look at the following scenario:
Scenario: Imagine we are developing a payment application, and part of this feature involves validating whether the user who wants to make the payment has: a positive balance, allows for a negative balance, or permits using a loan with a credit card limit.
Regra:
- Check if there is sufficient balance.
- Check if a negative balance is allowed.
- Check if using credit from the credit card is allowed.
Solution 1:
@Service
public class UserBalanceService {
public boolean authorizePayment(UserBalance userBalance, BigDecimal amount) {
if (amount.compareTo(userBalance.getBalance()) > 0) {
return true;
}
if (userBalance.isAllowNegativeBalance()) {
return true;
}
if (userBalance.isAllowUseCredit()) {
return true;
}
return false;
}
public class UserBalance {
private BigDecimal balance;
private boolean allowNegativeBalance;
private boolean allowUseCredit;
public UserBalance(BigDecimal balance, boolean allowNegativeBalance, boolean allowUseCredit) {
this.balance = balance;
this.allowNegativeBalance = allowNegativeBalance;
this.allowUseCredit = allowUseCredit;
}
public BigDecimal getBalance() {
return balance;
}
public boolean isAllowNegativeBalance() {
return allowNegativeBalance;
}
public boolean isAllowUseCredit() {
return allowUseCredit;
}
}
}
💡Insights:
- Notice that we have a single method: authorizePayment, which manages all possible authorization rules. Imagine writing a unit test for this method. At least three test scenarios would be needed to validate one method! (❌ Violates the Single Responsibility Principle).
- If more authorizations are added, our “if” statements will grow, making the code redundant and not extensible. (❌ Violates the Open/Closed Principle).
Solution 2:
@Service
public class UserBalanceService {
// Spring loads all implementations of AuthorizationPayment ordered!
private final Set<AuthorizationPayment> authorizationPayments;
public UserBalanceService(Set<AuthorizationPayment> authorizationPayments) {
this.authorizationPayments = authorizationPayments;
}
public boolean authorizePayment(UserBalance userBalance, BigDecimal amount) {
return authorizationPayments.stream()
.anyMatch(authorizationPayment -> authorizationPayment.authorizePayment(userBalance, amount));
}
public interface AuthorizationPayment {
boolean authorizePayment(UserBalance userBalance, BigDecimal amount);
}
@Order(1)
@Component
public class BalanceAuthorizationPayment implements AuthorizationPayment {
@Override
public boolean authorizePayment(UserBalance userBalance, BigDecimal amount) {
return amount.compareTo(userBalance.getBalance()) > 0;
}
}
@Order(2)
@Component
public class NegativeBalanceAuthorizationPayment implements AuthorizationPayment {
@Override
public boolean authorizePayment(UserBalance userBalance, BigDecimal amount) {
return userBalance.isAllowNegativeBalance();
}
}
@Order(3)
@Component
public class CreditAuthorizationPayment implements AuthorizationPayment {
@Override
public boolean authorizePayment(UserBalance userBalance, BigDecimal amount) {
return userBalance.isAllowUseCredit();
}
}
public class UserBalance {
private BigDecimal balance;
private boolean allowNegativeBalance;
private boolean allowUseCredit;
public UserBalance(BigDecimal balance, boolean allowNegativeBalance, boolean allowUseCredit) {
this.balance = balance;
this.allowNegativeBalance = allowNegativeBalance;
this.allowUseCredit = allowUseCredit;
}
public BigDecimal getBalance() {
return balance;
}
public boolean isAllowNegativeBalance() {
return allowNegativeBalance;
}
public boolean isAllowUseCredit() {
return allowUseCredit;
}
}
}
💡Insights:
- Notice that now each handler performs its specific task, encapsulating our business rule and delegating responsibility. (✅ Single Responsibility Principle)
- Notice that now our UserBalanceService class is completely agnostic to any changes in the authorization types, whether they are added or removed. (✅ Open/Closed Principle)
- Now, our service class depends on an interface, and we use polymorphism for its execution. (✅ Liskov Substitution Principle)
- Notice that our interface is lean and concise. (✅ Interface Segregation Principle)
- Our service class doesn’t need to know about the implementations of the interface or how to create them. We simply tell the Spring container: “Hey, give me all the ordered implementations!” (✅ Dependency Inversion Principle)
2. DRY (Don’t Repeat Yourself)
2.1 Use of Generics
A powerful feature of Java is Generics. It allows us to parameterize types of classes that share a similar data structure or behavior. In the JDK, we have several examples of its use, especially when looking at interfaces like List, Set, Map, and others.
So, how can we apply it? Let’s look at our problem!
Scenario: We have an application with multiple entities that require CRUD operations. Using popular Java and Spring frameworks, a possible implementation of CRUD could look like this:
Solution 1:
Class PersistenceEmployeeService:
@Component
@RequiredArgsConstructor
public class PersistenceEmployeeService {
private final EmployeeRepository employeeRepository;
public EmployeeEntity save(EmployeeEntity employee) {
return employeeRepository.save(employee);
}
public EmployeeEntity findById(Long id) {
return employeeRepository.findById(id).orElseThrow(() -> new RuntimeException("Employee not found"));
}
public void delete(EmployeeEntity employee) {
employeeRepository.delete(employee);
}
}
Class PersistenceDepartmentService
@Component
@RequiredArgsConstructor
public class PersistenceDepartmentService {
private final DepartmentRepository departmentRepository;
public DepartmentEntity save(DepartmentEntity department) {
return departmentRepository.save(department);
}
public DepartmentEntity findById(Long id) {
return departmentRepository.findById(id).orElseThrow(() -> new RuntimeException("Department not found"));
}
public void delete(DepartmentEntity department) {
departmentRepository.delete(department);
}
}
💡Insights:
- Notice that the classes are essentially identical, only differing in the parameter, return type, and which repository is injected. (❌ Violates the DRY Principle — Don’t Repeat Yourself)
Solution 2:
Class EntityPersistenceService:
public abstract class PersistenceEntityService<T> {
public T save(T entity) {
return repository().save(entity);
}
public T findById(Long id) {
return repository().findById(id).orElseThrow(() -> new RuntimeException("Entity not found"));
}
public void delete(T entity) {
repository().delete(entity);
}
protected abstract CrudRepository<T, Long> repository();
}Classe PersistenceDepartmentService:
@Component
@RequiredArgsConstructor
public class PersistenceDepartmentService extends PersistenceEntityService<DepartmentEntity> {
private final DepartmentRepository departmentRepository;
@Override
public DepartmentRepository repository() {
return departmentRepository;
}
}
Class PersistenceEmployeeService:
@Component
@RequiredArgsConstructor
public class PersistenceEmployeeService extends PersistenceEntityService<EmployeeEntity>{
private final EmployeeRepository employeeRepository;
@Override
protected CrudRepository<EmployeeEntity, Long> repository() {
return employeeRepository;
}
}
💡Insights:
- Now, all classes that extend the abstract class only need to specify which Entity class they handle and know which repository to inject. (✅ DRY Principle — Don’t Repeat Yourself)
4. Meaningful Classes, Methods and Variables Names
When writing our code, it is vitally important to faithfully represent our domain, and this includes terms, names, and expressions that make sense for the business. We need to abstract real-world objects and model them computationally.
Take a look at the following class that represents a User and its attributes: date of birth, active/inactive status, and salary.
Solution 1:
public class User {
private LocalDate date;
private boolean flag;
private BigDecimal number;
}💡Insights:
- Notice that someone who is encountering the application’s code for the first time, they are completely “in the dark” about the semantic and business meaning of the three properties above, even if the code is functioning correctly.
Solution 2:
public class User {
private LocalDate birthDate;
private boolean active;
private BigDecimal salary;
}💡Insights:
- In this solution, when we look at our User class, we have complete clarity about the value and what each attribute represents semantically.
- Although some people I know have an issue with long names, personally, this is not a concern for me. What matters is: faithfully representing and conveying the semantic value of the class, method, or variable!
5. Avoid Magic Numbers
Avoiding “magic numbers” or values is closely related to the previous topic. The core idea remains the same: to accurately represent our domain, including terms, names, and expressions that make sense for the business.
Scenario: Calculate the net salary after applying a 27% discount (e.g., taxes, deductions) and then determine the average net wage per business day. Assumptions:
- The employee works 8 hours per day.
- There are 22 business days in a month (typical for many countries).
Solution 1:
public class FinancialReportService {
public void calculateAverageNetSalaryByBussinessDay(BigDecimal salary) {
BigDecimal netSalary = salary.subtract(salary.multiply(new BigDecimal("0.27")));
BigDecimal averageNetSalaryByBussinessDay = netSalary.divide(new BigDecimal("22"), 2, RoundingMode.UP);
System.out.println("Average net salary by business day: " + averageNetSalaryByBussinessDay);
}
}💡Insights:
- Implicit knowledge that 0.27 is the tax discount rate.
- Implicit knowledge that 22 days is the number of working days in a month.
- If these values are reused in another method of the class, they will be duplicated.
Solution 2:
public class FinancialReportService {
private static final BigDecimal DISCOUNT = new BigDecimal("0.27");
private static final BigDecimal BUSINESS_DAYS = new BigDecimal("22");
private static final int DECIMALS = 2;
public void calculateAverageNetSalaryByBussinessDay(BigDecimal salary) {
BigDecimal netSalary = salary.subtract(salary.multiply(DISCOUNT));
BigDecimal averageNetSalaryByBussinessDay = netSalary.divide(BUSINESS_DAYS, DECIMALS, RoundingMode.UP);
System.out.println("Average net salary by business day: " + averageNetSalaryByBussinessDay);
}
}💡Insights:
- We no longer need to “infer” the discount value, working days, or the rounding decimal place.
- If these values are reused in another method of the class, they will not be duplicated.
6. Handle Generic Exceptions
When we handle generic exceptions, we are essentially saying: “Any problem that occurs, handle it this way!” The issue is that it becomes unclear for the developer to know what specific problems might occur because we are being too generic.
Solution 1:
try {
int result = 10 / 0; // This will throw ArithmeticException
} catch (Exception e) { // Bad: Catching generic Exception
System.out.println("An error occurred: " + e.getMessage());
}💡Insights:
- For example, catching Exception might hide a NullPointerException, ArrayIndexOutOfBoundsException, or other specific exceptions that should be handled differently.
- If you catch a generic exception and don’t handle it properly, you might accidentally “swallow” critical exceptions that should stop the program or be logged for further investigation.
- Generic exception handling makes the code harder to maintain because it doesn’t explicitly state what types of exceptions are expected and how they should be handled.
Solution 2:
try {
int result = 10 / 0;
} catch (ArithmeticException e) { // Good: Catching specific exception
System.out.println("Division by zero error: " + e.getMessage());
}💡Insights:
- Always catch the most specific exception type possible. This makes the code more readable and ensures that each exception is handled appropriately.
- Never catch an exception and do nothing with it (e.g., empty catch block or only logging message). At a minimum, log the exception for debugging purposes.
7. Less Code Is Not Synonymous with Better Code
When I started my development career, I thought that fewer lines of code were synonymous with better code. But through real-world experience and debugging issues, I realized that wasn’t necessarily the case.
Let’s go through a scenario to explain this better.
Scenario: Algorithm to check if a string is a palindrome.
Solution 1:
public boolean isPalindrome(String str) {
return str.replaceAll("\\s", "").equalsIgnoreCase(new StringBuilder(str).reverse().toString().replaceAll("\\s", ""));
}💡Insights:
- The code is compact but hard to understand at a glance. It combines multiple operations (replacing spaces, reversing the string, and comparing) into a single line.
- If you need to modify the logic (e.g., handle special characters or improve performance), it’s harder to do so without breaking the code.
Solution 2:
public boolean isPalindrome(String str) {
String cleanedStr = str.replaceAll("\\s", "").toLowerCase();
String reversedStr = new StringBuilder(cleanedStr).reverse().toString();
return cleanedStr.equals(reversedStr);
}💡Insights:
- The code is broken into logical steps, making it easier to understand.
- If you need to change the logic (e.g., ignore punctuation or improve performance), you can easily modify the relevant part without affecting the rest of the code.
- If something goes wrong, it’s easier to debug because each step is isolated.
9. Always Think Big
When developing a feature locally, everything is much easier. Ideally, we have a local database with only a few records in the tables, and there are no latency issues since our database runs on the same machine as the application. Life is good!
But when we deploy our code to production, the real world is a bit different. We deal with thousands or even millions of records, and the communication time between the application and the database increases, which can lead to bugs that weren’t considered during development.
That’s why we must “Always Think Big”! Always ask yourself: “What if millions of records are returned?”. Now, let’s look at a scenario.
Scenario 1: A simple user search endpoint.
Solution 1:
@RequiredArgsConstructor
@RequestMapping("/users")
@RestController
public class UserResource {
private final UserService userService;
@GetMapping
public ResponseEntity<List<User>> getAll() {
return ResponseEntity.ok(userService.findAll());
}
}
💡Insights:
- We are returning a list that could contain 1, 10, 100, or even 100 million users. Would a frontend consuming this service be able to handle such a large number of users easily?
Solution 2:
@RequiredArgsConstructor
@RequestMapping("/users")
@RestController
public class UserResource {
private final UserService userService;
@GetMapping
public ResponseEntity<Page<User>> getAll(Pageable pageable) {
return ResponseEntity.ok(userService.findAll(pageable));
}
}
💡Insights:
- Now, we have parameterized our endpoint to support pagination, allowing consumers to specify the page size, number of pages, and filters they need for more flexibility!
Scenario 2: Simple @One-to-Many mapping between Employee and Department (Eager vs. Lazy).
Solution 1:
Class EmployeeEntity:
@Entity
@AllArgsConstructor
@Getter
public class EmployeeEntity {
@Id
@GeneratedValue(strategy = jakarta.persistence.GenerationType.IDENTITY)
private Long id;
private String name;
@ManyToOne(optional = false)
@JoinColumn(name = "id_departament", nullable = false, referencedColumnName = "id")
private DepartmentEntity department;
}
Class DepartmentEntity:
@Entity
@AllArgsConstructor
@Getter
public class DepartmentEntity {
@Id
@GeneratedValue(strategy = jakarta.persistence.GenerationType.IDENTITY)
private Long id;
private String name;
@OneToMany(mappedBy = "department", fetch = FetchType.EAGER)
private Set<EmployeeEntity> employees;
}
💡Insights:
- FetchType.EAGER ensures that every time a Department is loaded by the EntityManager, the list of employees is also loaded. But what if a department has 1 million employees?
Solution 2:
Class DepartmentEntity:
@Entity
@AllArgsConstructor
@Getter
public class DepartmentEntity {
@Id
@GeneratedValue(strategy = jakarta.persistence.GenerationType.IDENTITY)
private Long id;
private String name;
@OneToMany(mappedBy = "department", fetch = FetchType.LAZY)
private Set<EmployeeEntity> employees;
}
💡Insights:
- FetchType.LAZY tells the EntityManager that this data set should not be loaded automatically, leaving it up to the developer to explicitly fetch it using specific queries (e.g., with Fetch Join).
10. Tell Don’t Ask
Although applying this concept is a subtle change, it is intrinsically linked to SOLID principles and the pillars of Object-Oriented Programming.
The “Tell, Don’t Ask” principle is a design guideline in software engineering that promotes encapsulation and better object-oriented design. The core idea is that objects should tell other objects what to do, rather than asking for their internal state and making decisions on their behalf.
Scenario: We are developing a feature that requires a status flow, where each status knows its next state. Example of possible flows:
- Created -> Analysing -> Approved
- Created -> Analysing -> Reject
- Created -> Analysing -> Cancelled
Solution 1:
Enum Status:
public enum Status {
CREATED, ANALYSING, APPROVED, REJECT, CANCELLED;
}Class Flowservice:
public class FlowService {
public boolean checkStatusIsFinished(Status status) {
return Status.APPROVED == status || Status.CANCELLED == status || Status.REJECT == status;
}
}💡Insights:
- Notice that the service class has to keep checking each type of instance to determine if it is considered “finalized.” Instead of doing this, we should delegate this responsibility to the enum itself, which owns this information.
Solution 2:
Enum Status
public enum Status {
CREATED, ANALYSING, APPROVED, REJECT, CANCELLED;
public boolean isFinished() {
return this == APPROVED || this == REJECT || this == CANCELLED;
}
}Class Flowservice:
public class FlowService {
public boolean checkStatusIsFinished(Status status) {
return status.isFinished();
}
}💡Insights:
- Now, the Status itself knows whether it is finalized or not — we simply ask: “Status, are you finalized?”.
11. YAGNI (You Ain’t Gonna Need It)
The YAGNI principle represents a software development concept that has become quite popular in the Agile and Extreme Programming (XP) communities. This principle emphasizes the idea of not implementing features or making optimizations until they are truly necessary.
In other words, the YAGNI principle aligns with the philosophy of simplicity and agility in software development. It suggests that developers should not spend time and effort prematurely on features or optimizations that may never be needed. Instead, they should focus on what is necessary at the moment and add functionality as requirements evolve.
💡Insights:
- Classes, methods, attributes, or variables that are not being used should be removed!
- Unused private methods should also be eliminated!
12. Avoid NullPointerException
How to avoid the famous error that every developer has encountered: the NullPointerException.
Whenever we code, we need to have a mindset of “protecting” our application from unexpected errors! One of the ways we can do this, which Java has offered since version 8, is by using Optional.
Here’s an example of how we can implement “defensive programming”.
Solution 1:
@Component
@RequiredArgsConstructor
public class ShipmentService {
// if exists zipCode, return address; if not return null;
private final AddressService addressService;
public String findAddressByZipCode(User user) {
return addressService.findAddressByZipCode(user.zipCode);
}
}
💡Insights:
- Note that we simply call the addressService, trusting that it will always return a value. But what if it returns null? Any operation performed on the return of our findAddressByZipCode method will result in an NPE!
Solution 2:
@Component
@RequiredArgsConstructor
public class ShipmentService {
private final AddressService addressService;
public Optional<String> findAddressByZipCode(User user) {
return Optional.of(addressService.findAddressByZipCode(user.zipCode));
}
}
💡Insights:
- Now we are “protecting” our code. For any call to the findAddressByZipCode method, the caller will be responsible for handling the Optional!
Conclusion
These are the 12 key principles I carry with me wherever I go in the software development world.
Feel free to share your thoughts in the comments and let me know if there’s anything I should add. The goal is to keep evolving!
Thank you for making it this far and for your valuable time reading this article.
Before you go:
- Be sure to clap 👏
- Share with other developers who might benefit from this solution!
- Follow the author to support: https://medium.com/@lucas.rj.fernandes/subscribe
- Social Network: Medium | LinkedIn | Substack
- Follow the publication page: Level Up Coding | Relocate Jobs
- Donate: Buy me a Coffee ☕
The Ultimate Guideline For a Good Code Review was originally published in Level Up Coding on Medium, where people are continuing the conversation by highlighting and responding to this story.
This content originally appeared on Level Up Coding - Medium and was authored by Lucas Fernandes
Lucas Fernandes | Sciencx (2025-02-12T01:50:41+00:00) The Ultimate Guideline For a Good Code Review. Retrieved from https://www.scien.cx/2025/02/12/the-ultimate-guideline-for-a-good-code-review/
Please log in to upload a file.
There are no updates yet.
Click the Upload button above to add an update.