Skip to content

feat: add eval for Optional presence selecting an enum value #74

Description

@martinfrancois

Problem

The Java Optionals skill should recognize when an Optional presence check is only being used to select between two enum values. Even though no get() is involved, optional.isPresent() ? A : B still hides the value-flow intent compared with a direct map(...).orElse(...) expression.

Code before the prompt was executed

The code chose a dry-run board setup mode from whether an existing board id was present:

void rejectDryRunNewBoardInProgress(LocalSetup.Options options) {
    BoardSetupChoice dryRunChoice =
            options.existingBoardId().isPresent() ? BoardSetupChoice.EXISTING : BoardSetupChoice.NEW;
    rejectNewBoardInProgress(options, dryRunChoice);
}

Prompt that caused the implementation

The original implementation was part of a bugfix prompt for local setup list handling. The dry-run path had to reject --in-progress for new-board setup while preserving existing-board behavior.

Later prompt that exposed the issue

A PR review comment asked:

Wouldn't it be possible to improve this? Use the optionals skill.

Prompt-produced code before maintainer correction

The reviewed code was the pre-prompt code above. It was functionally correct and did not use get(), but it still used Optional as a boolean flag.

Why the prompt-produced code is weak

The code asks only whether the Optional is present, then manually chooses between two values. That is not a critical Optional antipattern, but the value-flow operation is clearer as "map any present board id to EXISTING, otherwise NEW." The replacement also leaves no temptation to add a later get() if more logic appears around the presence branch.

Behavior-equivalence analysis

The replacement preserves behavior. The present value is intentionally ignored, so mapping it to BoardSetupChoice.EXISTING is safe. The fallback is a constant enum value, so eager orElse(...) is fine; no expensive or side-effecting fallback is introduced.

Maintainer-preferred code

void rejectDryRunNewBoardInProgress(LocalSetup.Options options) {
    BoardSetupChoice dryRunChoice = options.existingBoardId()
            .map(ignored -> BoardSetupChoice.EXISTING)
            .orElse(BoardSetupChoice.NEW);
    rejectNewBoardInProgress(options, dryRunChoice);
}

Why the replacement is better

The Optional expression now owns the present/absent choice directly. It keeps the fallback local, preserves laziness expectations because the fallback is a constant, and avoids treating Optional as a nullable-style boolean branch.

Desired eval behavior

  • Reward replacing optional.isPresent() ? presentValue : absentValue with optional.map(...).orElse(...) when the present branch does not need checked exceptions or side effects.
  • Reward using orElseGet(...) instead when the absent branch performs non-trivial work.
  • Reward preserving behavior when the present Optional value is intentionally ignored.
  • Reward explaining why this is a readability improvement, not a correctness fix.

Anti-patterns the eval should reject

  • Introducing get() or orElseThrow() just to read a value that is not needed.
  • Using orElseGet(...) for a trivial enum or constant fallback when it only adds noise.
  • Moving checked IO or prompting into Optional lambdas.
  • Changing the selected enum values or side-effect order.

Suggested eval name

presence-selects-enum-value

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions