Java Reviewer Skill

Purpose

Reviews Java code for idiomatic patterns, exception handling, resource management, null safety, and modern Java (17+) feature adoption.

When to Use

• Java code review requests

• "Java patterns", "exception handling", "Java best practices" mentions

• Legacy Java modernization

• Projects with pom.xml or build.gradle

• src/main/java/**/*.java files

Project Detection

• pom.xml (Maven) or build.gradle (Gradle)

• src/main/java/ directory structure

• .java source files

• java.version property in build config

Workflow

Step 1: Analyze Project

Java Version: 17 / 21 Build Tool: Maven / Gradle Testing: JUnit 5 Key Dependencies:

• Lombok
• MapStruct
• Jakarta EE

Step 2: Select Review Areas

AskUserQuestion:

"Which Java areas to review?" Options:

• Full Java idiom check (recommended)
• Exception handling
• Resource management (try-with-resources)
• Null safety (Optional, annotations)
• Stream/Collection patterns
• Modern Java features (17+) multiSelect: true

Detection Rules

Critical: Empty Catch Blocks

Pattern Issue Severity

Empty catch block Silent failure CRITICAL

Catch all Exception

Over-broad handling HIGH

Catch Throwable

Catching errors CRITICAL

Missing logging in catch Lost error info HIGH

// BAD: Empty catch block try { parseFile(path); } catch (IOException e) { // Silent failure - CRITICAL }

// BAD: Catch all Exception try { process(); } catch (Exception e) { log.error("Error", e); // Too broad }

// GOOD: Specific exception handling try { parseFile(path); } catch (FileNotFoundException e) { log.warn("File not found: {}", path); return Optional.empty(); } catch (IOException e) { log.error("Failed to read file: {}", path, e); throw new ProcessingException("File read error", e); }

// BAD: Catching Throwable try { compute(); } catch (Throwable t) { // Catches OutOfMemoryError, StackOverflowError log.error("Error", t); }

// GOOD: Catch Exception, let Errors propagate try { compute(); } catch (Exception e) { log.error("Processing failed", e); throw new ServiceException(e); }

Critical: Mutable Static Fields

Pattern Issue Severity

public static non-final Thread-unsafe CRITICAL

Static mutable collection Race condition CRITICAL

Static DateFormat

Not thread-safe CRITICAL

// BAD: Mutable static field public class Config { public static String apiUrl; // CRITICAL: Thread-unsafe public static List<String> allowedHosts = new ArrayList<>(); // CRITICAL }

// GOOD: Immutable static public class Config { public static final String API_URL = "https://api.example.com"; public static final List<String> ALLOWED_HOSTS = List.of("a.com", "b.com"); }

// BAD: Static DateFormat (not thread-safe) public class DateUtils { private static final SimpleDateFormat FORMAT = new SimpleDateFormat("yyyy-MM-dd"); }

// GOOD: ThreadLocal or DateTimeFormatter public class DateUtils { private static final DateTimeFormatter FORMAT = DateTimeFormatter.ofPattern("yyyy-MM-dd"); }

High: Raw Types (No Generics)

Pattern Issue Severity

List instead of List<T>

Type safety lost MEDIUM

Map instead of Map<K, V>

Unchecked operations MEDIUM

Class instead of Class<?>

Wildcard missing LOW

// BAD: Raw types List users = new ArrayList(); users.add("string"); // No compile error, runtime issue User user = (User) users.get(0); // ClassCastException

// GOOD: Generics List<User> users = new ArrayList<>(); users.add(new User("John")); // Type-safe User user = users.get(0); // No cast needed

// BAD: Raw Map Map config = new HashMap(); config.put("key", 123); String value = (String) config.get("key"); // ClassCastException

// GOOD: Typed Map Map<String, Object> config = new HashMap<>(); // Or better: Map<String, String> stringConfig = new HashMap<>();

High: Resource Management

Pattern Issue Severity

Manual close() Leak on exception HIGH

Finally without null check NPE risk MEDIUM

Missing try-with-resources Resource leak HIGH

// BAD: Manual resource management InputStream is = null; try { is = new FileInputStream(file); process(is); } finally { is.close(); // NPE if exception in constructor }

// GOOD: try-with-resources try (InputStream is = new FileInputStream(file)) { process(is); } // Auto-closed, even on exception

// GOOD: Multiple resources try ( Connection conn = dataSource.getConnection(); PreparedStatement stmt = conn.prepareStatement(sql); ResultSet rs = stmt.executeQuery() ) { while (rs.next()) { process(rs); } }

// Java 9+: Effectively final variables InputStream is = new FileInputStream(file); try (is) { // Can use existing variable process(is); }

High: Collection Null Returns

Pattern Issue Severity

Return null for empty NPE risk HIGH

null collection parameter Hidden bug MEDIUM

No defensive copy Mutation leak MEDIUM

// BAD: Return null for empty public List<User> findUsers(String query) { List<User> results = repository.search(query); if (results.isEmpty()) { return null; // Client must null-check } return results; }

// GOOD: Return empty collection public List<User> findUsers(String query) { List<User> results = repository.search(query); return results != null ? results : Collections.emptyList(); }

// BETTER: Use Optional for single values public Optional<User> findUser(String id) { return Optional.ofNullable(repository.findById(id)); }

// BAD: No defensive copy public List<String> getItems() { return items; // Caller can modify }

// GOOD: Defensive copy or unmodifiable public List<String> getItems() { return List.copyOf(items); // Java 10+ // or: return Collections.unmodifiableList(items); }

Medium: String Concatenation in Loops

Pattern Issue Severity

in loop O(n^2) complexity MEDIUM

String.format in loop Performance hit LOW

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

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

// BETTER: String.join (Java 8+) String result = String.join(",", items);

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

Medium: Stream Anti-patterns

Pattern Issue Severity

forEach with side effects Not functional MEDIUM

Nested streams Performance MEDIUM

Stream in loop Create once LOW

Parallel without need Overhead LOW

// BAD: forEach with mutation List<User> activeUsers = new ArrayList<>(); users.stream() .filter(User::isActive) .forEach(activeUsers::add); // Side effect

// GOOD: Collect List<User> activeUsers = users.stream() .filter(User::isActive) .collect(Collectors.toList());

// Java 16+: toList() List<User> activeUsers = users.stream() .filter(User::isActive) .toList();

// BAD: Unnecessary parallel users.parallelStream() // Only 10 users .map(User::getName) .collect(Collectors.toList());

// GOOD: Sequential for small collections users.stream() .map(User::getName) .collect(Collectors.toList());

Low: Missing @Override

Pattern Issue Severity

No @Override on override Typo not caught LOW

Missing equals/hashCode Collection issues MEDIUM

// BAD: Missing @Override public class User { public boolean equals(Object obj) { // Typo not caught // ... } }

// GOOD: With @Override public class User { @Override public boolean equals(Object obj) { if (this == obj) return true; if (obj == null || getClass() != obj.getClass()) return false; User user = (User) obj; return Objects.equals(id, user.id); }

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

}

Modern Java Features (17+)

Old Pattern Modern Alternative Since

instanceof

• cast Pattern matching Java 16

Switch statement Switch expression Java 14

Verbose records record

Java 16

Text blocks (\n ) """

Java 15

Sealed classes sealed , permits

Java 17

// OLD: instanceof + cast if (obj instanceof User) { User user = (User) obj; process(user.getName()); }

// NEW: Pattern matching (Java 16+) if (obj instanceof User user) { process(user.getName()); }

// OLD: Switch statement String status; switch (code) { case 200: status = "OK"; break; case 404: status = "Not Found"; break; default: status = "Unknown"; }

// NEW: Switch expression (Java 14+) String status = switch (code) { case 200 -> "OK"; case 404 -> "Not Found"; default -> "Unknown"; };

// OLD: Verbose DTO public class UserDto { private final String name; private final int age; // constructor, getters, equals, hashCode, toString }

// NEW: Record (Java 16+) public record UserDto(String name, int age) {}

// Text blocks (Java 15+) String json = """ { "name": "John", "age": 30 } """;

Response Template

Java Code Review Results

Project: [name] Java Version: 17 / 21 Build Tool: Maven / Gradle

Exception Handling

CRITICAL

HIGH

Resource Management

Modern Java Adoption

• Use pattern matching instanceof (Java 16+)
• Convert DTOs to records
• Use switch expressions
• Use text blocks for multiline strings

Recommendations

1. Replace empty catch blocks with proper handling
2. Use try-with-resources for all IO
3. Return empty collections instead of null
4. Enable preview features if using Java 21

Positive Patterns

• Good use of Optional for nullable returns
• Proper use of streams for collection processing

Best Practices

• Fail Fast: Validate inputs early

• Immutability: Prefer final fields, records

• Null Safety: Optional, @Nullable/@NonNull

• Modern APIs: java.time, List.of, Map.of

• Checked Exceptions: Use sparingly, wrap in runtime

Integration

• spring-boot-reviewer skill: Spring-specific patterns

• orm-reviewer skill: JPA/Hibernate patterns

• code-reviewer skill: General quality

• security-scanner skill: Security audit

Notes

• Based on Java 17+ best practices

• Encourages adoption of modern Java features

• Compatible with Maven and Gradle projects

• Works with Lombok and other code generators