Java Code Review Checklist: Best Practices and Common Pitfalls

NullPointerExceptions crash production despite passing tests. Thread-unsafe code works fine in development but corrupts data under load. Resources leak because try-with-resources wasn't used. These issues compile successfully and often slip through code review because they hide in familiar patterns.

This checklist helps catch what matters: thread safety issues, resource leaks, improper exception handling, and code that fights modern Java features instead of using them effectively.

Quick Reference Checklist

Use this checklist as a quick reference during code review. Each item links to detailed explanations below.

Resource Management

• Resources closed with try-with-resources: try (var stream = Files.lines(path))
• No manual close() calls in finally blocks
• Streams properly closed, especially for file operations
• Database connections, statements, and result sets in try-with-resources
• Thread pools shut down properly: executor.shutdown()
• No suppressed exceptions from automatic resource closing
• Resources not assigned to fields without careful lifecycle management

Thread Safety & Concurrency

• Shared mutable state protected by synchronization
• Volatile used for simple flags, not complex operations
• Double-checked locking avoided or implemented correctly with volatile
• Collections wrapped with Collections.synchronized*() if shared
• ConcurrentHashMap used instead of Hashtable or synchronized HashMap
• Thread-safe classes from java.util.concurrent preferred
• Immutable objects used for sharing state between threads
• No publishing partially constructed objects from constructors

Null Safety

• Optional used for return values that may be absent, not parameters
• Optional.of() only for known non-null values
• Optional.orElseThrow() preferred over get()
• Null checks before dereferencing: if (obj != null)
• @NonNull and @Nullable annotations document null contracts
• Objects class methods used for null-safe operations: Objects.requireNonNull()
• No returning null from collections (return empty collections instead)

Modern Java Features

• Streams used for collection processing instead of explicit loops (when clear)
• Stream operations don't have side effects
• Records used for simple data carriers: record User(String name, int age)
• Sealed classes restrict inheritance when appropriate (Java 17+)
• Pattern matching for instanceof eliminates casts (Java 16+)
• Text blocks used for multi-line strings (Java 15+)
• Local variable type inference var improves readability without hiding types
• Switch expressions return values instead of using fall-through (Java 14+)

Exception Handling

• Checked exceptions documented in javadoc with @throws
• Exceptions not swallowed with empty catch blocks
• Specific exceptions caught, not generic Exception unless necessary
• Custom exceptions extend appropriate base class (Exception or RuntimeException)
• Exception messages provide context: what failed and why
• Resources cleaned up even when exceptions occur
• Stack traces not used for control flow
• Exceptions not thrown in finally blocks (masks original exception)

Collections & Generics

• Generic types specified, not raw types: List<String> not List
• Diamond operator used: new ArrayList<>() not new ArrayList<String>()
• Appropriate collection type for use case (List vs Set vs Map)
• Unmodifiable collections created with List.of(), Set.of(), Map.of()
• Collections sized appropriately when size known: new ArrayList<>(expectedSize)
• No unnecessary boxing/unboxing in loops
• forEach used only for side effects, not transformations
• Stream collectors used appropriately: Collectors.toList(), groupingBy()

Code Organization

• Classes have single responsibility
• Methods short and focused (prefer <20 lines)
• Public API minimal (prefer package-private when possible)
• Utility classes have private constructor
• Builder pattern for objects with many parameters
• Immutability preferred: fields final when possible
• No public fields (use getters/setters or records)
• Package names lowercase, descriptive: com.example.user

Resource Management in Java

Java's try-with-resources statement, introduced in Java 7, provides the standard pattern for managing resources that need cleanup. When we see code opening files, database connections, or streams, we verify it uses try-with-resources. The pattern try (var stream = Files.lines(path)) automatically closes the stream when the block exits, whether through normal completion, exception, or early return. This eliminates the common mistake of forgetting to close resources or closing them incorrectly.

Manual resource management with finally blocks creates opportunities for errors. The pattern of calling close() in a finally block seems correct but has problems. If the try block throws an exception and the finally block also throws while closing, the original exception is lost. If you forget the null check in finally before calling close(), you might throw NullPointerException that masks the real problem. Try-with-resources handles these edge cases correctly.

Multiple resources in a single try-with-resources statement get closed in reverse order of declaration. The pattern try (var conn = getConnection(); var stmt = conn.prepareStatement(sql)) closes the statement before the connection, which is correct. When resources have dependencies, declaration order matters. We verify that dependent resources are declared in the right order so cleanup happens without errors.

Streams require special attention because they hold system resources. A Files.lines() call opens a file handle that stays open until the stream closes. Code that creates streams must ensure they close, typically with try-with-resources. When streams are returned from methods, the caller becomes responsible for closing them, which should be documented. Better design often processes streams within the creating method and returns results rather than exposing stream management to callers.

Thread pools and executors need explicit shutdown because they hold threads that prevent JVM exit. When we see Executors.newFixedThreadPool() or similar, we verify the code calls shutdown() or shutdownNow() appropriately, typically in try-finally or try-with-resources (ExecutorService implements AutoCloseable). Failure to shut down thread pools causes applications to hang at exit.

Thread Safety and Concurrent Code

Shared mutable state creates data races when accessed from multiple threads without synchronization. When reviewing code that might run concurrently, we identify which fields multiple threads might access. For each shared field, we verify synchronization protects all accesses—both reads and writes. The synchronized keyword on methods or blocks provides mutual exclusion, ensuring only one thread executes the protected code at a time.

Volatile variables provide visibility guarantees without locking. When one thread writes a volatile field and another reads it, the reader sees the latest value. This works for simple flags or references but doesn't provide atomicity for compound operations. Code like if (!initialized) { initialized = true; } isn't thread-safe even with volatile because the check-then-set isn't atomic. For these cases, we need proper synchronization or atomic classes.

Double-checked locking appears in lazy initialization code but requires careful implementation. The classic pattern of checking if a field is null, synchronizing if it is, then checking again doesn't work without volatile on the field. The pattern requires both the volatile keyword and proper memory barriers. Modern Java provides better alternatives: eager initialization, holder class pattern, or Lazy from utility libraries. When we see double-checked locking, we verify it's implemented correctly or suggest alternatives.

Collections from java.util aren't thread-safe. When collections are shared between threads, we need synchronization. Collections.synchronizedList() and similar wrappers provide synchronized access, but they require external synchronization for iteration. ConcurrentHashMap and other classes from java.util.concurrent provide better concurrent access with finer-grained locking. Hashtable and Vector are legacy thread-safe collections that should be replaced with modern alternatives.

Immutable objects eliminate many concurrency problems because state that can't change doesn't need synchronization. When reviewing class design for concurrent use, immutability simplifies correctness. Final fields initialized in constructors create safe publication. Value objects like LocalDate, String, and properly designed domain objects work safely across threads when immutable.

Null Safety and Optional

Optional provides a way to express that values might be absent, making the possibility explicit in the type system. We use Optional for return types when a value might not exist: Optional<User> findById(String id). This forces callers to handle absence explicitly. We don't use Optional for parameters—null is simpler and avoids creating objects just to wrap possibly null arguments.

Optional creation follows specific patterns. Optional.of(value) should only be used when the value is guaranteed non-null—if it might be null, that's a bug the code should detect with NullPointerException. Optional.ofNullable(value) accepts null and creates an empty Optional, appropriate when null might be valid. Optional.empty() creates an explicitly empty Optional. When we see Optional.of() in code review, we verify the value truly can't be null.

Extracting values from Optional requires care. The get() method throws if the Optional is empty, defeating much of Optional's purpose—it's little better than dereferencing null. Better alternatives include orElse(defaultValue) to provide a fallback, orElseGet(supplier) when the default is expensive to compute, or orElseThrow() to explicitly convert absence to an exception with a meaningful message.

Null checks remain necessary for parameters, fields, and local variables. The pattern if (param != null) guards against NullPointerException. The Objects class provides utility methods that clarify intent: Objects.requireNonNull(param, "param must not be null") throws NullPointerException with a clear message if null. Using these utilities makes null contracts explicit in code.

Collections should never return null—empty collections work better. Instead of return null when no items exist, return Collections.emptyList(), List.of(), or new ArrayList<>(). This eliminates null checks at call sites and makes the API more consistent. Code that checks for null collections or uses ternary operators to convert null to empty collections suggests the API should be improved.

Modern Java Features

Streams transform collections functionally, expressing operations as pipelines of transformations. When we see explicit loops performing transformations, we consider whether streams improve clarity. The pattern users.stream().filter(u -> u.isActive()).map(User::getName).collect(Collectors.toList()) expresses intent more clearly than an equivalent loop with conditional statements and list building. Streams shine for complex transformations but can obscure simple operations.

Stream operations should be free of side effects. Operations like filter() and map() should only compute results, not modify state. When we see stream operations calling methods that mutate shared state, that's a code smell. Side effects in streams create hard-to-debug issues because stream operations might execute in parallel or in unexpected orders. The purpose of streams is transformation, while iteration with forEach() handles side effects.

Records provide concise syntax for simple data carriers, reducing boilerplate for classes that just hold values. The declaration record User(String name, int age) generates a constructor, accessors, equals, hashCode, and toString automatically. Records work well for DTOs, value objects, and other data-centric classes. When we see classes with only final fields, getters, and standard object methods, records might be appropriate.

Sealed classes control inheritance by restricting which classes can extend or implement them. The pattern sealed interface Shape permits Circle, Rectangle makes the hierarchy closed, enabling exhaustive pattern matching. This proves useful for representing fixed alternatives like state machines or domain concepts with known variants. When reviewing inheritance hierarchies, sealed classes might improve design by making assumptions explicit.

Pattern matching for instanceof eliminates redundant casts. The old pattern of if (obj instanceof String) { String s = (String) obj; } becomes if (obj instanceof String s) where s is automatically cast and scoped to the if block. This reduces boilerplate and eliminates casting errors. When we see instanceof followed by casting, modern Java eliminates the cast.

Text blocks improve readability for multi-line strings like JSON, SQL, or HTML. The pattern using triple quotes preserves formatting without escape sequences:

String json = """
    {
        "name": "value"
    }
    """;

This beats string concatenation or arrays of strings for readability.

Exception Handling Patterns

Checked exceptions require explicit handling, which can be verbose but makes error conditions explicit. When reviewing method signatures that declare checked exceptions, we verify the exceptions are documented in javadoc explaining when they occur. Callers need this information to handle exceptions appropriately. Undocumented checked exceptions create confusion about when they might be thrown and how to handle them.

Empty catch blocks swallow exceptions, hiding problems that should surface. When we see catch (Exception e) {}, that's almost always wrong. If the exception truly doesn't matter, a comment explaining why provides context. More often, the exception should be logged, wrapped and rethrown, or converted to a return value or Optional. Silently swallowing exceptions creates bugs that are extremely hard to diagnose.

Catching specific exceptions rather than generic Exception demonstrates understanding of what can fail. When code catches Exception, it often means the author didn't consider specific failure modes. More specific exceptions allow different handling: a FileNotFoundException might mean creating a default configuration, while IOException indicates a real problem. Catch clauses should be as specific as the actual error handling requires.

Custom exceptions communicate domain-specific failures better than generic exceptions. When business rules fail or domain constraints are violated, custom exceptions with meaningful names and messages help. These should extend Exception for checked exceptions or RuntimeException for unchecked, depending on whether callers should be forced to handle them. Custom exceptions can carry additional context beyond the message string when callers need structured error information.

Finally blocks must not throw exceptions because this masks the original exception from the try block. If cleanup code in finally can throw, it needs its own try-catch to prevent hiding the real problem. This situation is common with resource cleanup, which is why try-with-resources is superior—it suppresses exceptions from close() while preserving the original exception.

Collections and Generic Types

Raw types defeat generics, sacrificing type safety for no benefit. When we see List instead of List<String>, that's a legacy pattern from pre-Java 5 code. Modern code should use generic types everywhere. The compiler warns about raw types because they allow runtime ClassCastException that generics prevent. Any use of raw types in new code suggests misunderstanding generics.

The diamond operator <> reduces verbosity in constructor calls. Instead of new ArrayList<String>(), we write new ArrayList<>() and let the compiler infer the type from the variable declaration. This improves maintainability when the declared type changes—the constructor doesn't need updating. When we see explicit type arguments in constructors, the diamond operator usually improves the code.

Collection choice affects performance and correctness. ArrayList works for most list needs, but LinkedList might be better for frequent insertions and deletions at arbitrary positions. HashSet provides O(1) lookup, while TreeSet maintains sort order. HashMap provides key-value storage, while LinkedHashMap preserves insertion order. When reviewing collection usage, we verify the chosen type matches the access patterns.

Factory methods like List.of() create unmodifiable collections concisely. Instead of Collections.unmodifiableList(Arrays.asList(1, 2, 3)), we write List.of(1, 2, 3). These factory methods return space-efficient implementations optimized for immutability. When code needs unmodifiable collections, these factory methods provide the clearest expression.

Pre-sizing collections when the size is known avoids repeated allocation and copying as the collection grows. new ArrayList<>(expectedSize) allocates capacity upfront, improving performance for large collections that grow to predictable sizes. This optimization matters in performance-critical code but can be premature optimization elsewhere.

Bringing It Together

Effective Java code review requires understanding both timeless principles and modern language features. Thread safety and resource management transcend language versions—they've been important since Java's beginning. Modern features like records, sealed classes, and pattern matching provide better ways to express intent, but the underlying design principles remain constant.

Not every issue warrants blocking a pull request. A missing try-with-resources for a file operation prevents a resource leak and should be fixed before merge. A verbose implementation that could use streams might not matter if the code is clear and correct. The key is distinguishing issues that affect reliability, security, or maintainability from those that represent minor polish.

Java's extensive standard library and mature ecosystem provide solutions for most common problems. Code that reimplements functionality available in the standard library or well-maintained libraries usually indicates the author didn't know better alternatives existed. Part of code review is sharing knowledge about what's available and when to use it, creating learning opportunities that improve the entire team's effectiveness.