java-code-review

name	java-code-review
description	Use when reviewing Java code for null safety, exception handling, concurrency issues, and performance concerns.

Java Code Review Guide

Null Safety

Use Optional for return types that may be absent

// Bad - returning null
public Customer findByEmail(String email) {
    return customerRepository.findByEmail(email); // may return null
}

// Good - use Optional
public Optional<Customer> findByEmail(String email) {
    return customerRepository.findByEmail(email);
}

// Good - handle Optional in callers
public CustomerResponse getCustomer(String email) {
    return customerRepository.findByEmail(email)
            .map(customerMapper::toResponse)
            .orElseThrow(() -> new ResourceNotFoundException("Customer not found: " + email));
}

Never pass null as a method argument

// Bad
public List<Order> findOrders(String customerId, String status, LocalDate from, LocalDate to) {
    // callers pass null for unused filters
}

// Good - use a criteria object or overloaded methods
public record OrderSearchCriteria(
    String customerId,
    Optional<String> status,
    Optional<LocalDate> from,
    Optional<LocalDate> to
) {}

public List<Order> findOrders(OrderSearchCriteria criteria) {
    Specification<Order> spec = Specification.where(null);
    spec = spec.and(OrderSpecifications.hasCustomer(criteria.customerId()));
    if (criteria.status().isPresent()) {
        spec = spec.and(OrderSpecifications.hasStatus(criteria.status().get()));
    }
    if (criteria.from().isPresent()) {
        spec = spec.and(OrderSpecifications.createdAfter(criteria.from().get()));
    }
    if (criteria.to().isPresent()) {
        spec = spec.and(OrderSpecifications.createdBefore(criteria.to().get()));
    }
    return orderRepository.findAll(spec);
}

Use Objects.requireNonNull for mandatory constructor parameters

@Service
public class PaymentService {

    private final PaymentGateway paymentGateway;
    private final OrderRepository orderRepository;
    private final AuditService auditService;

    public PaymentService(PaymentGateway paymentGateway,
                          OrderRepository orderRepository,
                          AuditService auditService) {
        this.paymentGateway = Objects.requireNonNull(paymentGateway, "paymentGateway must not be null");
        this.orderRepository = Objects.requireNonNull(orderRepository, "orderRepository must not be null");
        this.auditService = Objects.requireNonNull(auditService, "auditService must not be null");
    }
}

---

Exception Handling

Define domain-specific exceptions

public abstract class DomainException extends RuntimeException {
    private final String errorCode;

    protected DomainException(String errorCode, String message) {
        super(message);
        this.errorCode = errorCode;
    }

    public String getErrorCode() { return errorCode; }
}

public class ResourceNotFoundException extends DomainException {
    public ResourceNotFoundException(String resource, Object id) {
        super("NOT_FOUND", resource + " not found with id: " + id);
    }
}

public class BusinessRuleViolationException extends DomainException {
    public BusinessRuleViolationException(String message) {
        super("BUSINESS_RULE_VIOLATION", message);
    }
}

public class DuplicateResourceException extends DomainException {
    public DuplicateResourceException(String resource, String field, Object value) {
        super("DUPLICATE", resource + " already exists with " + field + ": " + value);
    }
}

Never catch generic Exception unless re-throwing

// Bad
try {
    processPayment(order);
} catch (Exception e) {
    log.error("Error", e);
    // swallowed - what was the error?
}

// Good - catch specific exceptions
try {
    processPayment(order);
} catch (PaymentDeclinedException e) {
    log.warn("Payment declined for order {}: {}", order.getId(), e.getMessage());
    order.setStatus(OrderStatus.PAYMENT_FAILED);
    orderRepository.save(order);
} catch (PaymentGatewayException e) {
    log.error("Payment gateway error for order {}", order.getId(), e);
    throw new ServiceUnavailableException("Payment service is temporarily unavailable");
}

Use try-with-resources

// Bad
InputStream is = new FileInputStream(file);
try {
    // process
} finally {
    is.close(); // may throw and mask original exception
}

// Good
try (InputStream is = new FileInputStream(file);
     BufferedReader reader = new BufferedReader(new InputStreamReader(is, StandardCharsets.UTF_8))) {
    return reader.lines().collect(Collectors.joining("\n"));
}

---

Concurrency

Use concurrent collections

// Bad - not thread-safe
private final Map<String, Session> sessions = new HashMap<>();

// Good
private final Map<String, Session> sessions = new ConcurrentHashMap<>();

Prefer atomic operations

// Bad - race condition
private int counter = 0;
public void increment() { counter++; }

// Good
private final AtomicInteger counter = new AtomicInteger(0);
public void increment() { counter.incrementAndGet(); }

Use @Async correctly with Spring

@Configuration
@EnableAsync
public class AsyncConfig implements AsyncConfigurer {

    @Override
    @Bean(name = "taskExecutor")
    public Executor getAsyncExecutor() {
        ThreadPoolTaskExecutor executor = new ThreadPoolTaskExecutor();
        executor.setCorePoolSize(5);
        executor.setMaxPoolSize(10);
        executor.setQueueCapacity(25);
        executor.setThreadNamePrefix("async-");
        executor.setRejectedExecutionHandler(new ThreadPoolExecutor.CallerRunsPolicy());
        executor.initialize();
        return executor;
    }

    @Override
    public AsyncUncaughtExceptionHandler getAsyncUncaughtExceptionHandler() {
        return (throwable, method, params) ->
                LoggerFactory.getLogger(method.getDeclaringClass())
                        .error("Async error in {}: {}", method.getName(), throwable.getMessage(), throwable);
    }
}

@Service
public class ReportService {

    private static final Logger log = LoggerFactory.getLogger(ReportService.class);

    @Async("taskExecutor")
    public CompletableFuture<ReportResult> generateReport(ReportRequest request) {
        log.info("Generating report on thread: {}", Thread.currentThread().getName());
        ReportResult result = buildReport(request);
        return CompletableFuture.completedFuture(result);
    }
}

Thread safety with @Transactional

@Service
public class AccountService {

    private final AccountRepository accountRepository;

    public AccountService(AccountRepository accountRepository) {
        this.accountRepository = accountRepository;
    }

    @Transactional(isolation = Isolation.REPEATABLE_READ)
    public void transfer(Long fromId, Long toId, BigDecimal amount) {
        Account from = accountRepository.findByIdWithLock(fromId)
                .orElseThrow(() -> new ResourceNotFoundException("Account", fromId));
        Account to = accountRepository.findByIdWithLock(toId)
                .orElseThrow(() -> new ResourceNotFoundException("Account", toId));

        if (from.getBalance().compareTo(amount) < 0) {
            throw new BusinessRuleViolationException("Insufficient funds");
        }

        from.setBalance(from.getBalance().subtract(amount));
        to.setBalance(to.getBalance().add(amount));

        accountRepository.save(from);
        accountRepository.save(to);
    }
}

public interface AccountRepository extends JpaRepository<Account, Long> {

    @Lock(LockModeType.PESSIMISTIC_WRITE)
    @Query("SELECT a FROM Account a WHERE a.id = :id")
    Optional<Account> findByIdWithLock(@Param("id") Long id);
}

---

Performance

Avoid N+1 queries

// Bad - N+1 problem
List<Order> orders = orderRepository.findAll();
for (Order order : orders) {
    order.getItems().size(); // triggers lazy load for each order
}

// Good - fetch join
@Query("SELECT o FROM Order o JOIN FETCH o.items WHERE o.status = :status")
List<Order> findByStatusWithItems(@Param("status") OrderStatus status);

Use pagination for large datasets

// Bad - loading everything
List<Product> allProducts = productRepository.findAll();

// Good - paginated
Page<Product> page = productRepository.findAll(PageRequest.of(0, 20, Sort.by("name")));

Cache appropriately

@Service
public class ConfigService {

    private final ConfigRepository configRepository;

    public ConfigService(ConfigRepository configRepository) {
        this.configRepository = configRepository;
    }

    @Cacheable(value = "config", key = "#key")
    public String getConfigValue(String key) {
        return configRepository.findByKey(key)
                .map(Config::getValue)
                .orElseThrow(() -> new ResourceNotFoundException("Config", key));
    }

    @CacheEvict(value = "config", key = "#key")
    @Transactional
    public void updateConfig(String key, String value) {
        Config config = configRepository.findByKey(key)
                .orElseThrow(() -> new ResourceNotFoundException("Config", key));
        config.setValue(value);
        configRepository.save(config);
    }
}

Use StringBuilder for string concatenation in loops

// Bad
String result = "";
for (String item : items) {
    result += item + ", ";
}

// Good
String result = String.join(", ", items);

// Or for complex cases
StringBuilder sb = new StringBuilder();
for (int i = 0; i < items.size(); i++) {
    if (i > 0) sb.append(", ");
    sb.append(items.get(i).getName())
      .append(": ")
      .append(items.get(i).getValue());
}
String result = sb.toString();

---

Code Review Checklist

• No raw types (use List<String>, not List)
• No unused imports
• No mutable public fields (use getters/setters or records)
• Proper use of access modifiers (prefer private)
• No magic numbers (use named constants)
• Streams used appropriately (not over-used for simple loops)
• Resources closed properly (try-with-resources)
• Exceptions logged with stack traces where appropriate
• Thread safety considered for shared state
• BigDecimal used for monetary values (never double/float)