java-code-review

name	java-code-review
description	Use when performing comprehensive code reviews on Java/Spring Boot code, checking for bugs, performance issues, security vulnerabilities, and adherence to best practices.

Java Code Review Skill

You are an expert Java code reviewer. You systematically evaluate code for correctness, performance, security, and maintainability.

Review Categories

1. Correctness

Check for common bugs:

// BUG: equals/hashCode contract violation
@Entity
public class Product {
    @Id
    @GeneratedValue(strategy = GenerationType.IDENTITY)
    private Long id;

    // WRONG - using mutable generated ID
    @Override
    public boolean equals(Object o) {
        if (this == o) return true;
        if (!(o instanceof Product other)) return false;
        return Objects.equals(id, other.id); // null before persist!
    }

    // CORRECT - use business key
    @Column(nullable = false, unique = true)
    private String sku;

    @Override
    public boolean equals(Object o) {
        if (this == o) return true;
        if (!(o instanceof Product other)) return false;
        return Objects.equals(sku, other.sku);
    }

    @Override
    public int hashCode() {
        return Objects.hash(sku);
    }
}

2. Null Safety

// WRONG - NullPointerException risk
public String getCustomerCity(Order order) {
    return order.getCustomer().getAddress().getCity();
}

// CORRECT - defensive handling
public Optional<String> getCustomerCity(Order order) {
    return Optional.ofNullable(order)
        .map(Order::getCustomer)
        .map(Customer::getAddress)
        .map(Address::getCity);
}

// CORRECT - guard clause when null is invalid
public void processOrder(Order order) {
    Objects.requireNonNull(order, "Order must not be null");
    Objects.requireNonNull(order.getCustomer(), "Customer must not be null");
    // proceed safely
}

3. Resource Management

// WRONG - resource leak
public String readFile(Path path) throws IOException {
    var reader = new BufferedReader(new FileReader(path.toFile()));
    return reader.readLine(); // reader never closed
}

// CORRECT - try-with-resources
public String readFile(Path path) throws IOException {
    try (var reader = Files.newBufferedReader(path)) {
        return reader.readLine();
    }
}

// WRONG - connection leak in JDBC
public List<String> getNames(DataSource ds) throws SQLException {
    var conn = ds.getConnection();
    var stmt = conn.prepareStatement("SELECT name FROM users");
    var rs = stmt.executeQuery();
    // If exception occurs, connection leaks
    var names = new ArrayList<String>();
    while (rs.next()) names.add(rs.getString("name"));
    return names;
}

// CORRECT
public List<String> getNames(DataSource ds) throws SQLException {
    try (var conn = ds.getConnection();
         var stmt = conn.prepareStatement("SELECT name FROM users");
         var rs = stmt.executeQuery()) {
        var names = new ArrayList<String>();
        while (rs.next()) names.add(rs.getString("name"));
        return names;
    }
}

4. Thread Safety

// WRONG - shared mutable state in singleton Spring bean
@Service
public class CounterService {
    private int count = 0; // Not thread-safe

    public int increment() {
        return ++count; // Race condition
    }
}

// CORRECT
@Service
public class CounterService {
    private final AtomicInteger count = new AtomicInteger(0);

    public int increment() {
        return count.incrementAndGet();
    }
}

// WRONG - mutable collection in singleton
@Service
public class CacheService {
    private final Map<String, Object> cache = new HashMap<>(); // Not thread-safe

    public void put(String key, Object value) {
        cache.put(key, value);
    }
}

// CORRECT
@Service
public class CacheService {
    private final ConcurrentMap<String, Object> cache = new ConcurrentHashMap<>();

    public void put(String key, Object value) {
        cache.put(key, value);
    }
}

5. Performance

// WRONG - N+1 query problem
@Transactional(readOnly = true)
public List<OrderResponse> getAllOrders() {
    var orders = orderRepository.findAll(); // 1 query
    return orders.stream()
        .map(order -> new OrderResponse(
            order.getId(),
            order.getItems().size()))  // N queries for items!
        .toList();
}

// CORRECT - fetch join
@Query("SELECT o FROM Order o JOIN FETCH o.items")
List<Order> findAllWithItems();

// WRONG - string concatenation in loop
public String buildReport(List<String> lines) {
    String result = "";
    for (var line : lines) {
        result += line + "\n"; // O(n^2) - creates new String each iteration
    }
    return result;
}

// CORRECT
public String buildReport(List<String> lines) {
    return String.join("\n", lines);
}

6. Security

// WRONG - SQL injection
@Query(value = "SELECT * FROM users WHERE name = '" + name + "'", nativeQuery = true)
List<User> findByName(String name);

// CORRECT - parameterized
@Query(value = "SELECT * FROM users WHERE name = :name", nativeQuery = true)
List<User> findByName(@Param("name") String name);

// WRONG - logging sensitive data
log.info("User login: email={}, password={}", email, password);

// CORRECT
log.info("User login attempt: email={}", email);

// WRONG - exposing stack traces to clients
@ExceptionHandler(Exception.class)
public ResponseEntity<String> handleError(Exception ex) {
    return ResponseEntity.status(500).body(ex.toString()); // Leaks internals
}

// CORRECT
@ExceptionHandler(Exception.class)
public ResponseEntity<ApiError> handleError(Exception ex) {
    log.error("Unexpected error", ex); // Log full trace internally
    return ResponseEntity.status(500).body(
        new ApiError(Instant.now(), 500, "Internal Server Error",
            "An unexpected error occurred", "", List.of()));
}

7. Exception Handling

// WRONG - catching generic Exception
try {
    processPayment(order);
} catch (Exception e) {
    log.error("Error", e); // Too broad
}

// CORRECT - specific exceptions
try {
    processPayment(order);
} catch (InsufficientFundsException e) {
    log.warn("Insufficient funds for order: {}", order.getId());
    throw new PaymentDeclinedException("Insufficient funds", e);
} catch (PaymentGatewayException e) {
    log.error("Payment gateway error for order: {}", order.getId(), e);
    throw new ServiceUnavailableException("Payment service temporarily unavailable", e);
}

// WRONG - empty catch block
try {
    sendNotification(order);
} catch (NotificationException e) {
    // silently swallowed
}

// CORRECT
try {
    sendNotification(order);
} catch (NotificationException e) {
    log.warn("Failed to send notification for order: {}, will retry", order.getId(), e);
    retryQueue.enqueue(new NotificationRetry(order.getId()));
}

Review Checklist

1. Correctness: Logic errors, off-by-one, null derefs, equals/hashCode
2. Null Safety: Proper use of Optional, null checks, @NonNull
3. Resources: try-with-resources, connection leaks, stream closing
4. Thread Safety: Mutable state in singletons, concurrent collections
5. Performance: N+1 queries, string concatenation, unnecessary allocations
6. Security: Injection, sensitive data logging, stack trace exposure
7. Exceptions: Specific catches, no empty catch, proper wrapping
8. Naming: Clear, consistent, following conventions
9. Testing: Sufficient coverage, edge cases tested
10. Documentation: Public API Javadoc, complex logic explained