Code Review · Java

Java Code Review: Optional Misuse

Review Java code that uses Optional in a way that adds complexity or defeats its purpose.

Code Review Exercise

Review the following code and identify as many issues as you can. Think about correctness, readability, maintainability, and modern Java practices.

java
public void notifyUser(String id) {
    Optional<User> user =
        Optional.of(repository.findById(id).get());

    if (user.isPresent()) {
        sendEmail(user.get());
    }
}

What concerns do you see?

Review Findings

Issue #1: get() defeats the purpose of Optional

The code immediately calls get() on the result of findById(). If no user exists, a NoSuchElementException will be thrown.

Issue #2: Unnecessary Optional wrapping

The repository already returns an Optional<User>. Wrapping it again inside Optional.of(...) adds complexity without providing any benefit.

Issue #3: Redundant isPresent() / get() pattern

The combination of isPresent() followed by get() is often a code smell. Modern Java provides cleaner alternatives.

Issue #4: Reduced readability

Optional is intended to make missing values explicit. This code introduces extra ceremony while still relying on exceptions for control flow.

Why This Happens

Many developers learn that Optional should be used whenever a value may be absent.

The mistake is treating Optional like a box that must immediately be opened again using get().

If the code always calls get(), it loses most of the safety benefits that Optional was designed to provide.

Improved Version

java
public void notifyUser(String id) {
    repository.findById(id)
              .ifPresent(this::sendEmail);
}

This version clearly expresses the intent:

  • Find the user.
  • If the user exists, send the email.
  • No manual Optional handling required.

Another Good Alternative

java
User user = repository.findById(id)
                      .orElseThrow();

sendEmail(user);

This version is appropriate when a missing user should be treated as an error condition.

Interview Follow-Ups

What is wrong with Optional.get()?

Nothing is inherently wrong with get(), but using it carelessly can reintroduce the same problems Optional was created to avoid.

Should Optional be used for fields?

Usually no. Optional is primarily intended for return values. Many teams avoid Optional fields because they add complexity and can create serialization issues.

Should Optional be used as a method parameter?

Usually no. Most style guides recommend accepting the actual value and handling nullability at the call site.

When is Optional most useful?

When a method may legitimately return a missing value and the caller should be forced to handle that possibility explicitly.

What Interviewers Expect

A junior reviewer may simply notice that get() can throw an exception.

A stronger reviewer often goes further:

  • Correctness: avoid unsafe get().
  • Readability: remove unnecessary wrapping.
  • Maintainability: use modern Optional APIs.
  • Design: choose the Optional pattern that best matches the business intent.