jyjeanne/code-review

Java Code Review Skill

Perform structured code reviews across five dimensions: Security, Correctness, Performance, Design, and Style. Always review in this priority order.

Review Dimensions (Priority Order)

---

🔴 CRITICAL — Security

Flag these immediately, block merge:

SQL Injection

// ❌ CRITICAL
String query = "SELECT * FROM users WHERE name = '" + name + "'";

// ✅ Safe
@Query("SELECT u FROM User u WHERE u.name = :name")
Optional<User> findByName(@Param("name") String name);

Sensitive Data Exposure

• Passwords, tokens, secrets in logs: log.info("Password: {}", password) ❌
• Sensitive fields in toString() auto-generated by Lombok ❌ (use @ToString.Exclude)
• API keys or credentials hardcoded in source ❌

Missing Authorization Checks

// ❌ Any user can delete any resource
@DeleteMapping("/{id}")
public void delete(@PathVariable Long id) { ... }

// ✅ Check ownership or role
@PreAuthorize("hasRole('ADMIN') or @securityService.isOwner(#id)")
@DeleteMapping("/{id}")
public void delete(@PathVariable Long id) { ... }

Deserialization / Input Trust Issues

• Deserializing user-controlled JSON into entities with sensitive fields
• Missing @Valid on @RequestBody
• Accepting file uploads without type/size validation

---

🔴 CRITICAL — Correctness

NullPointerException Risks

// ❌ Potential NPE
user.getAddress().getCity().toUpperCase();

// ✅ Safe
Optional.ofNullable(user.getAddress())
    .map(Address::getCity)
    .map(String::toUpperCase)
    .orElse("Unknown");

Transaction Boundaries

// ❌ No @Transactional on method that does multiple DB writes
public void transferFunds(Long fromId, Long toId, BigDecimal amount) {
    accountRepo.debit(fromId, amount);
    accountRepo.credit(toId, amount); // if this fails, money is lost
}

// ✅
@Transactional
public void transferFunds(...) { ... }

Entity Mutability in Collections

• Returning mutable internal collections from entities — caller can modify state
• Lazy loading outside transaction scope → LazyInitializationException

Equals/HashCode on JPA Entities

• Never use Lombok @EqualsAndHashCode on @Entity with all fields
• Use only id field for entity equality

---

🟡 IMPORTANT — Performance

N+1 Query Problem

// ❌ N+1: one query per user's orders
users.forEach(u -> System.out.println(u.getOrders().size()));

// ✅ Fetch join
@Query("SELECT u FROM User u LEFT JOIN FETCH u.orders WHERE u.id IN :ids")
List<User> findAllWithOrders(@Param("ids") List<Long> ids);

Missing Pagination

• Any endpoint returning a potentially unbounded list without Pageable ❌

Unnecessary DB Calls

// ❌ Loads full entity just to check existence
if (userRepository.findById(id).isPresent()) { ... }

// ✅
if (userRepository.existsById(id)) { ... }

Stream vs Collection Choice

• Using .collect(Collectors.toList()) then immediately streaming again
• Using parallel streams without understanding thread-safety implications

---

🟡 IMPORTANT — Design

Single Responsibility Violation

• Service class doing HTTP calls, DB access, business logic, AND email sending
• Controller containing business logic

Hardcoded Values

// ❌
if (user.getAge() > 18) { ... }
Thread.sleep(5000);

// ✅
if (user.getAge() > legalAgeRequirement) { ... }
@Value("${app.retry.delay-ms}") private int retryDelayMs;

Magic String / Status Checks

// ❌
if ("ACTIVE".equals(user.getStatus())) { ... }

// ✅
if (UserStatus.ACTIVE == user.getStatus()) { ... }

Tight Coupling

• new ConcreteService() instead of injected interface
• Static utility class calls that can't be mocked in tests

---

🟢 STYLE — Code Quality

Naming

• Method names should be verbs: calculateTotal(), not total()
• Boolean methods: isActive(), hasPermission(), canDelete()
• Avoid abbreviations: usr, svc, mgr

Method Length

• Flag methods > 20 lines for Extract Method refactoring

Commented-Out Code

• Remove all commented-out code before merging (use git history)

Logging Practices

// ❌ String concatenation in log
log.debug("Processing user " + userId + " with status " + status);

// ✅ Parameterized
log.debug("Processing user {} with status {}", userId, status);

Review Output Format

Structure your review as:

## Code Review Summary

### 🔴 Critical Issues (must fix)
- [Issue]: [Explanation] → [Fix]

### 🟡 Important Issues (should fix)
- [Issue]: [Explanation] → [Fix]

### 🟢 Suggestions (nice to have)
- [Suggestion]: [Explanation]

### ✅ What's Done Well
- [Positive observations]

### Overall Assessment
[Production-ready | Needs minor changes | Needs significant work]

Quick Checklist

• [ ] No SQL injection or hardcoded credentials
• [ ] Sensitive fields excluded from logs and toString
• [ ] @Valid on all @RequestBody parameters
• [ ] @Transactional on multi-step write operations
• [ ] No N+1 query patterns
• [ ] Pagination on collection endpoints
• [ ] Custom exceptions with meaningful messages
• [ ] No raw RuntimeException or Exception catches
• [ ] No commented-out code
• [ ] No System.out.println in production code