java-code-review

name	java-code-review
description	Systematic code review for Java with null safety, exception handling, concurrency, and performance checks. Use when user says "review code", "check this PR", "code review", or before merging changes.

Java Code Review Skill

Review Strategy

1. Scan - Read through code for overall understanding
2. Check - Apply checklist systematically
3. Report - Provide actionable feedback with severity levels

Output Format

### Code Review: [File/Component Name]

**Overall**: [Brief assessment]

#### Issues Found

1. **[CRITICAL]** [Title]
   - File: `path/to/File.java:line`
   - Issue: [Description]
   - Fix: [Suggested code or approach]

2. **[WARNING]** [Title]
   - File: `path/to/File.java:line`
   - Issue: [Description]
   - Suggestion: [Improvement]

3. **[INFO]** [Title]
   - Note: [Optional improvement]

#### Summary
- Critical: X | Warnings: Y | Info: Z
- Ready to merge: Yes/No

---

Review Checklist

1. Null Safety

// BAD: NullPointerException waiting to happen
public String getUserCity(Long userId) {
    User user = userRepository.findById(userId).get(); // NoSuchElement if absent
    return user.getAddress().getCity(); // NPE if address is null
}

// GOOD: Proper null handling
public String getUserCity(Long userId) {
    return userRepository.findById(userId)
        .map(User::getAddress)
        .map(Address::getCity)
        .orElse("Unknown");
}

Flags:

• .get() on Optional without .isPresent() check
• Method chains without null checks (train wreck)
• @Nullable parameters without validation
• Returning null from methods (prefer Optional or empty collection)

Suggest:

• Use Optional for return types that may be absent
• Use Objects.requireNonNull() for required parameters
• Use @NonNull / @Nullable annotations
• Return empty collections instead of null

// BAD
public List<User> findUsers(String name) {
    if (name == null) return null;  // Caller must null-check
    // ...
}

// GOOD
public List<User> findUsers(String name) {
    if (name == null) return Collections.emptyList();
    // ...
}

---

2. Exception Handling

// BAD: Swallowing exceptions
try {
    processPayment(order);
} catch (Exception e) {
    log.error("Error");  // Lost: exception type, message, stack trace
}

// BAD: Catching too broadly
try {
    int value = Integer.parseInt(input);
    return userRepository.findById(value);
} catch (Exception e) {  // Catches NumberFormatException AND DataAccessException
    return null;
}

// GOOD: Specific catch, preserve context
try {
    processPayment(order);
} catch (PaymentDeclinedException e) {
    log.warn("Payment declined for order {}: {}", order.getId(), e.getMessage());
    throw new OrderProcessingException("Payment failed for order " + order.getId(), e);
} catch (PaymentGatewayException e) {
    log.error("Payment gateway error for order {}", order.getId(), e);
    throw new ServiceUnavailableException("Payment service unavailable", e);
}

Flags:

• Empty catch blocks
• catch (Exception e) - too broad
• e.printStackTrace() in production code
• Throwing new Exception() or new RuntimeException() (use specific types)
• Losing the original exception (not passing cause)

Suggest:

• Create domain-specific exceptions
• Log at the appropriate level
• Always include the cause exception
• Use @RestControllerAdvice for REST API error handling

---

3. Collections & Streams

// BAD: Modifying collection while iterating
for (User user : users) {
    if (user.isInactive()) {
        users.remove(user); // ConcurrentModificationException
    }
}

// GOOD: Use removeIf or stream filter
users.removeIf(User::isInactive);
// OR
List<User> activeUsers = users.stream()
    .filter(User::isActive)
    .toList();

// BAD: Stream abuse - harder to read than a loop
Map<String, List<Map.Entry<String, Long>>> result = orders.stream()
    .flatMap(o -> o.getItems().stream())
    .collect(Collectors.groupingBy(
        Item::getCategory,
        Collectors.collectingAndThen(
            Collectors.toList(),
            items -> items.stream()
                .collect(Collectors.groupingBy(
                    Item::getName,
                    Collectors.counting()))
                .entrySet().stream()
                .sorted(Map.Entry.<String, Long>comparingByValue().reversed())
                .toList())));

// GOOD: Break complex streams into steps or use loops
Map<String, Map<String, Long>> itemCountsByCategory = orders.stream()
    .flatMap(order -> order.getItems().stream())
    .collect(Collectors.groupingBy(
        Item::getCategory,
        Collectors.groupingBy(Item::getName, Collectors.counting())
    ));

Flags:

• Modifying collections during iteration
• Using Arrays.asList() and then trying to add/remove (it's fixed-size)
• Complex nested stream operations (> 3 levels)
• Using forEach with side effects instead of collect

Suggest:

• Use List.of(), Map.of() for immutable collections
• Use toList() (Java 16+) instead of Collectors.toList()
• Break complex streams into named intermediate variables

---

4. Concurrency

// BAD: Non-thread-safe singleton with lazy init
public class ConnectionPool {
    private static ConnectionPool instance;

    public static ConnectionPool getInstance() {
        if (instance == null) {  // Race condition
            instance = new ConnectionPool();
        }
        return instance;
    }
}

// BAD: Shared mutable state without synchronization
@Service
public class CounterService {
    private int count = 0;  // Not thread-safe

    public void increment() {
        count++;  // Read-modify-write - not atomic
    }
}

// GOOD: Use atomic types or proper synchronization
@Service
public class CounterService {
    private final AtomicInteger count = new AtomicInteger(0);

    public void increment() {
        count.incrementAndGet();
    }
}

Flags:

• SimpleDateFormat shared between threads (not thread-safe)
• Shared mutable state without synchronization
• HashMap in concurrent context (use ConcurrentHashMap)
• Missing volatile on double-checked locking
• Synchronizing on non-final fields

Suggest:

• Use java.time instead of Date/SimpleDateFormat
• Prefer AtomicInteger, AtomicReference for simple cases
• Use ConcurrentHashMap for concurrent maps
• Consider @Async with Spring for async processing

---

5. Java Idioms

equals / hashCode / toString

// BAD: Missing equals/hashCode on entity used in Set/Map
public class Product {
    private Long id;
    private String name;
    // No equals/hashCode - Set<Product> won't work correctly
}

// GOOD: Implement equals/hashCode based on business key
public class Product {
    private Long id;
    private String name;

    @Override
    public boolean equals(Object o) {
        if (this == o) return true;
        if (o == null || getClass() != o.getClass()) return false;
        Product product = (Product) o;
        return Objects.equals(id, product.id);
    }

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

    @Override
    public String toString() {
        return "Product{id=%d, name='%s'}".formatted(id, name);
    }
}

// BEST: Use records for DTOs/value objects (auto equals/hashCode/toString)
public record ProductDto(Long id, String name, BigDecimal price) {}

Flags:

• Override equals without hashCode (or vice versa)
• Using == for String/Object comparison
• toString() that includes sensitive data (passwords, tokens)
• Missing toString() on classes used in logging

---

6. Resource Management

// BAD: Resource leak
public String readFile(String path) throws IOException {
    FileReader reader = new FileReader(path);
    BufferedReader br = new BufferedReader(reader);
    String content = br.readLine(); // If this throws, reader is never closed
    br.close();
    return content;
}

// GOOD: Try-with-resources
public String readFile(String path) throws IOException {
    try (BufferedReader br = new BufferedReader(new FileReader(path))) {
        return br.readLine();
    }
}

// GOOD: Multiple resources
public void copyData(String source, String target) throws IOException {
    try (InputStream in = new FileInputStream(source);
         OutputStream out = new FileOutputStream(target)) {
        in.transferTo(out);
    }
}

Flags:

• Manual close() calls instead of try-with-resources
• InputStream, Connection, ResultSet not in try-with-resources
• finally blocks for resource cleanup (use try-with-resources)

---

7. API Design

// BAD: Method returns different types based on conditions
public Object getResult(boolean detailed) {
    if (detailed) return new DetailedResult();
    else return new SummaryResult();
}

// GOOD: Use generics or separate methods
public DetailedResult getDetailedResult() { ... }
public SummaryResult getSummaryResult() { ... }

// BAD: Boolean parameters
public void sendNotification(User user, boolean urgent, boolean withAttachment) { ... }

// GOOD: Use enums or parameter objects
public void sendNotification(User user, NotificationOptions options) { ... }
public record NotificationOptions(Priority priority, boolean includeAttachment) {}

Flags:

• Methods with boolean parameters
• Returning Object type
• Public methods without Javadoc on library/API code
• Methods with more than 3 parameters

---

8. Performance Considerations

// BAD: String concatenation in loop
String result = "";
for (String item : items) {
    result += item + ", ";  // Creates new String object each iteration
}

// GOOD: StringBuilder
StringBuilder result = new StringBuilder();
for (String item : items) {
    result.append(item).append(", ");
}

// BEST: String.join or Collectors.joining
String result = String.join(", ", items);
String result = items.stream().collect(Collectors.joining(", "));

// BAD: N+1 query in loop
List<Order> orders = orderRepository.findAll();
for (Order order : orders) {
    Customer customer = customerRepository.findById(order.getCustomerId()); // N queries!
    // ...
}

// GOOD: Batch fetch
List<Order> orders = orderRepository.findAll();
Set<Long> customerIds = orders.stream()
    .map(Order::getCustomerId)
    .collect(Collectors.toSet());
Map<Long, Customer> customers = customerRepository.findAllById(customerIds).stream()
    .collect(Collectors.toMap(Customer::getId, Function.identity()));

Flags:

• String concatenation in loops
• N+1 query patterns
• LinkedList used where ArrayList is better (almost always)
• Boxing/unboxing in tight loops
• Creating objects in loops that could be reused

---

9. Testing Hints

Flags:

• No tests for new functionality
• Tests that depend on execution order
• Tests with no assertions
• Tests that test implementation details instead of behavior
• @SpringBootTest for unit tests (use @ExtendWith(MockitoExtension.class))
• Hard-coded test data that's fragile

Suggest:

• Follow Arrange-Act-Assert pattern
• Test behavior, not implementation
• Use @Nested for grouping related tests
• Use parameterized tests for multiple inputs

---

Severity Guidelines

Level	Meaning	Action
CRITICAL	Bug, security issue, data loss risk	Must fix before merge
WARNING	Code smell, potential issue, bad practice	Should fix, discuss if blocking
INFO	Style, minor improvement, suggestion	Nice to have, non-blocking

---

Token Optimization Tips

When reviewing large PRs:

1. Focus on new and modified files, not generated code
2. Skip reviewing auto-generated code (Lombok getters, MapStruct implementations)
3. Prioritize: Security > Correctness > Performance > Style
4. For large refactors, review the pattern, then spot-check individual files

---

Quick Reference Card

Check	What to Look For
Nulls	.get() on Optional, null returns, missing null checks
Exceptions	Empty catch, broad catch, swallowed exceptions
Resources	Missing try-with-resources, unclosed streams
Threading	Shared mutable state, non-atomic operations
Collections	Mutation during iteration, wrong collection type
Strings	Concatenation in loops, == comparison
APIs	Boolean params, > 3 params, inconsistent returns
Tests	Missing tests, no assertions, Spring context abuse
Security	SQL injection, exposed secrets, missing validation
Performance	N+1 queries, unnecessary object creation