java-code-review

name	java-code-review
description	Comprehensive Java code quality checklist covering correctness, security, performance, and maintainability

Java Code Review Checklist

1. Correctness

Null Safety

// BAD: potential NPE
String name = user.getAddress().getCity().getName();

// GOOD: use Optional or null checks
String name = Optional.ofNullable(user)
    .map(User::getAddress)
    .map(Address::getCity)
    .map(City::getName)
    .orElse("Unknown");

Equals and HashCode

// BAD: inconsistent equals/hashCode
@Entity
public class Product {
    @Id
    private Long id;
    private String sku;

    @Override
    public boolean equals(Object o) {
        if (this == o) return true;
        if (!(o instanceof Product p)) return false;
        return Objects.equals(id, p.id); // null id before persist
    }
}

// GOOD: use business key
@Entity
public class Product {
    @Id @GeneratedValue
    private Long id;
    @NaturalId
    private String sku;

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

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

Resource Management

// BAD: resource leak
InputStream is = new FileInputStream(file);
// process...

// GOOD: try-with-resources
try (var is = new FileInputStream(file);
     var reader = new BufferedReader(new InputStreamReader(is))) {
    return reader.lines().collect(Collectors.joining("\n"));
}

Collection Safety

// BAD: ConcurrentModificationException risk
for (var item : items) {
    if (item.isExpired()) {
        items.remove(item);
    }
}

// GOOD: use removeIf or stream
items.removeIf(Item::isExpired);

// or collect to new list
var activeItems = items.stream()
    .filter(item -> !item.isExpired())
    .toList();

2. Security

SQL Injection Prevention

// BAD: SQL injection
@Query("SELECT u FROM User u WHERE u.role = '" + role + "'")
List<User> findByRole(String role);

// GOOD: parameterized
@Query("SELECT u FROM User u WHERE u.role = :role")
List<User> findByRole(@Param("role") String role);

Sensitive Data Handling

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

// GOOD: mask or exclude sensitive fields
log.info("User login: email={}", maskEmail(email));

// BAD: returning sensitive fields in DTO
public record UserDto(String id, String email, String passwordHash) {}

// GOOD: exclude sensitive fields
public record UserDto(String id, String email, Instant lastLogin) {}

3. Performance

Stream Efficiency

// BAD: multiple stream passes
long count = items.stream().filter(predicate).count();
List<Item> filtered = items.stream().filter(predicate).toList();

// GOOD: single pass
var filtered = items.stream().filter(predicate).toList();
long count = filtered.size();

String Concatenation

// BAD: string concatenation in loop
String result = "";
for (var item : items) {
    result += item.getName() + ", ";
}

// GOOD: StringBuilder or String.join
String result = items.stream()
    .map(Item::getName)
    .collect(Collectors.joining(", "));

Collection Sizing

// BAD: default capacity with known size
var map = new HashMap<String, User>();
for (var user : users) { map.put(user.getId(), user); }

// GOOD: pre-sized collection
var map = new HashMap<String, User>(users.size() * 4 / 3 + 1);
// or better: use Collectors.toMap
var map = users.stream()
    .collect(Collectors.toMap(User::getId, Function.identity()));

4. Spring-Specific Checks

Dependency Injection

// BAD: field injection
@Service
public class OrderService {
    @Autowired
    private OrderRepository orderRepository;
}

// GOOD: constructor injection
@Service
@RequiredArgsConstructor
public class OrderService {
    private final OrderRepository orderRepository;
}

Transaction Management

// BAD: transaction too broad
@Transactional
public OrderDto createOrderAndNotify(CreateOrderRequest request) {
    var order = processOrder(request);
    sendEmail(order);       // external call inside transaction
    callWebhook(order);     // another external call
    return mapToDto(order);
}

// GOOD: narrow transaction, async for side effects
@Transactional
public OrderDto createOrder(CreateOrderRequest request) {
    var order = processOrder(request);
    eventPublisher.publishEvent(new OrderCreatedEvent(order.getId()));
    return mapToDto(order);
}

@TransactionalEventListener(phase = TransactionPhase.AFTER_COMMIT)
@Async
public void onOrderCreated(OrderCreatedEvent event) {
    sendEmail(event.orderId());
    callWebhook(event.orderId());
}

5. Maintainability

Method Length

• Keep methods under 20 lines
• Extract complex conditions into named methods
• One level of abstraction per method

Naming Conventions

• Classes: PascalCase nouns (OrderService, PaymentGateway)
• Methods: camelCase verbs (findById, calculateTotal)
• Constants: UPPER_SNAKE_CASE (MAX_RETRY_COUNT)
• Boolean methods: is/has/can/should prefix (isValid, hasPermission)

Magic Numbers

// BAD
if (retryCount > 3) { /* ... */ }
Thread.sleep(5000);

// GOOD
private static final int MAX_RETRIES = 3;
private static final Duration RETRY_DELAY = Duration.ofSeconds(5);

if (retryCount > MAX_RETRIES) { /* ... */ }
Thread.sleep(RETRY_DELAY.toMillis());

Review Summary Template

## Summary
[1-2 sentence overview]

## Critical (must fix)
- [ ] Issue description with file:line reference

## Improvements (should fix)
- [ ] Issue description with suggested alternative

## Suggestions (nice to have)
- [ ] Minor improvement or style suggestion

## Positive
- Acknowledge good patterns found