From c9c47d309328b40fd19f33349bbec54cf1a69838 Mon Sep 17 00:00:00 2001 From: martinfrancois Date: Sun, 28 Jun 2026 20:28:52 +0200 Subject: [PATCH 1/2] refactor(skill): move generic functional style ownership out of streams Keep java-streams focused on stream and collector semantics while documenting java-functional-style as the owner for generic lambda, identity-function, supplier-laziness, no-op callback stage, and callback readability guidance. Existing stream eval criteria are left unchanged so the composed setup can be compared against the frozen baseline before any eval cleanup. Co-Authored-By: marvinbuff Co-Authored-By: PReimers --- README.md | 19 +++ docs/agents/ownership-boundaries.md | 42 ++++++ docs/agents/skill-behavior.md | 7 +- skills/java-streams/SKILL.md | 22 ++-- skills/java-streams/references/hard-stops.md | 27 ++-- .../references/java-stream-api.md | 12 +- .../references/stream-examples.md | 122 +----------------- 7 files changed, 94 insertions(+), 157 deletions(-) create mode 100644 docs/agents/ownership-boundaries.md diff --git a/README.md b/README.md index 76ab089..7266ef2 100644 --- a/README.md +++ b/README.md @@ -18,6 +18,10 @@ as a design choice rather than a default optimization. It also tells the agent to check the project Java version first. The right stream code for Java 8 may be different from the right code for Java 17, Java 21, or Java 24. +For general lambda, method-reference, identity-function, no-op functional stage, supplier-laziness, +and callback readability guidance, install the companion package +`martinfrancois/java-functional-style` together with this stream package. + ## Contents - [Getting Started](#getting-started) @@ -234,6 +238,21 @@ Good fit: - choosing Java-version-compatible APIs such as `takeWhile`, `mapMulti`, `Stream.toList()`, and gatherers. +## Ownership Boundaries + +`java-streams` owns stream and collector semantics: terminal operation choice, collector choice, +duplicate key and null behavior, encounter order, primitive streams, stream Java-version +compatibility, parallel stream behavior, gatherers, and stream-specific behavior preservation. + +`java-functional-style` owns general Java lambda and functional-interface style: identity +functions, no-op functional stages, method references, callback readability, supplier laziness, and +callback side-effect boundaries. + +`java-optionals` owns Optional semantics. + +Install `java-streams` and `java-functional-style` together for stream cleanup involving non-trivial +callbacks. + Poor fit: - broad Java style enforcement unrelated to streams or collectors; diff --git a/docs/agents/ownership-boundaries.md b/docs/agents/ownership-boundaries.md new file mode 100644 index 0000000..4ea347d --- /dev/null +++ b/docs/agents/ownership-boundaries.md @@ -0,0 +1,42 @@ +# Ownership Boundaries + +`java-streams` owns stream and collector semantics: + +- terminal operation choice +- collector choice +- duplicate key and null behavior +- encounter order +- primitive streams +- stream Java-version compatibility +- `findFirst` versus `findAny` +- `parallelStream` +- `Gatherers.mapConcurrent` +- stream-specific behavior preservation + +`java-functional-style` owns general Java lambda and functional-interface style: + +- method references +- identity functions +- no-op functional stages +- callback readability and helper extraction +- supplier laziness +- callback side-effect boundaries + +`java-optionals` owns Optional semantics. + +The expected high-quality setup for stream cleanup involving non-trivial callbacks is both +`java-streams` and `java-functional-style`. + +Do not copy all generic lambda guidance back into `java-streams` to fix composition regressions. +Add only the smallest stream-side bridge instruction if hosted evidence proves it is required. + +## Composition Gate + +Before opening a PR for this split, existing stream evals must prove: + +```text +current java-streams behavior <= slimmed java-streams + java-functional-style behavior +``` + +The comparison must use existing evals unchanged and criterion-level results must be equal or +better. Local validation alone is not enough. diff --git a/docs/agents/skill-behavior.md b/docs/agents/skill-behavior.md index 7a101f7..ad3ae93 100644 --- a/docs/agents/skill-behavior.md +++ b/docs/agents/skill-behavior.md @@ -39,9 +39,9 @@ guidance, or auto-selection wording. that name. - The skill should not force streams over clear stateful loops. Stateful sequence output, checked IO, prompts, mutation-heavy code, or complex early exits can remain imperative. -- Keep stream lambdas as short glue. Prefer method references or one-expression lambdas whose body - stays on the same line as `->`, and extract named helpers for branching, loops, temporary - variables, formatting, merge rules, or nested stream chains that would continue on later lines. +- Generic lambda, method-reference, identity-function, no-op functional stage, supplier-laziness, + and callback readability guidance belongs to `java-functional-style`. Keep only stream and + collector semantics canonical here. - Runtime guidance should keep internal workflow language out of ordinary user-facing reviews. Avoid terms such as "hard stop", "marker", "scan", "checklist", and skill names unless the user asked for an explicit skill workflow, audit, or scan command. @@ -52,3 +52,4 @@ guidance, or auto-selection wording. - [README Guidance](readme.md) - [Eval Guidance](evals.md) +- [Ownership Boundaries](ownership-boundaries.md) diff --git a/skills/java-streams/SKILL.md b/skills/java-streams/SKILL.md index a655f7f..819d05e 100644 --- a/skills/java-streams/SKILL.md +++ b/skills/java-streams/SKILL.md @@ -1,7 +1,7 @@ --- name: java-streams license: MIT -description: Review Java stream performance advice, especially slow stream mappings, external collection mutation with forEach/add, and whether parallelStream is safe; clean up mutation and write or refactor Java Stream and Collector code. Avoid common stream antipatterns such as materializing just to inspect, sorting before min/max, counting for existence, nested stream collections, unsafe null sorting, multi-line lambdas, and careless findFirst/findAny changes. Use whenever writing, reviewing, or refactoring Java code that uses Java streams, collectors, stream pipelines, grouping, joining strings, first/any element lookup, sorting, limiting, distinct values, primitive totals, Optional values in streams, or parallel streams, including review prompts asking whether a lookup should use findFirst or findAny. +description: Review Java stream performance advice, especially slow stream mappings, external collection mutation with forEach/add, and whether parallelStream is safe; clean up mutation and write or refactor Java Stream and Collector code. Avoid common stream antipatterns such as materializing just to inspect, sorting before min/max, counting for existence, nested stream collections, unsafe null sorting, and careless findFirst/findAny changes. Use whenever writing, reviewing, or refactoring Java code that uses Java streams, collectors, stream pipelines, grouping, joining strings, first/any element lookup, sorting, limiting, distinct values, primitive totals, Optional values in streams, or parallel streams, including review prompts asking whether a lookup should use findFirst or findAny. --- # Java Streams Skill @@ -27,8 +27,7 @@ When the prompt asks for a named artifact such as `review.md` or a Java source f exact file. Do not answer only in chat when a file artifact is requested. 0. Check the Java baseline first. Use [java-stream-api.md](references/java-stream-api.md) for - minimum versions and fallbacks; do not emulate unavailable APIs with stateful or multi-line - lambdas. + minimum versions and fallbacks. 1. Identify the requested result and pick the matching terminal or collector: | Goal | Preferred API | @@ -80,9 +79,8 @@ exact file. Do not answer only in chat when a file artifact is requested. } ``` 3. Flatten nested sources deliberately. Use `flatMap`, `flatMap(Optional::stream)` on Java 9+, - and `mapMulti` on Java 16+ when clearer. Use helpers when nested lambdas would wrap. For subtype - primitives, filter/cast first, then call `mapToInt`/`mapToLong`/`mapToDouble` directly. For - nested collector callbacks, extract a named `Stream` helper. + and `mapMulti` on Java 16+ when clearer. For subtype primitives, filter/cast first, then call + `mapToInt`/`mapToLong`/`mapToDouble` directly. ```java // Java 9+: flatten Optional values instead of filter(Optional::isPresent).map(Optional::get) @@ -91,11 +89,9 @@ exact file. Do not answer only in chat when a file artifact is requested. 4. Choose accumulation/collectors by result semantics: use `reduce(identity, op)` for immutable non-primitives, `toMap` with merge behavior and deliberate null-key/value handling, non-null keys for `groupingBy`, `partitioningBy` for boolean splits, and flattened nested indexes when clearer. - Extract duplicate-key merge rules into named helpers when ties, nulls, or ordering need more than - a same-line expression. Carry `element + result`, never null sentinels. -5. Preserve ordering, mutability, short-circuit behavior, and lambda readability. Keep stream lambdas - as short glue or method references; use named helpers for branching, merge logic, or nested stream - work. + Preserve duplicate-key merge rules, tie handling, null contracts, and map suppliers. Carry + `element + result`, never null sentinels. +5. Preserve ordering, mutability, short-circuit behavior, and stream/collector semantics. 6. Keep imperative code when it is the clearer boundary for stateful output, checked IO, mutation-heavy logic, or complex early exits. 7. Verify changed branches for empty inputs, one element, duplicates, nulls, ordering, @@ -103,6 +99,10 @@ exact file. Do not answer only in chat when a file artifact is requested. [hard-stops.md](references/hard-stops.md), fix hits, and re-scan. In scan audits, keep hard-stop severities: required hits stay required unless explicitly acceptable. +Generic lambda, method-reference, identity-function, no-op callback stage, supplier-laziness, and +callback readability guidance belongs to the companion package `martinfrancois/java-functional-style`. +Install both packages when stream cleanup also depends on non-trivial callback style. + Review output: - Give a direct behavior-preserving decision plus one safe snippet. diff --git a/skills/java-streams/references/hard-stops.md b/skills/java-streams/references/hard-stops.md index e213847..48cebce 100644 --- a/skills/java-streams/references/hard-stops.md +++ b/skills/java-streams/references/hard-stops.md @@ -54,17 +54,6 @@ Fix these before finalizing: - `Stream.toList()` where a mutable result is required or later code mutates the list. Prefer a mutable collector; do not modernize this to `new ArrayList<>(stream.toList())` when the task or surrounding code says `Stream.toList()` is not valid. -- Multi-line stream lambdas: block lambdas (`-> { ... }`), arrows whose body starts on the next - line, lambdas that rely on more than one helperless boolean check, or lambdas where the nested - stream body continues on later lines (for example `item -> item.children().stream()` followed by - `.filter(...)`). Keep lambdas as glue: prefer method references, concise one-expression lambdas - whose body stays on the same line as `->`, or extracted named helpers. For mapping, predicates, - severity/status selection, and record construction, extract helper methods when the lambda does - non-trivial work instead of fitting it into a wrapped lambda. - -In reviews, avoid examples that put substantial work behind one lambda body, including ad-hoc helper -expressions like `-> input -> ...` or callbacks passed into custom parallel executors. If a non-trivial -decision is needed, extract to a named method and keep the stream chain as glue. - `stream().forEach(...)` or `parallelStream().forEach(...)` that mutates an external `Collection`, `Map`, array, counter, holder object, or `StringBuilder`. Make the stream produce the result directly with `toList()`, `collect(...)`, `toMap(...)`, `joining`, `sum`, or another @@ -83,18 +72,18 @@ decision is needed, extract to a named method and keep the stream chain as glue. For a version-drift audit, report these unavailable APIs and explicitly allowed markers only; do not add unrelated modernization suggestions, import cleanup, `groupingBy` null-key, or collector-safety caveats. - When giving below-baseline replacement code, do not emulate the missing API with multi-line or - stateful block lambdas. Prefer a named helper or a clear loop for Java 8 replacements such as - `takeWhile`/`dropWhile` prefixes, downstream flat-mapping, or paired min/max aggregation. - For downstream flat-mapping, do not show `flatMap(c -> c.items().stream()` with a nested - `.map(...)`/`.filter(...)` chain continuing on later lines; extract that nested stream to a named - helper and call it with a method reference. + For below-baseline replacement code, preserve stream semantics with a small loop or helper when + no equivalent stream API exists, such as `takeWhile`/`dropWhile` prefixes, downstream + flat-mapping, or paired min/max aggregation. Java 8 replacement snippets must not use Java 9+ helpers such as `Map.entry`. When one stream chain contains multiple unavailable APIs, list each unavailable API separately. Example: `flatMap(Optional::stream).toList()` on a Java 8 baseline has two version-drift hits: `Optional::stream` requires Java 9 and `Stream.toList()` requires Java 16. - Missing imports for stream APIs introduced by the rewrite, such as `Comparator`, `Map`, - `Collectors`, `Function`, `BinaryOperator`, or `Gatherers`. + `Collectors`, `BinaryOperator`, or `Gatherers`. + +Generic identity-function, no-op callback stage, supplier-laziness, and callback readability +markers belong to `java-functional-style`, not this stream hard-stop scan. ## Ordering Rules @@ -173,7 +162,7 @@ multiline mode so it catches normally formatted fluent chains. Some markers are classify legitimate uses instead of deleting them mechanically. ```bash -rg -nUP "count\\(\\)\\s*>\\s*0|collect\\([^;]+\\)\\s*\\.\\s*(?:isEmpty|size|getFirst)\\(|collect\\([^;]+\\)\\s*\\.\\s*get\\(\\s*0\\s*\\)|sorted\\([^;]*\\)\\s*\\.\\s*findFirst\\(|sorted\\(\\)\\s*\\.\\s*findFirst\\(|limit\\([^;]+\\)\\s*\\.\\s*sorted\\(|sorted\\([^;]*\\)\\s*\\.\\s*distinct\\(|sorted\\(\\)\\s*\\.\\s*distinct\\(|String\\.join\\(|filter\\(Optional::isPresent\\)\\s*\\.\\s*map\\(Optional::get\\)|parallelStream\\(|\\.parallel\\(|\\.forEach\\(|Collectors\\.toMap\\(|Collectors\\.groupingBy\\(|Comparator\\.naturalOrder\\(\\)|(?\\s*\\{|->\\s*$|->[^\\n]*\\.stream\\(\\)\\s*$" +rg -nUP "count\\(\\)\\s*>\\s*0|collect\\([^;]+\\)\\s*\\.\\s*(?:isEmpty|size|getFirst)\\(|collect\\([^;]+\\)\\s*\\.\\s*get\\(\\s*0\\s*\\)|sorted\\([^;]*\\)\\s*\\.\\s*findFirst\\(|sorted\\(\\)\\s*\\.\\s*findFirst\\(|limit\\([^;]+\\)\\s*\\.\\s*sorted\\(|sorted\\([^;]*\\)\\s*\\.\\s*distinct\\(|sorted\\(\\)\\s*\\.\\s*distinct\\(|String\\.join\\(|filter\\(Optional::isPresent\\)\\s*\\.\\s*map\\(Optional::get\\)|parallelStream\\(|\\.parallel\\(|\\.forEach\\(|Collectors\\.toMap\\(|Collectors\\.groupingBy\\(|Comparator\\.naturalOrder\\(\\)|(? ``` For each hit, decide whether it is legitimate for the project Java baseline and behavior. Fix diff --git a/skills/java-streams/references/java-stream-api.md b/skills/java-streams/references/java-stream-api.md index a4e5b1f..26199e1 100644 --- a/skills/java-streams/references/java-stream-api.md +++ b/skills/java-streams/references/java-stream-api.md @@ -65,8 +65,8 @@ existing null-key behavior deliberately instead of filtering or retaining null k ## Java 8 Fallback Guidance Use this section only when the project baseline is Java 8 or the task asks for Java 8-compatible -replacement code. Identify APIs that are unavailable and give replacements that preserve behavior -without introducing multi-line lambdas: +replacement code. Identify APIs that are unavailable and give replacements that preserve stream +behavior: For Java-version drift reviews, keep the audit scoped to unavailable APIs and explicitly allowed Java 8 stream usage. Do not add unrelated modernization advice such as `Collections.emptyList()` or @@ -78,12 +78,10 @@ general cleanup notes unless the task asks for a broader review. - `Collectors.flatMapping`: when empty downstream groups matter, use a loop or helper that creates the group before adding nested values. Pre-flatten with `flatMap(Type::helper)` before `groupingBy` only when the contract intentionally omits groups whose nested stream is empty. Do - not put a nested stream chain inside a collector lambda or `flatMap(c -> c.items().stream()` - snippet whose nested chain continues on later lines. If you show pre-flattening, use a named - helper and a Java 8-compatible holder such as `AbstractMap.SimpleImmutableEntry`, not `Map.entry`; - implement the helper as a loop or other concise method body, not as a multi-line nested stream. + not use `Map.entry` on Java 8; use a Java 8-compatible holder such as + `AbstractMap.SimpleImmutableEntry` when a holder is needed. - `Collectors.teeing`: use two clear stream passes, a simple loop, or named helper aggregation. Do - not replace it with a complex `reducing` collector whose merge lambda spans multiple lines. + not replace it with a collector that obscures the two aggregate semantics. - `mapMulti`: use `map`, `flatMap`, or a helper method for one-to-few emission on Java 8. - `Stream.toList()`: use `collect(Collectors.toList())`, and choose `Collectors.toCollection(ArrayList::new)` when mutability is required. diff --git a/skills/java-streams/references/stream-examples.md b/skills/java-streams/references/stream-examples.md index d94649c..b4f8f91 100644 --- a/skills/java-streams/references/stream-examples.md +++ b/skills/java-streams/references/stream-examples.md @@ -123,122 +123,11 @@ Pitfall: `Stream.toList()` returns an unmodifiable list. Keep `Collectors.toList mutated later. If a task says `Stream.toList()` is not valid for that mutable result, do not wrap it in `new ArrayList<>(...)`; use the mutable collector directly. -## External Mutation And Lambda Purity +## External Mutation -Keep lambdas as short glue. If a stream operation needs branching, loops, temporary variables, -formatting, a merge rule, or a nested stream chain that would continue after the lambda line, -extract that work into a named method and pass a method reference or a concise one-expression -lambda whose body stays on the same line as `->`. - -For predicate lambdas, any multi-check filter condition should be extracted to a named helper before -the stream boundary if readability is at stake: - -```java -List overdueNotices(List shipments, Clock clock) { - LocalDate today = LocalDate.now(clock); - return shipments.stream() - .filter(shipment -> isOverdue(shipment, today)) - .map(shipment -> toNotice(shipment, today)) - .toList(); -} - -private static boolean isOverdue(Shipment shipment, LocalDate today) { - return shipment.deliveredAt().isEmpty() && shipment.dueDate().isBefore(today); -} - -private static ShipmentNotice toNotice(Shipment shipment, LocalDate today) { - long daysLate = ChronoUnit.DAYS.between(shipment.dueDate(), today); - return new ShipmentNotice( - shipment.id(), - shipment.customerEmail(), - daysLate, - daysLate >= 14 ? "critical" : "late"); -} -``` - -Avoid burying derivation logic in a block lambda: - -```java -List escalations = tickets.stream() - .filter(Ticket::isOpen) - .map(ticket -> { - Duration age = Duration.between(ticket.openedAt(), now); - EscalationLevel level = levelFor(ticket.priority(), age); - return new TicketEscalation(ticket.id(), ticket.assignee(), level, age); - }) - .toList(); -``` - -Extract the logic so the stream chain stays readable and the helper can be tested directly: - -```java -List escalations = tickets.stream() - .filter(Ticket::isOpen) - .map(ticket -> escalationFor(ticket, now)) - .toList(); - -private static TicketEscalation escalationFor(Ticket ticket, Instant now) { - Duration age = Duration.between(ticket.openedAt(), now); - EscalationLevel level = levelFor(ticket.priority(), age); - return new TicketEscalation(ticket.id(), ticket.assignee(), level, age); -} -``` - -This is required even when the helper only builds one output record: temporary values and branching -inside `map(x -> { ... })` are not glue. - -Avoid wrapping a nested stream body inside a collector callback: - -```java -Map> openCaseIdsByOwner = accounts.stream() - .collect(Collectors.groupingBy( - Account::ownerTeam, - Collectors.flatMapping( - account -> account.supportCases().stream() - .filter(SupportCase::isOpen) - .map(SupportCase::caseId), - Collectors.toList()))); -``` - -Extract the nested stream so the collector reads as composition: - -```java -Map> openCaseIdsByOwner = accounts.stream() - .collect(Collectors.groupingBy( - Account::ownerTeam, - Collectors.flatMapping( - AccountCaseReports::openCaseIds, - Collectors.toList()))); - -private static Stream openCaseIds(Account account) { - return account.supportCases().stream() - .filter(SupportCase::isOpen) - .map(SupportCase::caseId); -} -``` - -The same rule applies to downstream `flatMapping` for sorted or de-duplicated nested data. Keep the -collector callback as a method reference: - -```java -Map> emailsByTrack = conferences.stream() - .flatMap(conference -> conference.sessions().stream()) - .filter(session -> session.track() != null) - .collect(Collectors.groupingBy( - Session::track, - Collectors.flatMapping( - SessionReports::optedInEmails, - Collectors.collectingAndThen( - Collectors.toCollection(TreeSet::new), - ArrayList::new)))); - -private static Stream optedInEmails(Session session) { - return session.registrations().stream() - .filter(Registration::optedIn) - .map(Registration::email) - .filter(Objects::nonNull); -} -``` +Generic lambda and callback readability belongs to `java-functional-style`. This package still owns +the stream-semantic rule that ordinary stream results should be produced by terminals and collectors +rather than external mutation. Do not build ordinary stream results by mutating external state from `forEach`: @@ -295,8 +184,7 @@ or mostly-small call paths. ``` Do not include optional parallelism snippets for this pattern unless the user explicitly requests -parallel code. If requested, avoid multiline lambda bodies and prefer method references or -single-line helpers. +parallel code. Only keep terminal `forEach` when the side effect is the operation's purpose, such as logging or calling an API, and the side effect is safe for the chosen stream mode. From 61c35e37ea5acf58c637805f06a25b1aebcd3518 Mon Sep 17 00:00:00 2001 From: martinfrancois Date: Sun, 28 Jun 2026 21:30:27 +0200 Subject: [PATCH 2/2] fix(skill): bridge stream reviews to functional style companion Tighten stream-specific review guidance for ordered findFirst replacements and external-mutation performance claims, and add a narrow bridge for applying the companion callback rules inside stream pipelines. This keeps stream semantics in java-streams while allowing composed java-streams plus java-functional-style runs to satisfy existing focused callback-shape coverage without restoring generic functional-style ownership to streams. Co-Authored-By: marvinbuff Co-Authored-By: PReimers --- skills/java-streams/SKILL.md | 104 +++++++++--------- skills/java-streams/references/hard-stops.md | 14 +++ .../references/stream-examples.md | 78 +++++++++++++ 3 files changed, 144 insertions(+), 52 deletions(-) diff --git a/skills/java-streams/SKILL.md b/skills/java-streams/SKILL.md index 819d05e..361d1ce 100644 --- a/skills/java-streams/SKILL.md +++ b/skills/java-streams/SKILL.md @@ -7,11 +7,9 @@ description: Review Java stream performance advice, especially slow stream mappi # Java Streams Skill Preserve requested behavior, public API/artifact shape, encounter order, exceptions, null handling, -side effects, mutability, and Java-version compatibility. For implementation prompts, write the -requested Java source file before explaining. Keep provided helper/record/service types in that file, -and keep them nested when requested; do not create sibling source files, package-private top-level -replacements, hooks, test seams, delegate fields, overloads, caches, retries, or adapters unless -asked. +side effects, mutability, and Java-version compatibility. For implementation prompts, write only the +requested source; no extra public API, Javadoc, null guards, constructors, or utility ceremony unless +asked. Keep provided helper/record/service types in the requested file and nested when requested. ## Reference Bundle @@ -32,8 +30,6 @@ exact file. Do not answer only in chat when a file artifact is requested. | Goal | Preferred API | |---|---| - | Arbitrary/equivalent match | `filter(...).findAny()` | - | First encounter-order match | `filter(...).findFirst()` | | Existence check | `anyMatch` / `noneMatch` / `allMatch` | | Transformed list/set | `map`/`filter` then collect | | Concatenated text | `Collectors.joining` | @@ -41,46 +37,47 @@ exact file. Do not answer only in chat when a file artifact is requested. | Two aggregates over same input (Java 12+) | `Collectors.teeing` | | Grouping/indexing | `groupingBy`, `partitioningBy`, or `toMap` with merge/null handling | - Find-first rule: keep `findFirst()` when code takes element `0`, sorted order, or encounter order - chooses the winner. In these reviews, include the exception before performance claims: `findAny` - fits only if all matching values are equivalent and order does not choose the winner. Keep - `sorted(...).filter(...).findFirst()` when it defines the selected value; suggest `min`/`max` only - when it preserves that winner. + Use `findAny()` only when all matches are equivalent and order does not choose the winner; in + reviews, name that exception with the code's noun, e.g. "all matching primary addresses are + equivalent." Use `findFirst()` when element `0`, sorted order, encounter order, or priority + selects the value. Do not offer `min`/`max` unless the user asks for optimization. 2. Use intent-encoding terminals: `anyMatch`, `count`, `joining`, `min`/`max`, Java 12+ `teeing`, and primitive terminals. Do not mutate external containers, arrays, counters, or builders from `forEach`; let the stream produce the result directly. - - Implementation prompts: for one-to-one transformations, write the direct stream result unless - mutable output or the Java baseline says otherwise; on Java 16+, prefer + - Direct transforms: write the direct stream result; on Java 16+, prefer `names.stream().map(String::toUpperCase).toList()` over a manual `ArrayList` loop. - - External mutation/performance reviews: show a sequential result-producing snippet first. For - million-item CPU maps, mention benchmarking a pure parallel variant and include: - "`parallelStream()` can be slower for small lists or call paths that are usually small." - - Parallel reviews: apply [hard-stops.md](references/hard-stops.md); no custom pool snippets - unless asked. - - Java 24 bounded blocking calls: use `Gatherers.mapConcurrent(limit, item -> carrier(item, - stub(item)))`, then filter/map/sort; no test hooks, overloads, `CompletableFuture` fan-out, or - null sentinels. - - ```java - import java.util.List; - - final class OrderChecks { - boolean hasOverdue(List orders) { - return orders.stream().anyMatch(Order::isOverdue); - } - - record Order(boolean overdue) { - boolean isOverdue() { - return overdue; - } - } - } - ``` + - Performance/parallel reviews: for external mutation, show a sequential result-producing snippet + first. Explicitly separate the correctness fix from the performance decision: collecting + directly removes external mutation; it is not a guaranteed throughput win. Do not cite resize + overhead or list throughput as why `forEach(add)` is wrong. If discussing parallel work, mention + measurement, common-pool/split overhead, and slower small-list or mostly-small call paths. + - Java 24 bounded blocking calls: use `Gatherers.mapConcurrent` as documented in + [java-stream-api.md](references/java-stream-api.md); no test hooks, overloads, + `CompletableFuture` fan-out, or null sentinels. Call the provided production stub directly and + carry `element + boolean result` through a non-null holder: + + ```java + records.stream() + .gather(Gatherers.mapConcurrent( + limit, + record -> new Checked<>(record, RemoteService.accepts(record.id())))) + .filter(Checked::accepted) + .map(Checked::value) + .toList(); + + record Checked(T value, boolean accepted) {} + ``` 3. Flatten nested sources deliberately. Use `flatMap`, `flatMap(Optional::stream)` on Java 9+, and `mapMulti` on Java 16+ when clearer. For subtype primitives, filter/cast first, then call `mapToInt`/`mapToLong`/`mapToDouble` directly. + Extract nested `flatMap` callbacks recursively: use `.flatMap(Type::childEntries)`, keep helpers + stream-based rather than temp-list loops, and repeat inside helpers until callbacks no longer + continue stream chains across lines. Apply the same shape to nested `anyMatch` chains and + downstream `Collectors.flatMapping`: use named stream-returning helpers or method references, not + lambdas whose bodies continue a nested stream chain on later lines. See + [stream-examples.md](references/stream-examples.md) for final-shape helper patterns. ```java // Java 9+: flatten Optional values instead of filter(Optional::isPresent).map(Optional::get) @@ -90,23 +87,26 @@ exact file. Do not answer only in chat when a file artifact is requested. non-primitives, `toMap` with merge behavior and deliberate null-key/value handling, non-null keys for `groupingBy`, `partitioningBy` for boolean splits, and flattened nested indexes when clearer. Preserve duplicate-key merge rules, tie handling, null contracts, and map suppliers. Carry - `element + result`, never null sentinels. + `element + result`, never null sentinels. Extract collector merge helpers when duplicate-key + resolution needs branching, tie-breaking, or more than one comparison; do not hide those rules in + multi-line ternaries or block merge lambdas. 5. Preserve ordering, mutability, short-circuit behavior, and stream/collector semantics. 6. Keep imperative code when it is the clearer boundary for stateful output, checked IO, mutation-heavy logic, or complex early exits. 7. Verify changed branches for empty inputs, one element, duplicates, nulls, ordering, parallel-safety, and baseline compatibility. Run the marker scan from [hard-stops.md](references/hard-stops.md), fix hits, and re-scan. In scan audits, keep - hard-stop severities: required hits stay required unless explicitly acceptable. - -Generic lambda, method-reference, identity-function, no-op callback stage, supplier-laziness, and -callback readability guidance belongs to the companion package `martinfrancois/java-functional-style`. -Install both packages when stream cleanup also depends on non-trivial callback style. - -Review output: - -- Give a direct behavior-preserving decision plus one safe snippet. -- Create `review.md` when requested, even for rejection-only reviews. -- Explain code behavior, not internal workflow. -- Avoid internal workflow labels such as "per the skill", "hard stop", "marker", "scan", - "checklist", "rubric", or "criteria" unless the user asks about the workflow itself. + hard-stop severities: required hits stay required unless explicitly acceptable; also name + acceptable non-primitive reductions such as `BigDecimal.reduce(...)` when present. + +Generic lambda and callback-style guidance belongs to the companion package +`martinfrancois/java-functional-style`. Install both packages when stream cleanup depends on callback +style. When both are available, keep stream callbacks as glue: extract nested callback bodies and +filters with more than one domain condition to named helpers. + +Review output: give a direct behavior-preserving decision plus one safe snippet. For `review.md`, +write short prose first; avoid tables, extra snippets, style sections, redundant-output sections, and +internal workflow labels unless asked. For ordered lookup rejections, include: "Keep the existing +`sorted(...).filter(...).findFirst()` chain." When rejecting `parallelStream().findAny()`, say +parallelism makes ordered selection less predictable or more expensive here and no CPU-bound work or +measurement justifies the overhead. diff --git a/skills/java-streams/references/hard-stops.md b/skills/java-streams/references/hard-stops.md index 48cebce..7033e79 100644 --- a/skills/java-streams/references/hard-stops.md +++ b/skills/java-streams/references/hard-stops.md @@ -32,6 +32,11 @@ Fix these before finalizing: `reduce(BigDecimal.ZERO, BigDecimal::add)` as acceptable. - Nested `map(... stream ... collect(...)).flatMap(...)` where a direct `flatMap` stream chain is clearer. +- Nested `flatMap`, `Collectors.flatMapping`, or `anyMatch` callbacks whose bodies continue stream + chains across later lines. Extract stream-returning or boolean helpers so the outer chain reads as + glue while preserving flattening and short-circuit semantics. +- Stream `filter` predicates with more than one meaningful domain condition when a named predicate + helper would explain the rule, such as "undelivered and past due". - `filter(Optional::isPresent).map(Optional::get)` on Java 9+. Use `flatMap(Optional::stream)`. - `toMap` without a merge function when duplicate keys are possible. - `groupingBy` where null classifier keys can reach the collector. Treat this as a required fix, @@ -94,6 +99,9 @@ markers belong to `java-functional-style`, not this stream hard-stop scan. - When reviewing a proposed replacement of ordered selection with `findAny()` or `parallelStream()`, recommend keeping the existing ordered `sorted(...).filter(...).findFirst()` chain first. Mention `min(...)`/`max(...)` only as an optional refactor when the task asks for optimization advice. + For a `parallelStream().findAny()` proposal, explicitly state that parallelism makes the ordered + selection less predictable or more expensive here, and that no CPU-bound work or measurement + justifies that overhead. - Use `findAny()` only when the domain explicitly says all matching domain values, such as primary/default/preferred values, are equivalent and encounter order does not matter. Use `findFirst()` for first configured, first listed, chronological, priority, fallback, or @@ -131,6 +139,9 @@ input is large. Explain the rewrite in terms of ownership, correctness, and read low-level allocation details as secondary unless measurements make them relevant. Never describe ordinary `ArrayList` growth as `O(N^2)`; resizing is amortized O(N) total. In reviews, show the sequential direct-collection fix before any parallel version. +For external-mutation performance reviews, include the distinction in plain text: direct collection +fixes correctness/readability first; throughput still requires benchmarking a side-effect-free +sequential stream against a side-effect-free parallel stream on the real workload. When the workload is not clearly CPU-bound or the input is expected to be small/tiny, explicitly state that parallelization is not justified here. For large CPU-bound transformations, strongly recommend benchmarking a pure parallel version after @@ -170,6 +181,9 @@ stream-quality issues. If a marker remains because it is legitimate, state why. for allowed stream markers or allowed usages, also call out plain `count()` when it is the requested numeric result rather than a `count() > 0` existence check, and state that plain `count()` is not a hit for the bundled scan regex. +Also call out non-primitive reductions such as `reduce(BigDecimal.ZERO, BigDecimal::add)` as +acceptable when the requested/domain type is non-primitive; do not force primitive aggregation for +`BigDecimal`. In ordinary code reviews, do not expose internal workflow labels such as "hard stop", "marker", "scan", or "skill checklist" in headings, rationale, or recommendations. Use those terms only when diff --git a/skills/java-streams/references/stream-examples.md b/skills/java-streams/references/stream-examples.md index b4f8f91..39349ee 100644 --- a/skills/java-streams/references/stream-examples.md +++ b/skills/java-streams/references/stream-examples.md @@ -178,6 +178,7 @@ Useful review wording for this pattern: ```text The first fix is to remove the external mutation; let the stream produce the list directly. +That is a correctness/readability fix, not a guarantee that the sequential stream is faster. For throughput, benchmark the sequential version against a pure side-effect-free parallel stream on the real workload before relying on parallelStream. `parallelStream()` can be slower for small lists or mostly-small call paths. @@ -257,6 +258,66 @@ private static Session longerSession(Session first, Session second) { } ``` +For nested `flatMap` callbacks, extract the full flattened element shape. Do not leave a callback +that calls a helper and then continues the helper stream on later lines. If the collector needs keyed +values, make the helper return entries directly: + +```java +Map> emailsByTrack = conferences.stream() + .flatMap(ConferenceIndexes::optedInEmailEntries) + .collect(Collectors.groupingBy( + Map.Entry::getKey, + Collectors.mapping(Map.Entry::getValue, Collectors.toList()))); +``` + +For downstream `Collectors.flatMapping`, prefer a named helper when the nested stream filters or maps +values. The collector should read as stream glue: + +```java +Map> emailsByTrack = conferences.stream() + .flatMap(conference -> conference.sessions().stream()) + .filter(session -> session.track() != null) + .collect(Collectors.groupingBy( + Session::track, + Collectors.flatMapping( + SessionIndexes::optedInEmails, + Collectors.collectingAndThen( + Collectors.toCollection(TreeSet::new), + ArrayList::new)))); + +private static Stream optedInEmails(Session session) { + return session.registrations().stream() + .filter(Registration::optedIn) + .map(Registration::email) + .filter(Objects::nonNull); +} +``` + +For nested existence checks, extract the inner stream checks too. Keep `anyMatch` short-circuiting, +but do not stack multi-line callbacks: + +```java +boolean hasWaitlistedRegistration(List conferences) { + return conferences.stream() + .flatMap(conference -> conference.sessions().stream()) + .anyMatch(SessionIndexes::hasWaitlistedRegistration); +} + +private static boolean hasWaitlistedRegistration(Session session) { + return session.registrations().stream().anyMatch(Registration::waitlisted); +} +``` + +For filters with more than one domain condition, extract a named predicate helper instead of leaving +the combined condition inline: + +```java +List notices = shipments.stream() + .filter(shipment -> isOverdue(shipment, today)) + .map(shipment -> toNotice(shipment, today)) + .toList(); +``` + When a task says to use nested records or helper types in the requested file, keep those types in that file instead of splitting them into separate top-level files. @@ -333,6 +394,23 @@ List favoriteProducts = user.getFavoriteProducts().stream() .toList(); ``` +For a blocking yes/no service check, the same shape is a typed non-null carrier. Keep the +concurrency bound inside `Gatherers.mapConcurrent`; do not create a separate executor, future fan-out +pipeline, or service indirection: + +```java +List acceptedJobs = jobs.stream() + .gather(Gatherers.mapConcurrent( + 8, + job -> new ScheduleCheck(job, SchedulerApi.canAccept(job.token())))) + .filter(ScheduleCheck::accepted) + .map(ScheduleCheck::job) + .sorted(Comparator.comparing(Job::startsAt).thenComparing(Job::token)) + .toList(); + +record ScheduleCheck(Job job, boolean accepted) {} +``` + `Map.entry` is appropriate in this example because the baseline is Java 24 and neither side of the entry is null. Do not return `null` from a `mapConcurrent` mapper to mean "skip"; carry the element with an explicit boolean result, then filter and map afterward. If nulls can reach the carrier or