Java Code Review Skill Systematic code review checklist for Java projects. When to Use User says "review this code" / "check this PR" / "code review" Before merging a PR After implementing a feature Review Strategy Quick scan - Understand intent, identify scope Checklist pass - Go through each category below Summary - List findings by severity (Critical → Minor) Output Format

Code Review: [file/feature name]

Critical

[Issue description + line reference + suggestion]

Improvements

[Suggestion + rationale]

Minor/Style

[Nitpicks, optional improvements]

Good Practices Observed

[Positive feedback - important for morale] Review Checklist 1. Null Safety Check for: // ❌ NPE risk String name = user . getName ( ) . toUpperCase ( ) ; // ✅ Safe String name = Optional . ofNullable ( user . getName ( ) ) . map ( String :: toUpperCase ) . orElse ( "" ) ; // ✅ Also safe (early return) if ( user . getName ( ) == null ) { return "" ; } return user . getName ( ) . toUpperCase ( ) ; Flags: Chained method calls without null checks Missing @Nullable / @NonNull annotations on public APIs Optional.get() without isPresent() check Returning null from methods that could return Optional or empty collection Suggest: Use Optional for return types that may be absent Use Objects.requireNonNull() for constructor/method params Return empty collections instead of null: Collections.emptyList() 2. Exception Handling Check for: // ❌ Swallowing exceptions try { process ( ) ; } catch ( Exception e ) { // silently ignored } // ❌ Catching too broad catch ( Exception e ) { } catch ( Throwable t ) { } // ❌ Losing stack trace catch ( IOException e ) { throw new RuntimeException ( e . getMessage ( ) ) ; } // ✅ Proper handling catch ( IOException e ) { log . error ( "Failed to process file: {}" , filename , e ) ; throw new ProcessingException ( "File processing failed" , e ) ; } Flags: Empty catch blocks Catching Exception or Throwable broadly Losing original exception (not chaining) Using exceptions for flow control Checked exceptions leaking through API boundaries Suggest: Log with context AND stack trace Use specific exception types Chain exceptions with cause Consider custom exceptions for domain errors 3. Collections & Streams Check for: // ❌ Modifying while iterating for ( Item item : items ) { if ( item . isExpired ( ) ) { items . remove ( item ) ; // ConcurrentModificationException } } // ✅ Use removeIf items . removeIf ( Item :: isExpired ) ; // ❌ Stream for simple operations list . stream ( ) . forEach ( System . out :: println ) ; // ✅ Simple loop is cleaner for ( Item item : list ) { System . out . println ( item ) ; } // ❌ Collecting to modify List < String

names

users . stream ( ) . map ( User :: getName ) . collect ( Collectors . toList ( ) ) ; names . add ( "extra" ) ; // Might be immutable! // ✅ Explicit mutable list List < String

names

users . stream ( ) . map ( User :: getName ) . collect ( Collectors . toCollection ( ArrayList :: new ) ) ; Flags: Modifying collections during iteration Overusing streams for simple operations Assuming Collectors.toList() returns mutable list Not using List.of() , Set.of() , Map.of() for immutable collections Parallel streams without understanding implications Suggest: List.copyOf() for defensive copies removeIf() instead of iterator removal Streams for transformations, loops for side effects 4. Concurrency Check for: // ❌ Not thread-safe private Map < String , User

cache

new HashMap <

( ) ; // ✅ Thread-safe private Map < String , User

cache

new ConcurrentHashMap <

( ) ; // ❌ Check-then-act race condition if ( ! map . containsKey ( key ) ) { map . put ( key , computeValue ( ) ) ; } // ✅ Atomic operation map . computeIfAbsent ( key , k -> computeValue ( ) ) ; // ❌ Double-checked locking (broken without volatile) if ( instance == null ) { synchronized ( this ) { if ( instance == null ) { instance = new Instance ( ) ; } } } Flags: Shared mutable state without synchronization Check-then-act patterns without atomicity Missing volatile on shared variables Synchronized on non-final objects Thread-unsafe lazy initialization Suggest: Prefer immutable objects Use java.util.concurrent classes AtomicReference , AtomicInteger for simple cases Consider @ThreadSafe / @NotThreadSafe annotations 5. Java Idioms equals/hashCode: // ❌ Only equals without hashCode @Override public boolean equals ( Object o ) { . . . } // Missing hashCode! // ❌ Mutable fields in hashCode @Override public int hashCode ( ) { return Objects . hash ( id , mutableField ) ; // Breaks HashMap } // ✅ Use immutable fields, implement both @Override public boolean equals ( Object o ) { if ( this == o ) return true ; if ( ! ( o instanceof User user ) ) return false ; return Objects . equals ( id , user . id ) ; } @Override public int hashCode ( ) { return Objects . hash ( id ) ; } toString: // ❌ Missing - hard to debug // No toString() // ❌ Including sensitive data return "User{password='" + password + "'}" ; // ✅ Useful for debugging @Override public String toString ( ) { return "User{id=" + id + ", name='" + name + "'}" ; } Builders: // ✅ For classes with many optional parameters User user = User . builder ( ) . name ( "John" ) . email ( "john@example.com" ) . build ( ) ; Flags: equals without hashCode Mutable fields in hashCode Missing toString on domain objects Constructors with > 3-4 parameters (suggest builder) Not using instanceof pattern matching (Java 16+) 6. Resource Management Check for: // ❌ Resource leak FileInputStream fis = new FileInputStream ( file ) ; // ... might throw before close // ✅ Try-with-resources try ( FileInputStream fis = new FileInputStream ( file ) ) { // ... } // ❌ Multiple resources, wrong order try ( BufferedWriter writer = new BufferedWriter ( new FileWriter ( file ) ) ) { // FileWriter might not be closed if BufferedWriter fails } // ✅ Separate declarations try ( FileWriter fw = new FileWriter ( file ) ; BufferedWriter writer = new BufferedWriter ( fw ) ) { // Both properly closed } Flags: Not using try-with-resources for Closeable / AutoCloseable Resources opened but not in try-with-resources Database connections/statements not properly closed 7. API Design Check for: // ❌ Boolean parameters process ( data , true , false ) ; // What do these mean? // ✅ Use enums or builder process ( data , ProcessMode . ASYNC , ErrorHandling . STRICT ) ; // ❌ Returning null for "not found" public User findById ( Long id ) { return users . get ( id ) ; // null if not found } // ✅ Return Optional public Optional < User

findById ( Long id ) { return Optional . ofNullable ( users . get ( id ) ) ; } // ❌ Accepting null collections public void process ( List < Item

items ) { if ( items == null ) items = Collections . emptyList ( ) ; } // ✅ Require non-null, accept empty public void process ( List < Item

items ) { Objects . requireNonNull ( items , "items must not be null" ) ; } Flags: Boolean parameters (prefer enums) Methods with > 3 parameters (consider parameter object) Inconsistent null handling across similar methods Missing validation on public API inputs 8. Performance Considerations Check for: // ❌ String concatenation in loop String result = "" ; for ( String s : strings ) { result += s ; // Creates new String each iteration } // ✅ StringBuilder StringBuilder sb = new StringBuilder ( ) ; for ( String s : strings ) { sb . append ( s ) ; } // ❌ Regex compilation in loop for ( String line : lines ) { if ( line . matches ( "pattern." ) ) { } // Compiles regex each time } // ✅ Pre-compiled pattern private static final Pattern PATTERN = Pattern . compile ( "pattern." ) ; for ( String line : lines ) { if ( PATTERN . matcher ( line ) . matches ( ) ) { } } // ❌ N+1 in loops for ( User user : users ) { List < Order

orders

orderRepo . findByUserId ( user . getId ( ) ) ; } // ✅ Batch fetch Map < Long , List < Order

ordersByUser

orderRepo . findByUserIds ( userIds ) ; Flags: String concatenation in loops Regex compilation in loops N+1 query patterns Creating objects in tight loops that could be reused Not using primitive streams ( IntStream , LongStream ) 9. Testing Hints Suggest tests for: Null inputs Empty collections Boundary values Exception cases Concurrent access (if applicable) Severity Guidelines Severity Criteria Critical Security vulnerability, data loss risk, production crash High Bug likely, significant performance issue, breaks API contract Medium Code smell, maintainability issue, missing best practice Low Style, minor optimization, suggestion Token Optimization Focus on changed lines (use git diff ) Don't repeat obvious issues - group similar findings Reference line numbers, not full code quotes Skip files that are auto-generated or test fixtures Quick Reference Card Category Key Checks Null Safety Chained calls, Optional misuse, null returns Exceptions Empty catch, broad catch, lost stack trace Collections Modification during iteration, stream vs loop Concurrency Shared mutable state, check-then-act Idioms equals/hashCode pair, toString, builders Resources try-with-resources, connection leaks API Boolean params, null handling, validation Performance String concat, regex in loop, N+1