Code Review Skill

You now have expertise in conducting comprehensive code reviews. Follow this structured approach:

Review Checklist

1. Security (Critical)

Check for:

• Injection vulnerabilities: SQL injection, command injection, XSS, LDAP injection
• Authentication issues: Hardcoded credentials, weak authentication mechanisms
• Authorization flaws: Missing access controls, IDOR, privilege escalation
• Data exposure: Sensitive data in logs, error messages, or stack traces
• Cryptography: Weak algorithms, improper key management, plaintext secrets
• Dependencies: Known vulnerabilities in third-party libraries

# Quick security scans for Java projects
mvn dependency-check:check          # OWASP dependency check
./gradlew dependencyCheckAnalyze    # Gradle equivalent
grep -r "password\|secret\|api_key\|apiKey" --include="*.java" --include="*.yml" --include="*.properties"

2. Correctness

Check for:

• Logic errors: Off-by-one, null handling, edge cases
• Race conditions: Concurrent access without synchronization, shared mutable state
• Resource leaks: Unclosed streams, connections, or resources (missing try-with-resources)
• Error handling: Swallowed exceptions, overly broad catch blocks, missing error paths
• Type safety: Unchecked casts, raw types, missing generics

3. Performance

Check for:

• N+1 queries: Database/API calls in loops (common with JPA/Hibernate)
• Memory issues: Large object allocations, retained references, improper caching
• Blocking operations: Blocking I/O in reactive/async code paths
• Inefficient algorithms: O(n^2) when O(n) is possible, unnecessary object creation
• Missing caching: Repeated expensive computations or database lookups
• String concatenation: Using + in loops instead of StringBuilder

4. Maintainability

Check for:

• Naming: Clear, consistent, descriptive (follows Java conventions)
• Complexity: Methods > 50 lines, deep nesting > 3 levels, cyclomatic complexity
• Duplication: Copy-pasted code blocks violating DRY
• Dead code: Unused imports, unreachable branches, commented-out code
• Comments: Outdated Javadoc, redundant comments, or missing documentation where needed
• SOLID principles: Single responsibility, proper abstractions, dependency injection

5. Testing

Check for:

• Coverage: Critical paths and business logic tested
• Edge cases: Null, empty, boundary values, error conditions
• Mocking: External dependencies properly isolated (Mockito, WireMock)
• Assertions: Meaningful, specific checks (not just assertNotNull)
• Test naming: Descriptive names following conventions (e.g., shouldReturnEmpty_whenInputIsNull)

Review Output Format

## Code Review: [file/component name]

### Summary
[1-2 sentence overview]

### Critical Issues
1. **[Issue]** (line X): [Description]
   - Impact: [What could go wrong]
   - Fix: [Suggested solution]

### Improvements
1. **[Suggestion]** (line X): [Description]

### Positive Notes
- [What was done well]

### Verdict
[ ] Ready to merge
[ ] Needs minor changes
[ ] Needs major revision

Common Patterns to Flag

Java

// Bad: SQL injection
String query = "SELECT * FROM users WHERE id = " + userId;
stmt.executeQuery(query);
// Good: Use parameterized queries
PreparedStatement ps = conn.prepareStatement("SELECT * FROM users WHERE id = ?");
ps.setString(1, userId);

// Bad: Resource leak
InputStream is = new FileInputStream("file.txt");
// processing without close
// Good: try-with-resources
try (InputStream is = new FileInputStream("file.txt")) {
    // processing
}

// Bad: Swallowed exception
try { riskyOperation(); } catch (Exception e) { }
// Good: Log or rethrow
try { riskyOperation(); } catch (Exception e) {
    log.error("Operation failed", e);
    throw new ServiceException("Operation failed", e);
}

// Bad: Mutable shared state without synchronization
private List<String> cache = new ArrayList<>();
// Good: Use concurrent collections or synchronization
private List<String> cache = new CopyOnWriteArrayList<>();

// Bad: N+1 query in JPA
List<Order> orders = orderRepo.findAll();
orders.forEach(o -> o.getItems().size()); // triggers lazy load per order
// Good: Use fetch join
@Query("SELECT o FROM Order o JOIN FETCH o.items")
List<Order> findAllWithItems();

Spring Boot

// Bad: Hardcoded secrets
@Value("sk-abc123xyz")
private String apiKey;
// Good: Use externalized configuration
@Value("${api.key}")
private String apiKey;

// Bad: Missing validation
@PostMapping("/users")
public User create(@RequestBody User user) { ... }
// Good: Validate input
@PostMapping("/users")
public User create(@Valid @RequestBody User user) { ... }

Review Commands

# Show recent changes
git diff HEAD~5 --stat
git log --oneline -10

# Find potential issues
grep -rn "TODO\|FIXME\|HACK\|XXX" --include="*.java" .
grep -rn "password\|secret\|token" --include="*.java" --include="*.properties" --include="*.yml" .

# Check for common issues
mvn spotbugs:check           # Static analysis
mvn checkstyle:check         # Code style
./gradlew spotbugsMain       # Gradle equivalent

# Run tests with coverage
mvn test jacoco:report
./gradlew test jacocoTestReport

# Check dependencies
mvn versions:display-dependency-updates
./gradlew dependencyUpdates

Review Workflow

1. Understand context: Read PR description, linked issues, and acceptance criteria
2. Run the code: Build, run tests, start locally if possible (mvn spring-boot:run)
3. Read top-down: Start with controllers/entry points, follow the call chain
4. Check tests: Are changes tested? Do tests pass? Is coverage adequate?
5. Security scan: Run SpotBugs, OWASP dependency check
6. Manual review: Use checklist above, focus on business logic correctness
7. Write feedback: Be specific, suggest fixes, be kind and constructive