Your Name
77ae05c28c
The refactoring is now published as Slice on sammancoaching.org: https://sammancoaching.org/refactorings/slice.html - git mv ideas/split-loop-with-lambda -> ideas/slice-with-lambda - rename learning-hour file and update frontmatter name - update paths in tools/rehearse scripts and report title - update prose: 'Split Loop with Lambda' -> 'Slice with Lambda' in learning hour, exercise READMEs, storyboard, solution comments - link the published Slice refactoring from the pattern draft
354 lines
13 KiB
Markdown
354 lines
13 KiB
Markdown
# Slice with Lambda — storyboard / handout
|
|
|
|
Step-by-step mechanics of the **Slice with Lambda** refactoring (formerly called "Split Loop with Lambda"), one diagram per
|
|
step, each step followed by the exact code (full before/after, highlights as `←`
|
|
comments). Written to be sliced into slides later; until then it works as a handout.
|
|
|
|
Sources: `../zeroth-draft-of-refactoring-pattern.md` (the mechanics),
|
|
`../slice-with-lambda-learning-hour.md` (where we stop: after step 3 + the test),
|
|
`../exercise/rehearsal-findings.md` (the drift anti-pattern and the "both calls!"
|
|
caution). Java only. Pristine start: `../exercise/java`, end state: `../exercise/solution`.
|
|
C# demo examples live at `../exercise/csharp/` (MetaphorFree.cs, WeatherStation.cs, IceCreamShop.cs); the slides are at `~/qwan/slides-rjs/decks/refactoring/slice-with-lambda/slides.md`, served at http://localhost:9001/#/refactoring/slice-with-lambda.
|
|
|
|
## How to read the diagrams
|
|
|
|
|
|
|
|
---
|
|
|
|
## Step 0 — Where we start: one method, three loops, one unwanted dependency
|
|
|
|
**Goal:** see the problem before touching anything. `longRangeForecast()` resets, looks
|
|
up weather, and scores — and the weather part calls `scorer`, which stands for an
|
|
external weather service (it uses `Random`, so the approval test is red *and* flaky).
|
|
|
|
|
|
|
|
**Before:**
|
|
|
|
```java
|
|
public Map<IceCream, Integer> longRangeForecast() {
|
|
var result = new HashMap<IceCream, Integer>();
|
|
for (IceCream flavour : IceCream.values()) {
|
|
result.put(flavour, 0);
|
|
}
|
|
List<Boolean> expectedWeather = new ArrayList<>();
|
|
for (Instant interestingDate : DATES) {
|
|
scorer.updateSelection(); // ← unwanted dependency (void)
|
|
long daysForward = ChronoUnit.DAYS.between(ANCHOR_DATE, interestingDate);
|
|
Boolean weather = scorer.lookupWeather(daysForward); // ← unwanted dependency (value)
|
|
expectedWeather.add(weather);
|
|
}
|
|
for (IceCream flavour : result.keySet()) {
|
|
result.put(flavour, result.get(flavour) + 10);
|
|
var sunnyHolidays = expectedWeather.stream().filter(s -> s).count();
|
|
if (sunnyHolidays > 2 && flavour == IceCream.Vanilla) {
|
|
result.put(flavour, result.get(flavour) + 5);
|
|
}
|
|
if (sunnyHolidays > 1 && expectedWeather.get(1)) {
|
|
result.put(flavour, result.get(flavour) + 2);
|
|
}
|
|
}
|
|
return result;
|
|
}
|
|
```
|
|
|
|
**Checkpoint:**
|
|
|
|
- Run the approval test. It fails or flakes. Why? (`Scorer` calls `Random` — it *is*
|
|
the unwanted dependency.)
|
|
- Plain **Split Loop** can identify the three loops — but it cannot cut the weather
|
|
loop loose, because the thing you would introduce and slide is not a value, it is a
|
|
*call on an object you don't want*. That is where this pattern starts.
|
|
|
|
---
|
|
|
|
## Step 1 — Extract the dependency-using code into a lambda
|
|
|
|
**Goal:** make the laziness explicit. Instead of asking the weather service now, we
|
|
inject *a function that can answer the weather question when asked*. Both `scorer`
|
|
calls go inside the lambda — the void one too (see the anti-pattern slide for what
|
|
happens if you leave it behind). This step is a small manual edit: no refactoring tool
|
|
does it.
|
|
|
|
|
|
|
|
**After:**
|
|
|
|
```java
|
|
public Map<IceCream, Integer> longRangeForecast() {
|
|
var result = new HashMap<IceCream, Integer>();
|
|
for (IceCream flavour : IceCream.values()) {
|
|
result.put(flavour, 0);
|
|
}
|
|
List<Boolean> expectedWeather = new ArrayList<>();
|
|
LongFunction<Boolean> dayOffsetToWeather = daysForward -> { // ← NEW: the lambda...
|
|
scorer.updateSelection(); // ← ...holds BOTH scorer calls
|
|
return scorer.lookupWeather(daysForward);
|
|
};
|
|
for (Instant interestingDate : DATES) {
|
|
long daysForward = ChronoUnit.DAYS.between(ANCHOR_DATE, interestingDate);
|
|
expectedWeather.add(dayOffsetToWeather.apply(daysForward)); // ← invocation replaced lookupWeather
|
|
}
|
|
for (IceCream flavour : result.keySet()) {
|
|
// ... scoring loop unchanged ...
|
|
}
|
|
return result;
|
|
}
|
|
```
|
|
|
|
(IDE will offer to add `import java.util.function.LongFunction;` — take it.)
|
|
|
|
**Checkpoint:**
|
|
|
|
- Compiles; behaviour unchanged; test still flaky — the lambda still calls `Scorer`
|
|
for now, that is fine.
|
|
- The lambda's parameter (`daysForward`) is *only* in scope inside the lambda. The
|
|
loop still computes its own `daysForward` and hands it to the invocation.
|
|
|
|
---
|
|
|
|
## Step 2 — Slide the lambda definition to the top of the method
|
|
|
|
**Goal:** *slide, don't move.* The **definition** of the lambda slides to the top of
|
|
the method; the **invocation** (`.apply(...)`) stays exactly where the code used to
|
|
be. This is the Split Loop "slide statement" move — except what slides is a
|
|
function, not a value.
|
|
|
|
|
|
|
|
**After:**
|
|
|
|
```java
|
|
public Map<IceCream, Integer> longRangeForecast() {
|
|
LongFunction<Boolean> dayOffsetToWeather = daysForward -> { // ← SLID to the top...
|
|
scorer.updateSelection();
|
|
return scorer.lookupWeather(daysForward);
|
|
};
|
|
var result = new HashMap<IceCream, Integer>();
|
|
for (IceCream flavour : IceCream.values()) {
|
|
result.put(flavour, 0);
|
|
}
|
|
List<Boolean> expectedWeather = new ArrayList<>();
|
|
for (Instant interestingDate : DATES) {
|
|
long daysForward = ChronoUnit.DAYS.between(ANCHOR_DATE, interestingDate);
|
|
expectedWeather.add(dayOffsetToWeather.apply(daysForward)); // ← ...invocation stays here
|
|
}
|
|
for (IceCream flavour : result.keySet()) {
|
|
// ... scoring loop unchanged ...
|
|
}
|
|
return result;
|
|
}
|
|
```
|
|
|
|
**Checkpoint:**
|
|
|
|
- Compiles; behaviour unchanged; test still flaky.
|
|
- Read the method out loud: everything it needs from outside the weather loop is
|
|
now "given a day offset, is it sunny?" — the loop body itself no longer mentions
|
|
`Scorer`.
|
|
|
|
---
|
|
|
|
## Step 3 — Extract the weather loop; the lambda becomes a parameter
|
|
|
|
**Goal:** select from the `expectedWeather` declaration through the end of the
|
|
weather loop and Extract Method. The IDE infers the parameter type
|
|
(`LongFunction<Boolean>`) from the lambda variable. The extracted method has no
|
|
field dependencies left — `DATES` and `ANCHOR_DATE` are constants — so it can be
|
|
`static` right away.
|
|
|
|
|
|
|
|
**Before:** end state of step 2 (above).
|
|
|
|
**After:**
|
|
|
|
```java
|
|
public Map<IceCream, Integer> longRangeForecast() {
|
|
LongFunction<Boolean> dayOffsetToWeather = daysForward -> {
|
|
scorer.updateSelection();
|
|
return scorer.lookupWeather(daysForward);
|
|
};
|
|
var result = new HashMap<IceCream, Integer>();
|
|
for (IceCream flavour : IceCream.values()) {
|
|
result.put(flavour, 0);
|
|
}
|
|
List<Boolean> expectedWeather = collectExpectedWeather(dayOffsetToWeather); // ← EXTRACTED
|
|
for (IceCream flavour : result.keySet()) {
|
|
result.put(flavour, result.get(flavour) + 10);
|
|
var sunnyHolidays = expectedWeather.stream().filter(s -> s).count();
|
|
if (sunnyHolidays > 2 && flavour == IceCream.Vanilla) {
|
|
result.put(flavour, result.get(flavour) + 5);
|
|
}
|
|
if (sunnyHolidays > 1 && expectedWeather.get(1)) {
|
|
result.put(flavour, result.get(flavour) + 2);
|
|
}
|
|
}
|
|
return result;
|
|
}
|
|
|
|
private static List<Boolean> collectExpectedWeather( // ← NEW — and already static
|
|
LongFunction<Boolean> dayOffsetToWeather) {
|
|
var expectedWeather = new ArrayList<Boolean>();
|
|
for (Instant interestingDate : DATES) {
|
|
long daysForward = ChronoUnit.DAYS.between(ANCHOR_DATE, interestingDate);
|
|
expectedWeather.add(dayOffsetToWeather.apply(daysForward));
|
|
}
|
|
return expectedWeather;
|
|
}
|
|
```
|
|
|
|
**Checkpoint — the drift test.** This is where the refactoring can silently fail:
|
|
|
|
- `grep -n scorer src/main/java/codingdojo/LongRangeForecasts.java` — the new method
|
|
must contain **no** hits.
|
|
- Try to make `collectExpectedWeather` `static` (it already is here). If any field
|
|
reference remained, it would not compile.
|
|
- If `scorer.updateSelection()` is still in the loop instead of inside the lambda,
|
|
the dependency travelled *with* the extraction and the split is cosmetic. Put it
|
|
in the lambda and start this step again.
|
|
|
|
### Inset — the overload: let the λ come from outside
|
|
|
|
**Goal:** right now the lambda is a local variable — the test still cannot reach the
|
|
seam. Change Signature on the big method: *add* the lambda as a parameter and *drop*
|
|
the local. Then give callers a no-arg overload that bakes in the "real" lambda.
|
|
|
|
|
|
|
|
**After:**
|
|
|
|
```java
|
|
public Map<IceCream, Integer> longRangeForecast() { // ← no-arg overload: callers unchanged
|
|
return longRangeForecast(daysForward -> {
|
|
scorer.updateSelection();
|
|
return scorer.lookupWeather(daysForward);
|
|
});
|
|
}
|
|
|
|
public Map<IceCream, Integer> longRangeForecast( // ← the seam: λ is a parameter
|
|
LongFunction<Boolean> dayOffsetToWeather) {
|
|
var expectedWeather = collectExpectedWeather(dayOffsetToWeather);
|
|
// ... reset loop + scoring loop, unchanged ...
|
|
return result;
|
|
}
|
|
```
|
|
|
|
**Checkpoint:** compiles; existing callers (and the old flaky test, if you keep it)
|
|
still work through the no-arg overload.
|
|
|
|
---
|
|
|
|
## Step 5-lite — test through the seam with a pure lambda
|
|
|
|
**Goal:** replace the weather service with a pure function in the test. Deterministic,
|
|
glanceable, and `Scorer` is never touched.
|
|
|
|
|
|
|
|
**The test (end of document version — this is the whole thing):**
|
|
|
|
```java
|
|
package codingdojo;
|
|
|
|
import org.approvaltests.Approvals;
|
|
import org.junit.jupiter.api.Test;
|
|
|
|
import java.util.ArrayList;
|
|
import java.util.Collections;
|
|
import java.util.Map;
|
|
import java.util.function.LongFunction;
|
|
|
|
public class LongRangeForecastsTest {
|
|
|
|
@Test
|
|
void longRangeForecast() {
|
|
// pure weather implementation: every holiday is sunny
|
|
LongFunction<Boolean> alwaysSunny = daysForward -> true;
|
|
var actual = new LongRangeForecasts().longRangeForecast(alwaysSunny);
|
|
Approvals.verify(printIceCreamForecast(actual));
|
|
}
|
|
|
|
private static String printIceCreamForecast(Map<IceCream, Integer> actual) {
|
|
var printedResult = new StringBuilder();
|
|
printedResult.append("{");
|
|
var sortedKeys = new ArrayList<>(actual.keySet());
|
|
Collections.sort(sortedKeys);
|
|
for (IceCream key : sortedKeys) {
|
|
printedResult.append(key);
|
|
printedResult.append(":");
|
|
printedResult.append(actual.get(key));
|
|
printedResult.append(", ");
|
|
}
|
|
printedResult.append("}");
|
|
return printedResult.toString();
|
|
}
|
|
}
|
|
```
|
|
|
|
Approved output:
|
|
|
|
```
|
|
{Strawberry:12, Chocolate:12, Vanilla:17, }
|
|
```
|
|
|
|
**Checkpoint:** `mvn test` is green — deterministically. Nothing in the test path
|
|
touches `Scorer`. *What just became testable that was not before?*
|
|
|
|
---
|
|
|
|
## Debrief
|
|
|
|
### The dead call the lambda exposed
|
|
|
|
|
|
|
|
`scorer.updateSelection()` re-rolls `Scorer.flavour` — state that nothing in this
|
|
method ever reads (its only real consumer is the unrelated `salesForecast()`; it is
|
|
copy-paste residue in the long-range loop). Delete it from the lambda: the approved
|
|
file does not change. The refactoring didn't just hide the dependency — it exposed
|
|
that one of the two calls was dead all along.
|
|
|
|
### Anti-pattern — the drift: scorer left behind in the extracted method
|
|
|
|
|
|
|
|
If you lambda-ize only the value-producing call and leave `updateSelection()` as
|
|
"loop machinery", the extraction carries the `scorer` field into the new method:
|
|
|
|
```java
|
|
// DRIFTED — the method still depends on the field; step 4 (move it
|
|
// to another class) would be blocked, and the split is cosmetic.
|
|
private List<Boolean> collectExpectedWeather(
|
|
LongFunction<Boolean> dayOffsetToWeather) {
|
|
var expectedWeather = new ArrayList<Boolean>();
|
|
for (Instant interestingDate : DATES) {
|
|
scorer.updateSelection(); // ← LEFT BEHIND
|
|
long daysForward = ChronoUnit.DAYS.between(ANCHOR_DATE, interestingDate);
|
|
expectedWeather.add(dayOffsetToWeather.apply(daysForward));
|
|
}
|
|
return expectedWeather;
|
|
}
|
|
```
|
|
|
|
Worse: the test still passes (the stray call only mutates state nothing reads), so
|
|
**green ≠ drift-free**. Catch it mechanically: `grep scorer` on the extracted method
|
|
must return nothing, or try to make it `static` — it won't compile with a field
|
|
reference.
|
|
|
|
---
|
|
|
|
## Next moves (not storyboarded yet)
|
|
|
|
These are steps 4 and 6 of the full pattern — the learning hour stops before them:
|
|
|
|
- **Step 4:** move `collectExpectedWeather` to a class where `Scorer` is unknown.
|
|
(It is already `static` and scorer-free — the move is now unblocked.)
|
|
- **Step 6:** more pure-lambda combinations are now one line each:
|
|
`daysForward -> false` (no sunny holidays → all flavours 10),
|
|
`daysForward -> daysForward == 5` (exactly one sunny holiday, not at index 1 → all 10).
|
|
- The design view: the lambda is the smallest **Port** (function signature) +
|
|
**Adapter** (the extracted code with the unwanted dependency) that could work —
|
|
see the Role Interface discussion in the pattern draft.
|