Skip to main content

Java Code Review Checklist

Rapid-scan checklist for Java code reviews. Use this to quickly identify issues, then dive into linked documentation for details.

Security Red Flags - Block PR Immediately​

  • SQL injection: String concatenation in SQL queries → Use parameterized queries/PreparedStatement
  • Deserialization of untrusted data: ObjectInputStream.readObject() on external input → Deserialization vulnerabilities
  • Path traversal: User input directly in file paths (new File(userInput)) → Validate and sanitize paths
  • Command injection: Runtime.exec() with user input → Avoid or use allowlists
  • Hardcoded secrets: Passwords, API keys, tokens in source code → Configuration Management
  • Reflection abuse: Class.forName() or Method.invoke() with unsanitized user input
  • XML External Entity (XXE): XML parsers without disabling external entities
  • Insecure random: Using java.util.Random for security tokens → Use SecureRandom for cryptographic operations
  • Sensitive data in logs: Passwords, credit cards, SSNs, API keys in log statements → Sensitive Data Protection
  • Missing input validation: User input not validated before processing
  • Insecure crypto: Weak algorithms (MD5, SHA1, DES) or ECB mode → Use AES-256-GCM, SHA-256+

Common Mistakes​

Exception Handling​

  • Catching generic Exception or Throwable (too broad) → Catch specific exceptions
  • Empty catch blocks swallowing exceptions
  • Not logging exceptions before rethrowing
  • Exposing stack traces to users (leaks implementation details)
  • Using exceptions for control flow instead of validation
  • Not including cause when wrapping exceptions → Use new CustomException(message, cause)

Resource Management​

  • Not using try-with-resources for AutoCloseable resources → Resource leaks prevention
  • Streams/readers/writers not closed properly
  • Database connections not returned to pool
  • File handles leaked in error paths
  • Thread pools not shut down in finally block
// GOOD: try-with-resources ensures cleanup
try (var stream = Files.lines(path)) {
    stream.forEach(System.out::println);
}

// BAD: Stream never closed if exception occurs
var stream = Files.lines(path);
stream.forEach(System.out::println);

Null Handling​

  • Missing null checks on method parameters
  • Returning null instead of Optional for nullable values → Optional usage
  • Not using @NonNull / @Nullable annotations
  • Calling methods on potentially null references without checks

Collections​

  • Using mutable collections when immutable would work → Use List.of(), Set.of(), Map.of() (Collections)
  • Modifying collections while iterating (causes ConcurrentModificationException)
  • Using wrong collection type (ArrayList for frequent inserts/deletes at beginning)
  • Not specifying initial capacity for large collections (causes resizing overhead)

Equals and HashCode​

  • Overriding equals() without hashCode() (breaks HashSet/HashMap)
  • Using mutable fields in hashCode() calculation
  • equals() not symmetric, transitive, or consistent
  • Not checking instanceof before casting in equals()

Watch For - Concurrency & Thread Safety​

Thread Safety Issues​

  • Mutable static fields accessed by multiple threads → Java Concurrency
  • Missing synchronization on shared mutable state
  • Double-checked locking without volatile (broken pattern)
  • Non-thread-safe collections (ArrayList, HashMap) shared between threads
  • Race conditions in check-then-act patterns
// BAD: Race condition - multiple threads can pass check
if (counter < MAX) {
    counter++;  // Not atomic!
}

// GOOD: Atomic operation
atomicCounter.getAndIncrement();

Virtual Threads Issues (Java 21+)​

  • Pinning virtual threads with synchronized blocks doing I/O → Use ReentrantLock (Virtual Threads)
  • Using thread-local storage heavily (memory overhead with millions of virtual threads)
  • CPU-bound tasks in virtual threads (better on platform threads)
  • Not using virtual threads for I/O-bound operations in Spring Boot 3.2+

Concurrency Primitives​

  • Using Thread.stop() or Thread.suspend() (deprecated, unsafe)
  • Not handling InterruptedException properly (catching and ignoring)
  • Busy-waiting instead of using wait()/notify() or higher-level constructs
  • Creating threads directly instead of using ExecutorService
  • Not shutting down ExecutorService (leaks threads)

CompletableFuture & Async​

  • Not handling exceptions in CompletableFuture chains → Use exceptionally() or handle()
  • Blocking operations in async callbacks (defeats purpose)
  • Missing timeout on future operations (can hang indefinitely)
  • Using default executor (ForkJoinPool.commonPool) for I/O tasks

Watch For - Performance & Correctness​

String Operations​

  • String concatenation in loops (str += x) → Use StringBuilder (Performance)
  • Using String.format() in hot paths (slow)
  • Not reusing compiled Pattern for repeated regex operations
  • String.split() when indexOf() would work

Object Creation​

  • Creating unnecessary objects in tight loops
  • Not reusing immutable objects (wrapper types, strings)
  • Using new String("literal") instead of string literal
  • Boxing/unboxing in performance-critical code

Streaming & Collections​

  • Using streams for trivial operations (overhead vs simple loop)
  • Not using parallel streams when beneficial (CPU-bound bulk operations)
  • Using parallel streams for I/O operations (blocks threads)
  • Creating intermediate collections unnecessarily
// BAD: Creates intermediate list
list.stream()
    .collect(Collectors.toList())
    .stream()
    .filter(...)

// GOOD: Single stream pipeline
list.stream()
    .filter(...)

Large Methods​

  • Methods longer than 40-50 lines (hard to test/understand)
  • Cyclomatic complexity > 10 (too many branches)
  • Deeply nested conditionals (> 3 levels)
  • Missing early returns for validation

Best Practices - Code Quality​

Modern Java Features (Java 21+)​

Immutability​

  • Final fields by default → Immutability
  • Immutable collections (List.of(), Set.of(), Map.of())
  • Defensive copying when exposing internal state
  • Records instead of mutable POJOs for data carriers

Enums & Constants​

  • Enums instead of string/int constants → Enums
  • Enums with behavior (methods per constant)
  • Constants defined close to usage, not in global Constants class → Constants Location

SOLID Principles​

  • Single Responsibility: one class, one reason to change → SRP
  • Dependency Inversion: depend on interfaces, not concrete classes → DIP
  • Constructor injection over field injection
  • Composition over inheritance

Exception Handling​

  • Custom exceptions with domain context → Custom Exceptions
  • Specific exception types (not generic Exception)
  • Include cause when rethrowing/wrapping
  • Proper logging before rethrowing

Code Quality​

  • Zero compiler warnings (enable -Werror)
  • No @SuppressWarnings without justification comment
  • Spotless/Google Java Format for consistent formatting → Code Quality
  • JavaDoc for public APIs and complex methods

Quick Scan Table​

CategoryWhat to CheckSeverityReference
SecurityNo SQL string concatenationBlockSQL Injection
SecurityNo deserialization of untrusted dataBlockSecurity
SecurityNo path traversal vulnerabilitiesBlockValidate file paths
SecurityNo hardcoded secretsBlockConfiguration
SecuritySecureRandom for crypto (not Random)BlockUse SecureRandom
SecurityNo sensitive data in logsBlockLogging
Resourcestry-with-resources for AutoCloseableFixJava General
ExceptionsNo empty catch blocksFixException Handling
ExceptionsSpecific exceptions (not generic)FixException Handling
GOOD:Null SafetyOptional for nullable returnsReview
ConcurrencyNo mutable static fieldsBlockConcurrency
ConcurrencyProper synchronization on shared stateFixConcurrency
GOOD:ConcurrencyVirtual threads for I/O operationsReview
ConcurrencyNo pinning with synchronized + I/OFixVirtual Threads
GOOD:CollectionsImmutable collections preferredReview
Equals/HashhashCode() overridden with equals()FixCommon practice
PerformanceStringBuilder in loops (not +=)FixPerformance
GOOD:Modern JavaRecords for immutable DTOsReview
GOOD:Modern JavaSealed classes for type hierarchiesReview
GOOD:Modern JavaPattern matching usedReview
Code QualityNo compiler warningsFixCode Quality

Legend:

  • Block PR - Security vulnerability or data corruption risk
  • Fix Before Merge - Performance issue, correctness problem, or likely bug
  • [GOOD] Review & Discuss - Code quality improvement

Additional Considerations​

Deprecated APIs​

  • Using deprecated methods without migration plan
  • Not addressing deprecation warnings
  • Missing @Deprecated annotation with @deprecated JavaDoc explaining alternative

Date/Time APIs​

  • Using legacy Date, Calendar, SimpleDateFormat → Use java.time.* (LocalDate, Instant, ZonedDateTime)
  • Not using DateTimeFormatter for thread-safe date formatting
  • Missing timezone awareness when needed (ZonedDateTime vs LocalDateTime)

Generics​

  • Raw types (missing generic parameters) → Use List<String> not List
  • Unchecked casts without @SuppressWarnings("unchecked") and justification
  • Type erasure issues in complex generic hierarchies
  • Not using bounded wildcards when appropriate (? extends T, ? super T)

Serialization​

  • Implementing Serializable without serialVersionUID
  • Transient fields not properly initialized on deserialization
  • Security vulnerabilities from deserializing untrusted data
  • Not considering alternatives (JSON, Protocol Buffers) to Java serialization

Logging​

  • String concatenation in log statements → Use placeholders log.info("User {}", userId) (Logging)
  • Wrong log levels (DEBUG for errors, ERROR for info)
  • Logging entire request/response objects (may contain PII)
  • Not including correlation IDs for tracing

Testing Considerations​

  • Missing unit tests for public methods → Java Testing
  • Not testing edge cases (null, empty, boundary values)
  • Integration tests without TestContainers (using in-memory DBs incorrectly)
  • Missing mutation testing coverage
  • Tests with hard-coded sleeps (Thread.sleep()) instead of proper synchronization

Block Checklist - Must Fix Before Merge​

  • No SQL injection vulnerabilities (parameterized queries only)
  • No deserialization of untrusted data
  • No path traversal vulnerabilities
  • No hardcoded secrets in code
  • SecureRandom used for security operations (not Random)
  • No sensitive data in logs
  • All AutoCloseable resources use try-with-resources
  • No empty catch blocks
  • No mutable static fields accessed by multiple threads
  • equals() and hashCode() overridden together
  • No compiler warnings (or justified suppressions)
  • Virtual threads not pinned with synchronized blocks doing I/O

Java-Specific Testing Checklist​

Before approving Java PRs:

  • Unit tests cover happy path and error cases → Java Testing
  • Edge cases tested (null, empty, boundary values)
  • Exceptions properly tested
  • Mocks/stubs used appropriately
  • Integration tests use TestContainers (not H2 for PostgreSQL) → Testing
  • Mutation testing passing configured threshold
  • No flaky tests (tests pass consistently)
  • Tests don't rely on execution order
  • No hard-coded Thread.sleep() (use proper await/polling)
  • Test names clearly describe what's being tested

Java Language Guides:

Framework-Specific (Spring Boot):

Process: