awesome-skills__code-review-skill/reference/java8.md

Java 8 / Legacy Stack Code Review Guide

Review guidance for codebases still on Java 8 (and nearby legacy stacks: Spring Boot 2.x, javax.*, Hibernate 5). Do not require Java 17/21 features (records, text blocks, virtual threads, ProblemDetail, etc.) on these PRs.

For modern stacks (Java 17/21 + Spring Boot 3), use the Java Guide.

Table of Contents

• When to Use This Guide
• Lambdas & Functional Interfaces
• Stream API
• Optional
• Date/Time API (java.time)
• Concurrency: Thread Pools & CompletableFuture
• Spring Boot 2
• JPA / Hibernate 5
• Exception Handling
• Testing
• Review Checklist
• References

---

When to Use This Guide

Scenario	Load
Java 8 / 11 (no modern syntax), Spring Boot 2.x, javax.persistence	This file
Java 17/21, Spring Boot 3, jakarta.*, virtual threads	java.md
Migrating Boot 2 → Boot 3	Use both: this guide for legacy pitfalls, java.md for the target

Confirm java.version, Spring Boot version, and javax vs jakarta package names in pom.xml / build.gradle before choosing a guide.

---

Lambdas & Functional Interfaces

Keep them short; prefer method references

// ❌ Long lambdas are hard to read, test, and debug
users.stream().forEach(u -> {
    // dozens of lines of business logic...
});

// ✅ Extract a method, or use a method reference
users.forEach(this::processUser);

Prefer JDK functional interfaces

// ❌ Unnecessary custom functional interface
@FunctionalInterface
interface UserCallback {
    void accept(User u);
}

// ✅ Use Consumer / Function / Predicate / Supplier / BiFunction, etc.
void process(Consumer<User> callback) { ... }

Captured variables must be effectively final

// ❌ Mutating a captured variable → compile error (or Atomic/array hacks)
int sum = 0;
list.forEach(n -> sum += n); // does not compile

// ✅ Reduce with a stream, or use an explicit mutable accumulator type
int sum = list.stream().mapToInt(Integer::intValue).sum();

---

Stream API

Do not force Streams for simple loops

// ❌ Side effects only — Stream adds no value
items.stream().forEach(item -> process(item));

// ✅ Plain for-each is clearer
for (Item item : items) {
    process(item);
}

Collect with Collectors on Java 8

// ❌ Stream.toList() is Java 16+ — not available on Java 8
list.stream().map(...).toList();

// ✅ Java 8
List<Dto> result = list.stream()
    .map(...)
    .collect(Collectors.toList());

Note: Collectors.toList() is not guaranteed immutable. For an unmodifiable list, wrap with Collections.unmodifiableList(...) or use Guava / List.copyOf (the latter needs a newer JDK).

Two classic Collectors.toMap pitfalls

// ❌ Null values → NPE (internally uses Map.merge, which forbids null values)
Map<Long, String> map = users.stream()
    .collect(Collectors.toMap(User::getId, User::getNickname)); // nickname may be null

// ✅ Filter first, or handle nulls explicitly
Map<Long, String> map = users.stream()
    .filter(u -> u.getNickname() != null)
    .collect(Collectors.toMap(User::getId, User::getNickname));

// ❌ Duplicate keys → IllegalStateException
.collect(Collectors.toMap(User::getName, Function.identity()));

// ✅ Provide a merge function
.collect(Collectors.toMap(User::getName, Function.identity(), (a, b) -> a));

Be careful with parallelStream()

// ❌ Small collections / I/O / shared mutable state — often slower or unsafe
list.parallelStream().forEach(sharedList::add); // race

// ❌ Side-effecting forEach into a non-concurrent collection
map.entrySet().parallelStream().forEach(e -> result.put(e.getKey(), e.getValue()));

// ✅ Consider only for CPU-bound work, no shared mutable state, and large enough data
// ✅ Collect with toMap / toConcurrentMap — do not forEach into an external Map
Map<K, V> result = list.parallelStream()
    .collect(Collectors.toConcurrentMap(Item::getKey, Item::getValue, (a, b) -> a));

Parallel streams use ForkJoinPool.commonPool() by default and compete with other parallel / CompletableFuture work in the same process.

Do not mutate the stream source; avoid nested forEach

// ❌ Mutating the source during the pipeline → ConcurrentModificationException
list.stream().peek(list::add).count();

// ❌ Nested forEach is hard to read and hard to short-circuit
a.forEach(x -> b.forEach(y -> ...));

// ✅ Prefer flatMap or ordinary loops for cartesian / join-style logic

Prefer primitive streams to avoid boxing

// ❌ Stream<Integer> boxing overhead
int sum = list.stream().map(Order::getAmount).reduce(0, Integer::sum);

// ✅ IntStream / LongStream / DoubleStream
int sum = list.stream().mapToInt(Order::getAmount).sum();

---

Optional

Intent: express possible absence as a return type, not as a general null replacement.

// ❌ Optional as field / parameter / collection element (serialization, reflection, API noise)
class User {
    private Optional<String> email;
}
void send(Optional<String> email) { ... }
Optional<List<Order>> findOrders(); // empty list already means "none"

// ✅ Return type only; return empty collections for "none"
public Optional<User> findById(Long id) { ... }
public List<Order> findOrders(Long userId) { ... } // emptyList when none

Do not use isPresent() + get() as a null check

// ❌ More verbose than a null check, and get() can still blow up
if (userOpt.isPresent()) {
    return userOpt.get().getName();
}
return "Unknown";

// ✅ Functional chain (available on Java 8)
return userOpt.map(User::getName).orElse("Unknown");

orElse vs orElseGet

// ❌ orElse argument is always evaluated (even when the Optional is present)
return findUser(id).orElse(loadDefaultFromDb()); // always hits DB

// ✅ Expensive defaults → orElseGet
return findUser(id).orElseGet(this::loadDefaultFromDb);

// ✅ Required value (Java 8)
return findUser(id).orElseThrow(() -> new UserNotFoundException(id));
// Note: no-arg orElseThrow() is Java 10+; Java 8 requires a Supplier

of vs ofNullable; nest with flatMap

// ❌ of(null) → immediate NPE
Optional.of(possiblyNull);

// ✅
Optional.ofNullable(possiblyNull);

// ❌ map returning Optional → Optional<Optional<T>>
optional.map(this::findOther); // findOther returns Optional

// ✅
optional.flatMap(this::findOther);

Java 8 has no Optional.stream() / ifPresentOrElse / or (those are Java 9+). Filtering a collection of Optionals:

list.stream()
    .map(this::findUser)
    .filter(Optional::isPresent)
    .map(Optional::get) // OK after filter; or extract a helper
    .collect(Collectors.toList());

---

Date/Time API (java.time)

One of the most common production footguns in legacy systems: keeping Date / Calendar / SimpleDateFormat.

// ❌ SimpleDateFormat is not thread-safe; a shared static instance corrupts state
private static final SimpleDateFormat SDF = new SimpleDateFormat("yyyy-MM-dd");

// ✅ DateTimeFormatter is immutable and thread-safe
private static final DateTimeFormatter FMT = DateTimeFormatter.ofPattern("yyyy-MM-dd");

Pick the right type

Type	Use for
Instant	Machine timestamps, audit, cross-service events (UTC timeline)
LocalDate	Date only (birthday, business day)
LocalDateTime	Date-time without zone; do not use for "when an event happened"
ZonedDateTime / OffsetDateTime	Human time that needs a zone or offset

// ❌ LocalDateTime for "order placed at" — ambiguous across zones / DST
private LocalDateTime createdAt = LocalDateTime.now();

// ✅ Instant for event time; convert to a zone only for display
private Instant createdAt = Instant.now();

// ❌ now() depends on the JVM default zone — CI / prod / laptop disagree
LocalDate.now();

// ✅ Explicit ZoneId, or inject Clock for tests
LocalDate.now(ZoneOffset.UTC);
LocalDate.now(clock);

Formatting traps

// ❌ YYYY is week-based year — wrong year near year boundaries
DateTimeFormatter.ofPattern("YYYY-MM-dd");

// ✅ Calendar year uses yyyy
DateTimeFormatter.ofPattern("yyyy-MM-dd");

Interop with legacy APIs: date.toInstant(), Date.from(instant), LocalDateTime.ofInstant(instant, zone).

---

Concurrency: Thread Pools & CompletableFuture

Java 8 has no virtual threads. I/O-heavy work needs a well-sized pool — not newCachedThreadPool or an unbounded queue.

// ❌ Unbounded queue + default rejection — latency explodes or OOM under load
ExecutorService exec = Executors.newFixedThreadPool(8); // unbounded queue

// ❌ Never shut down → thread leak
Executors.newFixedThreadPool(8).submit(task);

// ✅ Bounded queue + explicit rejection policy + lifecycle management
ThreadPoolExecutor exec = new ThreadPoolExecutor(
    8, 16, 60L, TimeUnit.SECONDS,
    new ArrayBlockingQueue<>(500),
    new ThreadPoolExecutor.CallerRunsPolicy());
// On shutdown: shutdown → awaitTermination → shutdownNow

CompletableFuture

// ❌ I/O on the common pool (supplyAsync with no Executor)
CompletableFuture.supplyAsync(() -> restTemplate.getForObject(url, Dto.class));

// ✅ Explicit I/O executor
CompletableFuture.supplyAsync(() -> callRemote(), ioExecutor);

// ❌ thenApply returning a CF → nested CompletableFuture<CompletableFuture<T>>
.thenApply(id -> findAsync(id));

// ✅ Dependent async work → thenCompose
.thenCompose(id -> findAsync(id));

// ❌ get()/join() inside async callbacks — starves the pool or deadlocks
.thenApply(x -> other.join());

// ✅ Combine with allOf / thenCombine; join once at the boundary

Timeouts: Java 8 has no orTimeout / completeOnTimeout (Java 9+). Use get(timeout, unit), or a scheduler + applyToEither that completes exceptionally.

Exceptions: Attach exceptionally / handle / whenComplete at the chain boundary so failures are not swallowed.

// ❌ Shared mutable SimpleDateFormat / HashMap as a cache
private static final SimpleDateFormat SDF = ...;
private final Map<String, String> cache = new HashMap<>(); // concurrent puts

// ✅ ConcurrentHashMap; dates via java.time
private final ConcurrentHashMap<String, String> cache = new ConcurrentHashMap<>();

---

Spring Boot 2

Packages are javax.*, not jakarta.*. Do not require a Jakarta migration unless the PR's goal is upgrading to Boot 3.

Dependency injection

// ❌ Field @Autowired: hard to test, opaque dependencies
@Autowired
private UserRepository userRepo;

// ✅ Constructor injection (Boot 2: single constructor can omit @Autowired)
private final UserRepository userRepo;

public UserService(UserRepository userRepo) {
    this.userRepo = userRepo;
}

Configuration

// ❌ Hard-coded secrets; @Value scattered everywhere
@Value("${app.payment.api-key}")
private String apiKey;

// ✅ @ConfigurationProperties (Java 8 uses a class, not a record)
@ConfigurationProperties(prefix = "app.payment")
public class PaymentProperties {
    private String apiKey;
    private int timeoutMs;
    // getters / setters
}

Register with @EnableConfigurationProperties. On Boot 2.2+, @ConfigurationPropertiesScan also works (@SpringBootApplication scans the startup class package by default).

RestTemplate must have timeouts

Default is infinite wait. A hung downstream can exhaust Tomcat / worker threads.

// ❌ Bare new RestTemplate() — no timeouts
return new RestTemplate();

// ✅ Boot 2.1+ Duration-based timeouts
@Bean
public RestTemplate restTemplate(RestTemplateBuilder builder) {
    return builder
        .setConnectTimeout(Duration.ofSeconds(2))
        .setReadTimeout(Duration.ofSeconds(5))
        .build();
}

If you customize ClientHttpRequestFactory (e.g. Apache HttpClient), set connect / read / connectionRequest timeouts on the factory too — otherwise builder timeouts may be ignored.

Transaction proxy trap (same on Boot 2/3)

// ❌ Same-class self-call — @Transactional does not apply (proxy not invoked)
public void create(Order o) {
    save(o); // internal call
}
@Transactional
public void save(Order o) { ... }

// ✅ Put the transaction boundary on the public entry point, or split into another bean
@Transactional
public void create(Order o) { saveInternal(o); }

@Transactional on private methods is also ineffective.

---

JPA / Hibernate 5

Entity annotations come from javax.persistence.*.

N+1

Cross-language background: N+1 query guide

// ❌ EAGER, or loops that trigger lazy loads
@OneToMany(fetch = FetchType.EAGER)
private List<Order> orders;

for (User u : userRepo.findAll()) {
    u.getOrders().size(); // N queries when lazy
}

// ✅ JOIN FETCH / @EntityGraph; keep LAZY by default
@Query("SELECT u FROM User u JOIN FETCH u.orders")
List<User> findAllWithOrders();

Transactions & read-only

// ❌ Transactions opened in Controllers; or @Transactional on private methods
// ✅ Public Service methods; mark reads readOnly
@Transactional(readOnly = true)
public User get(Long id) { ... }

Entities & Lombok

// ❌ @Data equals/hashCode often pulls in lazy associations
@Entity
@Data
public class User { ... }

// ✅ @Getter/@Setter; equals/hashCode on a stable business key, or null-safe id
// ⚠️ Do not include lazy associations; new unsaved entities all have null id,
//    so id-only equals treats them as unequal (usually acceptable)
@Entity
@Getter
@Setter
public class User {
    @Id
    private Long id;

    @Override
    public boolean equals(Object o) {
        if (this == o) return true;
        if (!(o instanceof User)) return false;
        return id != null && id.equals(((User) o).id);
    }

    @Override
    public int hashCode() {
        return getClass().hashCode();
    }
}

Temporal fields

Hibernate 5 supports java.time, but mapping ZonedDateTime to a zone-less TIMESTAMP normalizes with the JVM zone and drifts across regions. Prefer:

• Store UTC: Instant or OffsetDateTime
• Set spring.jpa.properties.hibernate.jdbc.time_zone=UTC when the team agrees
• Migrate legacy java.util.Date fields deliberately; do not add new Date / @Temporal in new code

---

Exception Handling

Boot 2 does not treat Spring 6 ProblemDetail as a first-class citizen (that is a Boot 3 story). Use a shared @ControllerAdvice with clear HTTP statuses.

// ❌ Swallow exceptions, printStackTrace, return null to hide failure
try {
    userService.create(user);
} catch (Exception e) {
    e.printStackTrace();
    return null;
}

// ✅ Domain exceptions + global handler
@RestControllerAdvice
public class GlobalExceptionHandler {
    @ExceptionHandler(UserNotFoundException.class)
    public ResponseEntity<ApiError> handleNotFound(UserNotFoundException e) {
        return ResponseEntity.status(HttpStatus.NOT_FOUND)
            .body(new ApiError("USER_NOT_FOUND", e.getMessage()));
    }
}

Close resources with try-with-resources (Java 7+). Avoid hand-rolled finally { close() } that forgets null checks.

---

Testing

// ❌ @SpringBootTest for every unit test (slow and brittle)
@SpringBootTest
public class UserServiceTest { ... }

// ✅ Pure unit tests: JUnit 4/5 + Mockito
@RunWith(MockitoJUnitRunner.class) // JUnit 4
// or @ExtendWith(MockitoExtension.class) // JUnit 5
public class UserServiceTest {
    @Mock private UserRepository repo;
    @InjectMocks private UserService service;

    @Test
    public void shouldCreateUser() { ... }
}

Inject Clock into time-sensitive logic so tests do not depend on Instant.now().

Legacy stacks often use JUnit 4; if JUnit 5 is mixed in, keep one runner style per module.

---

Review Checklist

Version & scope

• Confirmed Java 8 / Boot 2 / javax.* — do not demand Java 17+ APIs or jakarta.*
• Do not treat "use records / virtual threads / text blocks" as blocking feedback

Language features

• Lambdas stay short; prefer method references and standard functional interfaces
• Streams used for transform / filter / reduce, not simple side-effect loops
• Collection uses Collectors.* (no Stream.toList())
• toMap handles null values and duplicate keys
• parallelStream is justified and has no shared mutable state
• Optional is return-type only; no isPresent+get abuse; expensive defaults use orElseGet
• Dates use java.time; no shared SimpleDateFormat; correct type (Instant vs LocalDateTime)
• Patterns use yyyy, not week-based YYYY (unless week-year is intentional)

Concurrency

• Thread pools are bounded and shut down; I/O does not use ForkJoinPool.commonPool()
• CompletableFuture uses an explicit Executor; thenCompose for nested async; timeouts and error handling present
• Shared state uses concurrent collections; no static mutable DateFormat

Spring Boot 2 / JPA

• Constructor injection; config via @ConfigurationProperties
• RestTemplate (and RequestFactory) has connect/read timeouts
• @Transactional on public entry points — no self-invocation / private-method failures
• No N+1; entities avoid @Data; temporal strategy is explicit (UTC)
• Packages stay consistently javax.* — no javax/jakarta mix

Quality

• Exceptions are not swallowed; error responses are centralized
• try-with-resources for I/O and DB resources
• Core logic has unit tests; time is injectable via Clock

---

References

• What to Look for in Java 8 Code (JetBrains)
• JDK-8148463: Collectors.toMap fails on null values
• Oracle Tutorial: Parallelism
• Baeldung: Migrating to Java 8 Date/Time API
• Baeldung: CompletableFuture and ThreadPool
• Spring Boot RestTemplate customization
• Thorben Janssen: Hibernate/JPA Date and Time