Code Smells & Refactoring — A Music Streaming App

Identify smells in real Python code and apply named refactorings safely. The first refactoring you do by hand to anchor the safety dance; the rest you drive with Monaco's tool support so you can spend your time thinking instead of typing. Tests stay green throughout. Live UML class diagrams make the structural change visible. Built around a music streaming codebase you incrementally clean up over ten steps.

1

The Cost of Duplication You Didn't Notice

Welcome — what this tutorial trains

You’re a CS2 student who already knows Python and pytest. You haven’t yet learned the discipline of changing existing code without breaking it. That discipline has a name — refactoring — and a lot of structure to it. Over the next ten steps you’ll learn:

  • How to recognize a handful of high-impact code smells in real Python.
  • How to apply named refactorings that fix each smell safely.
  • How tests give you the safety net to change structure without changing behavior.
  • How to judge when a refactoring is worth doing, and when it isn’t.

The codebase grows over the tutorial: you start with three small functions and end with a small music streaming app. Most of the typing is done by Monaco’s refactoring tools — you’ll select code, pick a refactoring, and judge the diff. The thinking is yours.

Prerequisite: Testing Foundations — pytest discovery, assert, and @pytest.mark.parametrize. If those feel new, do that one first.

Step 1 — A Bug, in Triplicate

Learning objective. After this step you will be able to (a) define refactoring as behavior-preserving structural change, and (b) explain operationally why duplicated code multiplies the cost of every future bug.

Open royalty.py. The RoyaltyCalculator class has three methods that calculate the creator’s share of streaming royalties for three different track types — songs, podcasts, audiobooks. They all use the same formula: plays × rate × 0.7 (the platform takes 30%, creators get 70%).

class RoyaltyCalculator:
    def royalty_song(self, plays: int, rate: float) -> float:
        return plays * rate * 0.7

    def royalty_podcast(self, plays: int, rate: float) -> float:
        return plays + rate * 0.7   # ← bug

    def royalty_audiobook(self, plays: int, rate: float) -> float:
        return plays + rate * 0.7   # ← bug

Two of the three have the same bug — + where there should be *. The bug ships unnoticed because the test suite only covers royalty_song. (You can already see the class in the UML diagram in the bottom-left — three methods, one box.)

Your task

  1. Run the existing test (test_royalty.py). It passes — royalty_song is correct.
  2. Extend the parametrize table in test_royalty.py to cover royalty_podcast and royalty_audiobook with at least two (plays, rate) cases each. Use the same formula plays * rate * 0.7 to compute the expected values.
  3. Run again. Two tests will turn red.
  4. Fix the bug in royalty.py. Notice that you have to fix it in two separate places because the logic is duplicated.

You will not yet refactor the duplication away. That waits until Step 4. The point of this step is to feel the cost: a single bug, fixed in N places, where N is the number of duplicates.

Refactoring, defined

Throughout this tutorial we use Martin Fowler’s definition (Refactoring, 2018):

Refactoring is a disciplined technique for restructuring an existing body of code, altering its internal structure without changing its external behavior.

The two halves are equally important. Structural change without behavior preservation is a rewrite; behavior preservation without structural change is unnecessary churn. A refactoring is both at once.

Not every triplet is a smell

Visual similarity isn’t the diagnostic — bug-coupling is. Look at this counter-example:

def round_currency_usd(amount: float) -> float:
    return round(amount, 2)            # cents: 2 decimal places

def round_currency_jpy(amount: float) -> float:
    return round(amount, 0)            # yen: no fractional unit

def round_currency_kwd(amount: float) -> float:
    return round(amount, 3)            # Kuwaiti dinar: 3 decimal places

Three near-identical functions, but each captures a real domain rule (ISO 4217 minor-unit precision per currency). Consolidating these into one function with a precision parameter would not remove a bug coupling — there are no shared bugs to fix in three places, because each precision is independent. The visual similarity is a coincidence of the API shape, not duplicated knowledge.

The rule of thumb: if changing one of the functions would also force you to change the others (in lockstep), that’s duplication. If they evolve independently, leave them alone. Step 4 returns to this trade-off — for now, hold the distinction in mind.

Starter files
royalty.py
"""Streaming royalty calculations.

The platform takes 30% commission; creators get 70% of plays * rate.
"""


class RoyaltyCalculator:
    """Computes creator royalty payouts per track type."""

    def royalty_song(self, plays: int, rate: float) -> float:
        return plays * rate * 0.7

    def royalty_podcast(self, plays: int, rate: float) -> float:
        return plays + rate * 0.7

    def royalty_audiobook(self, plays: int, rate: float) -> float:
        return plays + rate * 0.7
payouts.py
"""Monthly creator-payout aggregator — uses RoyaltyCalculator across all track types.

This is the *production caller* that exercises the three royalty methods.
The bug in royalty_podcast / royalty_audiobook propagates through here:
MonthlyPayouts.total_creator_earnings will return wildly wrong totals
until the underlying calculator is fixed.
"""
from typing import List, Tuple
from royalty import RoyaltyCalculator


class MonthlyPayouts:
    """Aggregates monthly creator earnings across songs, podcasts, audiobooks."""

    def __init__(self, calculator: RoyaltyCalculator) -> None:
        self.calculator: RoyaltyCalculator = calculator

    def total_creator_earnings(
        self,
        song_plays: List[Tuple[int, float]],
        podcast_plays: List[Tuple[int, float]],
        audiobook_plays: List[Tuple[int, float]],
    ) -> float:
        """Sum royalties across all three track types for one month."""
        total: float = 0.0
        for plays, rate in song_plays:
            total += self.calculator.royalty_song(plays, rate)
        for plays, rate in podcast_plays:
            total += self.calculator.royalty_podcast(plays, rate)
        for plays, rate in audiobook_plays:
            total += self.calculator.royalty_audiobook(plays, rate)
        return total
test_royalty.py
"""Tests for streaming royalty calculations and the MonthlyPayouts caller."""
import pytest
from royalty import RoyaltyCalculator
from payouts import MonthlyPayouts


@pytest.fixture
def calc() -> RoyaltyCalculator:
    return RoyaltyCalculator()


@pytest.mark.parametrize("plays,rate,expected", [
    (100, 0.01, 0.7),
    (1000, 0.005, 3.5),
])
def test_royalty_song(calc: RoyaltyCalculator, plays: int, rate: float, expected: float) -> None:
    assert calc.royalty_song(plays, rate) == pytest.approx(expected)


# TODO: extend the parametrize table to cover royalty_podcast and
# royalty_audiobook with at least two cases each. Use the formula
# plays * rate * 0.7 to compute the expected values.


def test_monthly_payouts_sums_across_track_types(calc: RoyaltyCalculator) -> None:
    """Integration test: the bug in any royalty_* method also breaks this aggregate."""
    payouts: MonthlyPayouts = MonthlyPayouts(calc)
    total: float = payouts.total_creator_earnings(
        song_plays=[(100, 0.01), (1000, 0.005)],   # 0.7 + 3.5  = 4.2
        podcast_plays=[(100, 0.02)],               # 1.4        = 1.4
        audiobook_plays=[(200, 0.02)],             # 2.8        = 2.8
    )
    assert total == pytest.approx(8.4)

if __name__ == "__main__":
    import sys
    pytest.main([sys.argv[0], "-v"])

Step 1 — The Cost of Duplication

Min. score: 70%

1. Retrieval. Which operator was wrong in two of the three royalty functions, and what should it have been?

  • + was wrong; should have been *
  • * was wrong; should have been +

    The formula in the docstring is plays × rate × 0.7 — multiplication is correct. The bug was the reverse: two functions had + where the formula calls for *.

  • / was wrong; should have been *
  • - was wrong; should have been +

The docstring stated the formula as plays × rate × 0.7. Two of the three functions had plays + rate * 0.7, which silently returns garbage values for any non-trivial input. Naming the bug operator from memory anchors the smell — when you next see “three near-identical functions,” the very next thought should be “what bug might be hiding in just two of them?”

2. Which of the following best matches Fowler’s definition of refactoring?

  • Rewriting code from scratch in a cleaner style

    A rewrite typically does change behavior in subtle ways and starts from a blank slate. Refactoring is incremental and behavior-preserving — the external contract is unchanged.

  • Restructuring existing code without changing its external behavior
  • Adding new features while improving the existing code

    Adding features mixes refactoring with feature work. Discipline says do one or the other — refactor under the safety net of tests, then add the feature on the cleaner foundation.

  • Removing unused code paths and dead branches

    Removing dead code can be part of a refactor, but the term refers more broadly to any structural improvement that preserves behavior, not specifically to deletion.

Refactoring has two halves: structural change AND behavior preservation. Without the second half, it’s a rewrite. Without the first, it’s churn. Both halves are non-negotiable.

3. You discover the same bug in three near-identical copies of a function. Which order do you apply?

  • Refactor first to remove the duplication, then fix the bug in one place

    Tempting, but dangerous: extracting the common code with the bug intact would propagate the buggy formula into the helper. The duplication would be gone, but the bug would remain. Fix-then-refactor preserves the safety property: every step has a passing test suite.

  • Fix the bug in all three places, then refactor to remove the duplication
  • Fix in one place; let the duplication remain — the others are unused
  • Either order is equivalent; the result is identical code

    Order matters because of the test safety net. Fix-then-refactor verifies the fix in each location with a green test before changing structure. Refactor-then-fix can hide the original bug inside the new abstraction.

Fix-then-refactor. Fix the bug in each duplicate first, with tests passing for each location. Only then refactor to consolidate — the consolidated helper now contains the corrected formula. This is one of the most useful sequencing rules in the refactoring toolkit.

4. Three near-identical functions, one bug introduced in two of them. How many places did you change to fix it?

  • 1 — the test suite caught it once

    The original test passed only because it covered one of the three functions. Once the table was extended, two new failures appeared — one per buggy duplicate.

  • 2 — the same bug appeared in two places
  • 3 — every duplicate had to be inspected

    All three functions had to be inspected to confirm correctness, but only two needed code changes. The cost-of-duplication count is the number of places that changed, which is the count of duplicates that contained the bug.

  • 0 — the original test passed without any changes

The cost of duplication is empirical: it equals the number of duplicates that contain the same bug. Step 4 will introduce Extract Function to consolidate three places into one, so the next bug is found and fixed exactly once.

2

Long Method → Extract Function (by hand)

Long Method, the smell

Learning objective. After this step you will be able to (a) recognize a Long Method by its sub-goal structure, (b) apply Extract Function while keeping tests green, and (c) explain the safety dance — the test discipline that makes refactoring safe.

Open player.py. The class Streaming has a method process_play_event(user, track) that is 35 lines long and does five different things:

  1. Verify the user’s subscription
  2. Check that the track is available in the user’s region
  3. Compute royalty for the play
  4. Update the play count
  5. Append to the user’s history

The comments label these sub-goals — that’s a hint that the method is doing too much. A coherent method does one thing and is named for it. A method that does five things needs five names, which means five separate methods.

The safety dance

Refactoring without tests is a coin flip. Refactoring with tests is reliable. The discipline:

Step Action
1 Run the tests. They should pass. Now you have a baseline.
2 Make one structural change. Do not change behavior.
3 Run the tests again. They must still pass. If they fail, the change broke something — undo and try again.
4 Repeat from (2) until the structure is clean.

The rhythm is green → change → green → change → green. You are never more than one undo away from a known-good state.

Predict before reading

The first sub-goal of process_play_event is the subscription check (lines tagged # 1.). Before you read the worked example below, write down the function name you would choose for that block. One word or short phrase. Hold your choice in mind. After you see the worked example, compare to the name we chose — and ask yourself whether yours is clearer, equivalent, or worse.

Naming is the load-bearing micro-decision in every Extract Function. Generating a name yourself first — even if you change it after — anchors the schema better than reading our name first.

Worked example — extracting the first sub-goal

Look at the first comment block in process_play_event:

# 1. Verify the user's subscription
if user.subscription_tier == "free":
    if user.daily_plays >= 5:
        raise PermissionError("Free tier daily limit reached")
elif user.subscription_tier == "premium":
    pass  # unlimited
else:
    raise ValueError(f"Unknown tier: {user.subscription_tier}")

Five lines doing one thing. That’s a perfect Extract Method candidate.

The mechanics, narrated.

  1. Cut the five lines.
  2. Write a new helper method on the Streaming class: 
    def _check_subscription(self, user: User) -> None:
    """Raise an exception if the user is not allowed to play."""
    if user.subscription_tier == "free":
        if user.daily_plays >= 5:
            raise PermissionError("Free tier daily limit reached")
    elif user.subscription_tier == "premium":
        pass  # unlimited
    else:
        raise ValueError(f"Unknown tier: {user.subscription_tier}")
  3. Replace the cut block in process_play_event with a call: 
    self._check_subscription(user)
  4. Run the tests. Green.

Three things to notice:

  • The helper has a type-annotated signature. user: User and the -> None return tell future readers what it consumes and what it produces. The annotation is part of the refactoring, not optional polish.
  • The helper has a leading underscore in its name. Convention: _name means “internal — not part of the public API.” process_play_event is public; helpers it depends on are internal.
  • The helper’s name describes what it does, not how. _check_subscription is a coherent sub-goal; _block_one would not be.

Your task

Two more sub-goals are still inline in process_play_event. Extract them as methods on Streaming. The file already has the worked-example extraction applied; you continue from there.

  • Sub-goal 2: geo-restriction. Extract _check_geo_restriction(self, user: User, track: Track) -> None (raises on failure).
  • Sub-goal 3: royalty calculation. Extract _compute_royalty(self, track: Track) -> float returning the royalty amount.

Run the tests after each extraction. They should stay green throughout. Watch the UML diagram in the bottom-left grow with each extraction — Streaming gains methods, process_play_event’s body shrinks.

Why we do this manually one time. Steps 4–8 will use Monaco’s tool to do extractions for you with two clicks. Doing one slow extraction by hand right now anchors the mechanics in your fingers. When the tool acts later, you’ll recognize what it did because you’ll have done it yourself.

Starter files
player.py
"""The streaming player — orchestrates a single play event."""
from dataclasses import dataclass, field
from typing import List


@dataclass
class User:
    user_id: str
    subscription_tier: str   # "free" or "premium"
    region: str
    daily_plays: int = 0
    history: List[str] = field(default_factory=list)


@dataclass
class Track:
    track_id: str
    title: str
    duration_sec: int
    available_regions: List[str]
    rate: float
    play_count: int = 0


class Streaming:
    """Orchestrates streaming play events for users and tracks."""

    def _check_subscription(self, user: User) -> None:
        """Raise if the user is not allowed to play right now."""
        if user.subscription_tier == "free":
            if user.daily_plays >= 5:
                raise PermissionError("Free tier daily limit reached")
        elif user.subscription_tier == "premium":
            pass  # unlimited
        else:
            raise ValueError(f"Unknown tier: {user.subscription_tier}")

    def process_play_event(self, user: User, track: Track) -> float:
        """Run one play: returns the royalty paid to the creator."""
        # 1. Verify the user's subscription
        self._check_subscription(user)

        # 2. Geo-restriction check
        if track.available_regions and user.region not in track.available_regions:
            raise PermissionError(f"Track not available in {user.region}")

        # 3. Compute royalty (creator gets 70%)
        royalty: float = 1 * track.rate * 0.7

        # 4. Update play count
        track.play_count += 1
        user.daily_plays += 1

        # 5. Append to user's history
        user.history.append(track.track_id)

        return royalty
playback_session.py
"""A playback session runs multiple plays for one user.

Production caller for Streaming.process_play_event. Behavior must be
preserved across the Long-Method extraction in Step 2 — the body of
process_play_event changes shape internally, but its contract (royalty
per play, raises on subscription/geo violation) stays identical.
"""
from typing import List
from player import Streaming, User, Track


class PlaybackSession:
    """Plays a sequence of tracks and accumulates the total royalty paid."""

    def __init__(self, streaming: Streaming, user: User) -> None:
        self.streaming: Streaming = streaming
        self.user: User = user
        self.total_royalty: float = 0.0

    def run_session(self, tracks: List[Track]) -> float:
        """Play each track in order; return cumulative royalty."""
        for track in tracks:
            self.total_royalty += self.streaming.process_play_event(self.user, track)
        return self.total_royalty
test_player.py
"""Behavior tests — lock the contract before refactoring."""
import pytest
from typing import List
from player import User, Track, Streaming
from playback_session import PlaybackSession


@pytest.fixture
def streaming() -> Streaming:
    return Streaming()


@pytest.fixture
def free_user() -> User:
    return User(user_id="u1", subscription_tier="free", region="US", daily_plays=2)


@pytest.fixture
def premium_user() -> User:
    return User(user_id="u2", subscription_tier="premium", region="EU")


@pytest.fixture
def track_us_only() -> Track:
    return Track(track_id="t1", title="Song A", duration_sec=180,
                 available_regions=["US"], rate=0.01)


@pytest.fixture
def track_global() -> Track:
    return Track(track_id="t2", title="Song B", duration_sec=240,
                 available_regions=[], rate=0.02)


def test_premium_user_global_track_pays_royalty(streaming: Streaming,
                                                premium_user: User,
                                                track_global: Track) -> None:
    # Premium has no daily limit; global track has no geo restriction.
    royalty: float = streaming.process_play_event(premium_user, track_global)
    assert royalty == pytest.approx(0.014)
    assert track_global.play_count == 1
    assert premium_user.daily_plays == 1
    assert premium_user.history == ["t2"]


def test_free_user_under_limit_pays_royalty(streaming: Streaming,
                                            free_user: User,
                                            track_us_only: Track) -> None:
    royalty: float = streaming.process_play_event(free_user, track_us_only)
    assert royalty == pytest.approx(0.007)
    assert track_us_only.play_count == 1
    assert free_user.daily_plays == 3


def test_free_user_at_daily_limit_blocked(streaming: Streaming,
                                          free_user: User,
                                          track_us_only: Track) -> None:
    free_user.daily_plays = 5
    with pytest.raises(PermissionError, match="daily limit"):
        streaming.process_play_event(free_user, track_us_only)


def test_geo_restriction_blocks_play(streaming: Streaming,
                                     premium_user: User,
                                     track_us_only: Track) -> None:
    with pytest.raises(PermissionError, match="not available"):
        streaming.process_play_event(premium_user, track_us_only)


# ---- Caller test: PlaybackSession exercises process_play_event ----

def test_playback_session_accumulates_royalty(
    streaming: Streaming, premium_user: User, track_global: Track
) -> None:
    session: PlaybackSession = PlaybackSession(streaming, premium_user)
    tracks: List[Track] = [track_global, track_global, track_global]
    total: float = session.run_session(tracks)
    # 3 plays × 0.014 royalty per play
    assert total == pytest.approx(0.014 * 3)
    assert track_global.play_count == 3
    assert premium_user.daily_plays == 3

if __name__ == "__main__":
    import sys
    pytest.main([sys.argv[0], "-v"])

Step 2 — Long Method

Min. score: 70%

1. Retrieval. What’s the minimum test discipline a refactoring requires?

  • Run the tests once at the end to confirm nothing broke

    Running tests only at the end means a single failure indicates some change broke behavior — but not which one. Refactoring relies on small steps, each individually verified.

  • Run the tests before AND after each structural change
  • Write new tests after each refactoring to cover the new structure

    Refactoring is by definition behavior-preserving — the existing tests should still cover the unchanged behavior. New tests follow new behavior, not refactorings.

  • No tests are needed if the IDE supports the refactoring

    Even tool-driven refactorings can fail in edge cases (missed call sites, dynamic references). The test suite is the safety net regardless of how the structural change happens.

Green → change → green is the rhythm. Each structural change happens between two green test runs; if the second run fails, you know exactly which change to revert.

2. Which of the following is the best sign that a function is too long?

  • It exceeds 50 lines

    Line count is a heuristic, not a rule. A 60-line function with one coherent purpose is fine; a 20-line function with five sub-goals is too long.

  • It contains nested loops or conditionals

    Nested control flow is its own smell (Complex Class), but a function can be long and flat — many sequential blocks rather than nesting.

  • Its body has multiple sub-goals labeled with comments
  • It uses no helper functions of its own

    Many short functions delegate to helpers; many short functions don’t. The presence of helpers is independent of length.

Comments-as-section-headers are the diagnostic. When a reader needs # 1. Validate, # 2. Compute, # 3. Format to navigate a function, the function is doing those three things and should be three functions. The comment names become the new function names.

3. You add a comment to a long method explaining what each section does. Have you fixed the smell?

  • Yes — the function is now self-documenting

    Comments help readers; they don’t change the structural problem. The function still has five sub-goals jammed together; the next change still risks breaking unrelated logic.

  • Yes, as long as each comment is one line

    Length doesn’t change the lesson. A one-line comment can label a sub-goal, but the sub-goal is still inline. Extract the function — let the function’s name be the comment.

  • No — comments document a smell that should be fixed structurally
  • Only if the comments use a standard format like reStructuredText

Comments-as-deodorant. Section-header comments are a sign the code wants to be split. The fix isn’t tighter comments; it’s named functions whose call sites read like an outline. After Extract Function, the comment in the call site is the helper’s name.

4. After extracting one sub-goal into a helper, you run the tests. They pass. What do you do next?

  • Commit the change and continue with the next sub-goal
  • Run the tests again to be sure

    Re-running the same test command rarely surfaces new information. The first green run is the signal.

  • Manually inspect the helper for any subtle differences from the original

    If the test passed, the extraction preserved behavior. Suspicion that the test is too weak points to a separate problem (oracle strength) — manually inspecting every change papers over the gap rather than addressing it.

  • Add a new test for the helper before extracting more sub-goals

Trust the tests, then move on. The whole point of behavior preservation tests is that they make manual inspection unnecessary. Each green run is a small commit point. Adding new tests for the helper is optional — the existing tests cover the public behavior, which is what matters.

3

Boolean Anti-Patterns — and the trap that costs you everything

Three boolean smells

Learning objective. After this step you will be able to (a) simplify three common boolean anti-patterns, (b) recognize the IfsMerged trap — a dangerous “simplification” that silently drops a branch — and (c) use a @pytest.mark.parametrize truth table to detect behavior change in boolean code.

Open gates.py. The PlaybackGates class has three small methods that guard playback. All three have boolean smells. Two are safe to simplify; one looks safe but isn’t.

Sub-task A — boolean return

def is_trial_expired(self, free_trial_days_left: int) -> bool:
    if free_trial_days_left <= 0:
        return True
    else:
        return False

The body is if condition: return True else: return False. The condition is already a boolean — wrapping it in an if/else does nothing. Simplify to:

def is_trial_expired(self, free_trial_days_left: int) -> bool:
    return free_trial_days_left <= 0

This is the most common conditional anti-pattern in novice Python — about 30% of conditional anti-patterns in one large empirical study (Naude et al., 2024).

Sub-task B — confusing else

def stream_quality(self, is_premium: bool) -> str:
    if is_premium:
        return "high"
    if not is_premium:
        return "low"
    return "low"  # unreachable, but the type checker insists

Two ifs with mutually-exclusive conditions and a “just in case” return at the end. The intent is if/else. Simplify to:

def stream_quality(self, is_premium: bool) -> str:
    if is_premium:
        return "high"
    else:
        return "low"

Sub-task C — the IfsMerged trap

Here’s the method:

def play_decision(self, is_logged_in: bool, is_offline: bool) -> str:
    if is_logged_in:
        if not is_offline:
            return "stream"
        else:
            return "play_cached"
    return "error_login_required"

A reasonable-looking simplification:

def play_decision(self, is_logged_in: bool, is_offline: bool) -> str:
    if is_logged_in and not is_offline:
        return "stream"
    return "error_login_required"

It compiles. The happy-path test passes. Looks clean. It’s wrong.

The original returned "play_cached" for (is_logged_in=True, is_offline=True). The simplified version returns "error_login_required" for the same input. We’ve silently dropped a branch.

Why a happy-path test misses this

A single test like assert gates.play_decision(True, False) == "stream" exercises one of four possible truth-value combinations of (is_logged_in, is_offline). Three remain unchecked, and the bug lives in one of those three.

The fix is a truth table — a parametrize over all four combinations:

@pytest.mark.parametrize("is_logged_in,is_offline,expected", [
    (False, False, "error_login_required"),
    (False, True,  "error_login_required"),
    (True,  False, "stream"),
    (True,  True,  "play_cached"),
])
def test_play_decision(gates: PlaybackGates, is_logged_in: bool, is_offline: bool, expected: str) -> None:
    assert gates.play_decision(is_logged_in, is_offline) == expected

Four rows. Two columns of input. One expected output per row. Any behavior change in the function shows up somewhere in the table.

Mini warmup — parametrize in 5 lines. If @pytest.mark.parametrize is unfamiliar, the syntax is ("name1,name2", [(value1a, value2a), (value1b, value2b), ...]). Each tuple becomes one test case; the test function receives the tuple’s values as named parameters. That’s it.

Your task

  1. Sub-task A: simplify is_trial_expired.
  2. Sub-task B: rewrite stream_quality as a clean if/else.
  3. Sub-task C — fill the truth table FIRST, then refactor. Open truth_table.md. Two of the four rows are pre-filled. Fill the other two by reading the original nested play_decision (in gates.py). Then circle (with a ← BREAKS comment) the row where the naive if is_logged_in and not is_offline: return "stream" simplification would diverge from the original. Only after the table is complete, write the correct simplified version of play_decision and extend the parametrize table in test_play_decision to cover all four rows.

The lesson is not “the original was unsimplifiable” — it can be simplified, but only if the simplification preserves the four-row truth table. The elegant version that does:

def play_decision(self, is_logged_in: bool, is_offline: bool) -> str:
    if not is_logged_in:
        return "error_login_required"
    return "play_cached" if is_offline else "stream"

Externalizing the truth table on paper before touching the function is the load-bearing move: it converts a 6-element working-memory task (4 truth combinations × 2 versions to compare) into a written artifact you can stare at.

Starter files
gates.py
"""Boolean playback gates — one class, three method-level smells."""


class PlaybackGates:
    """Encapsulates the boolean decisions a player makes per playback event."""

    def is_trial_expired(self, free_trial_days_left: int) -> bool:
        # Sub-task A: simplify
        if free_trial_days_left <= 0:
            return True
        else:
            return False

    def stream_quality(self, is_premium: bool) -> str:
        # Sub-task B: rewrite as if/else
        if is_premium:
            return "high"
        if not is_premium:
            return "low"
        return "low"

    def play_decision(self, is_logged_in: bool, is_offline: bool) -> str:
        # Sub-task C: don't simplify naively — fill the truth table first.
        if is_logged_in:
            if not is_offline:
                return "stream"
            else:
                return "play_cached"
        return "error_login_required"
play_controller.py
"""Routes play decisions through all three boolean gates.

Production caller for PlaybackGates. The gate methods get simplified
internally during this step (boolean-return collapse, if/else cleanup,
IfsMerged-safe nested-if rewrite) but their *behavior* must stay
identical — this caller's tests prove it.
"""
from dataclasses import dataclass
from gates import PlaybackGates


@dataclass
class UserState:
    free_trial_days_left: int
    is_premium: bool
    is_logged_in: bool
    is_offline: bool


class PlayController:
    """Decides what action to take for a user about to play."""

    def __init__(self, gates: PlaybackGates) -> None:
        self.gates: PlaybackGates = gates

    def decide(self, state: UserState) -> str:
        """Return one of: trial_expired, error_login_required, play_cached, stream:high, stream:low."""
        if self.gates.is_trial_expired(state.free_trial_days_left):
            return "trial_expired"
        decision: str = self.gates.play_decision(state.is_logged_in, state.is_offline)
        if decision == "stream":
            quality: str = self.gates.stream_quality(state.is_premium)
            return f"stream:{quality}"
        return decision
test_gates.py
"""Truth-table-driven tests for boolean gates and the PlayController caller."""
import pytest
from gates import PlaybackGates
from play_controller import PlayController, UserState


@pytest.fixture
def gates() -> PlaybackGates:
    return PlaybackGates()


@pytest.mark.parametrize("days,expected", [
    (5, False),
    (1, False),
    (0, True),
    (-1, True),
])
def test_is_trial_expired(gates: PlaybackGates, days: int, expected: bool) -> None:
    assert gates.is_trial_expired(days) == expected


@pytest.mark.parametrize("is_premium,expected", [
    (True, "high"),
    (False, "low"),
])
def test_stream_quality(gates: PlaybackGates, is_premium: bool, expected: str) -> None:
    assert gates.stream_quality(is_premium) == expected


# TODO: extend this table to cover ALL FOUR truth combinations of
# (is_logged_in, is_offline). The current table is happy-path only and
# will not catch the IfsMerged trap.
@pytest.mark.parametrize("is_logged_in,is_offline,expected", [
    (True, False, "stream"),
])
def test_play_decision(gates: PlaybackGates, is_logged_in: bool, is_offline: bool, expected: str) -> None:
    assert gates.play_decision(is_logged_in, is_offline) == expected


# ---- Caller tests: PlayController routes through all three gates ----

@pytest.mark.parametrize("state,expected", [
    (UserState(free_trial_days_left=0, is_premium=True,  is_logged_in=True,  is_offline=False), "trial_expired"),
    (UserState(free_trial_days_left=5, is_premium=False, is_logged_in=False, is_offline=False), "error_login_required"),
    (UserState(free_trial_days_left=5, is_premium=True,  is_logged_in=True,  is_offline=True),  "play_cached"),
    (UserState(free_trial_days_left=5, is_premium=True,  is_logged_in=True,  is_offline=False), "stream:high"),
    (UserState(free_trial_days_left=5, is_premium=False, is_logged_in=True,  is_offline=False), "stream:low"),
])
def test_play_controller_decide(gates: PlaybackGates, state: UserState, expected: str) -> None:
    controller: PlayController = PlayController(gates)
    assert controller.decide(state) == expected

if __name__ == "__main__":
    import sys
    pytest.main([sys.argv[0], "-v"])
truth_table.md
# Truth table for `play_decision`

Read the **original nested** `play_decision` in `gates.py`. Fill the two
remaining rows by simulating what the original would return for each
combination. Then put a `← BREAKS` comment on the row where the naive
`if is_logged_in and not is_offline: return "stream"` collapse diverges.

| is_logged_in | is_offline | original returns      | naive `A and not B` returns |
|--------------|------------|-----------------------|------------------------------|
| False        | False      | error_login_required  | error_login_required         |
| False        | True       | error_login_required  | error_login_required         |
| True         | False      | TODO                  | TODO                         |
| True         | True       | TODO                  | TODO                         |

Step 3 — Boolean Anti-Patterns

Min. score: 70%

1. Retrieval / prediction. In a truth table over (is_logged_in, is_offline), which row does the naive if is_logged_in and not is_offline collapse get wrong?

  • (False, False)

    (False, False) returns ‘error_login_required’ in both the original and the naive version — the outer if not is_logged_in guard handles it identically.

  • (False, True)

    (False, True) is also handled identically — when is_logged_in is False, both versions return ‘error_login_required’ regardless of is_offline.

  • (True, False)

    (True, False) returns ‘stream’ in both versions. This is the happy-path row — and the only one a single-test suite would check.

  • (True, True)

(True, True) is the IfsMerged row. Original code returns "play_cached"; the naive if A and not B collapse returns "error_login_required". The bug is invisible to any test that doesn’t include this exact input. Truth-table tests are not optional for boolean logic — they’re load-bearing.

2. Which of the following is the simplest correct simplification of if cond: return True else: return False?

  • return cond is True

    is True checks identity, not truthiness. For a comparison like x <= 0, identity-comparison can fail surprisingly when the result is a numpy bool or similar non-True-singleton.

  • return bool(cond)

    bool(cond) is correct but redundant when cond is already a comparison — comparisons return bool. It’s useful only when the input might be non-boolean (a list, an int) and explicit coercion is desired.

  • return cond
  • if cond: return True; return False

    Removing the else keeps the same anti-pattern. The redundancy is the if/else plus the True/False literals, not just the else.

The condition IS the answer. Comparisons (<=, ==, in, etc.) already return booleans. Wrapping them in if/return True/return False adds no information.

3. You see this code: 

if is_active:
    process()
if not is_active:
    skip()
What’s the smell, and what’s the fix?

  • Speculative generality — fix is to inline the helpers

    Speculative generality is about unused abstractions, not about conditional structure.

  • Confusing else — fix is to use a single if/else
  • Long Method — fix is Extract Function

    These four lines are too short to be a Long Method. The smell is in the shape of the conditional, not the length.

  • There’s no smell; this is fine because the conditions are exclusive

    Logically the conditions are exclusive, but the structure invites two separate evaluations of is_active — and silently breaks if a third branch is later added (e.g., is_active is None). The if/else makes the exclusivity explicit and atomic.

Confusing else is the official name (cf. anti-patterns catalogues). Two ifs with mutually-exclusive conditions are an if/else in disguise. The fix removes the duplicate condition evaluation and makes the structure more change-resistant.

4. Which of these is the safest way to verify a boolean refactoring?

  • Run the existing happy-path test once

    Happy-path tests pass on naive simplifications that drop branches — the IfsMerged trap demonstrates this exactly.

  • Add one new test for the simplified version

    Adding one test catches one bug (if it happens to test the right input). Adding a full table catches every behavior change.

  • Run a parametrize table covering all input combinations
  • Manually inspect the diff for syntactic similarity

    Syntactic similarity is not behavioral equivalence. The IfsMerged collapse looks similar to the original but evaluates differently on one of four rows.

For boolean code, a truth-table parametrize is the only sound oracle. Two-input booleans need 4 rows; three-input need 8; n-input need 2^n. If the table has fewer rows than the input space, the verification is incomplete.

4

Duplicated Code → Extract Function (with the tool)

The tool does the typing — you do the thinking

The tool will do the typing. You are doing three things: choosing the boundary, choosing the name, and judging whether the result is better. Those decisions are yours.

Learning objective. After this step you will be able to (a) recognize duplicated code where the variation between copies is captured by a single parameter, (b) use Monaco’s Refactor: Extract Function/Method action, and (c) decide when to extract using the Rule of Three.

Open filters.py. The MusicLibrary class has two filter methods:

class MusicLibrary:
    def __init__(self, tracks: List[Dict]) -> None:
        self.tracks: List[Dict] = tracks

    def apply_genre_filter(self, genre: str) -> List[Dict]:
        result: List[Dict] = [t for t in self.tracks if t["genre"] == genre]
        result.sort(key=lambda t: t["title"])
        return result[:50]

    def apply_artist_filter(self, artist: str) -> List[Dict]:
        result: List[Dict] = [t for t in self.tracks if t["artist"] == artist]
        result.sort(key=lambda t: t["title"])
        return result[:50]

The structure is identical — filter, sort, paginate. The variation is exactly one thing: the predicate that decides which tracks to keep. That variation is a candidate parameter.

Initial state

MusicLibrary tracks apply_genre_filter(genre) apply_artist_filter(artist)

(One class with two near-duplicate methods. After the refactor a private _apply_filter helper joins them, and each public method becomes a one-line delegation.)

Draft the Issue line FIRST

Before you click anything, open memo.md and write a one-sentence Issue line naming the smell in your own words. Doing this before tool invocation calibrates whether the refactoring you’re about to apply matches the smell you diagnosed. If you can’t articulate the smell in a sentence, the tool will gladly produce a “clean” diff that doesn’t actually fix anything.

💡 Expert’s note. When you see two near-duplicates, the question is what varies between them. If the variation is a constant, it’s a parameter. If the variation is a piece of behavior — like a predicate — it’s a callable parameter. Naming the variation is the load-bearing decision.

How the tool works

  1. Select the lines you want to extract. (Highlight them in the editor.)
  2. Right-click to open the context menu. You’ll see a group of Refactor: actions:
    • Refactor: Rename Symbol…
    • Refactor: Extract Function/Method…
    • Refactor: Introduce Parameter Object…
    • Refactor: Move Method…
    • Refactor: Move Field…
  3. Click the action you want. A dialog appears asking for the new function’s name.
  4. Type the name. The tool shows you a diff preview of the change.
  5. Accept the diff if it looks right. Reject if it doesn’t.

The tool handles the mechanics — cutting the lines, generating the function definition, writing the call site, updating any references it can find. It does not handle judgment. You decide which lines to extract, what to name them, and whether the post-state is better. Those are the load-bearing decisions; the typing is not.

Your task

  1. In apply_genre_filter, select the body (list comprehension, sort, slice).
  2. Invoke Refactor: Extract Function/Method. Name the new method _apply_filter. Accept the diff.
  3. The tool may have made the predicate a literal string. Edit the extracted method so that the predicate is a callable parameter with this signature: 
    from typing import Callable, Dict, List
def _apply_filter(self, predicate: Callable[[Dict], bool]) -> List[Dict]:
    result: List[Dict] = [t for t in self.tracks if predicate(t)]
    result.sort(key=lambda t: t["title"])
    return result[:50]

    And rewrite apply_genre_filter to call it:

    def apply_genre_filter(self, genre: str) -> List[Dict]:
    return self._apply_filter(lambda t: t["genre"] == genre)
  4. Replace the body of apply_artist_filter with a similar call: 
    def apply_artist_filter(self, artist: str) -> List[Dict]:
    return self._apply_filter(lambda t: t["artist"] == artist)
  5. Run the tests. They should still pass. The UML diagram now shows a third method _apply_filter on MusicLibrary — visible proof of the extraction.

The Memo template

From this step on, every refactoring gets a memo — a structured note that captures the design decision. The four fields:

Field What it captures
Issue What smell is present in the original code?
Rationale Why is this refactoring the right fix?
Invariant What property of behavior is preserved?
Tests Which tests confirm the invariant?

Open memo.md. Two fields are pre-filled (Rationale, Invariant). You write the Issue and Tests fields. Don’t skip this — the memo is part of the deliverable for this step.

When NOT to extract — the Rule of Three

Two duplicates is the minimum for extraction. Some teams use the Rule of Three: don’t extract until you see the same pattern three times. The reason: the third occurrence reveals what’s truly common vs. what’s accidental similarity. Extracting on the second occurrence sometimes produces an abstraction that breaks when you find the third.

For this step, two is enough — the variation (predicate) is clearly the only difference. If you’re unsure, wait for the third instance.

Starter files
filters.py
"""Filters for the music library."""
from typing import Dict, List


class MusicLibrary:
    """An in-memory music library with genre and artist filters."""

    def __init__(self, tracks: List[Dict]) -> None:
        self.tracks: List[Dict] = tracks

    def apply_genre_filter(self, genre: str) -> List[Dict]:
        result: List[Dict] = [t for t in self.tracks if t["genre"] == genre]
        result.sort(key=lambda t: t["title"])
        return result[:50]

    def apply_artist_filter(self, artist: str) -> List[Dict]:
        result: List[Dict] = [t for t in self.tracks if t["artist"] == artist]
        result.sort(key=lambda t: t["title"])
        return result[:50]
search_handler.py
"""Dispatches user search queries to the right MusicLibrary filter.

Production caller for MusicLibrary's filter methods. The student
extracts a private `_apply_filter` helper inside MusicLibrary; the
PUBLIC methods (`apply_genre_filter`, `apply_artist_filter`) keep
their signatures, so this caller doesn't need to change. That's the
point of refactoring — internal structure changes, external contract
stays identical.
"""
from typing import Dict, List
from filters import MusicLibrary


class SearchHandler:
    """Routes a (attribute, value) query to the right filter method."""

    def __init__(self, library: MusicLibrary) -> None:
        self.library: MusicLibrary = library

    def search(self, attribute: str, value: str) -> List[Dict]:
        """Dispatch to the genre or artist filter based on attribute name."""
        if attribute == "genre":
            return self.library.apply_genre_filter(value)
        elif attribute == "artist":
            return self.library.apply_artist_filter(value)
        return []
test_filters.py
"""Behavior tests for the filters and the SearchHandler caller."""
import pytest
from typing import Dict, List
from filters import MusicLibrary
from search_handler import SearchHandler


@pytest.fixture
def library() -> MusicLibrary:
    tracks: List[Dict] = [
        {"title": "B", "artist": "Alice", "genre": "rock"},
        {"title": "A", "artist": "Alice", "genre": "jazz"},
        {"title": "C", "artist": "Bob",   "genre": "rock"},
        {"title": "D", "artist": "Carol", "genre": "rock"},
    ]
    return MusicLibrary(tracks)


def test_genre_filter_returns_sorted_matches(library: MusicLibrary) -> None:
    result: List[Dict] = library.apply_genre_filter("rock")
    assert [t["title"] for t in result] == ["B", "C", "D"]


def test_artist_filter_returns_sorted_matches(library: MusicLibrary) -> None:
    result: List[Dict] = library.apply_artist_filter("Alice")
    assert [t["title"] for t in result] == ["A", "B"]


def test_genre_filter_paginates_at_50() -> None:
    tracks: List[Dict] = [
        {"title": f"T{i:03d}", "artist": "X", "genre": "rock"} for i in range(100)
    ]
    big_library = MusicLibrary(tracks)
    result: List[Dict] = big_library.apply_genre_filter("rock")
    assert len(result) == 50


# ---- Caller tests: SearchHandler dispatches both filter kinds ----

def test_search_handler_dispatches_to_genre(library: MusicLibrary) -> None:
    handler: SearchHandler = SearchHandler(library)
    result: List[Dict] = handler.search("genre", "rock")
    assert [t["title"] for t in result] == ["B", "C", "D"]


def test_search_handler_dispatches_to_artist(library: MusicLibrary) -> None:
    handler: SearchHandler = SearchHandler(library)
    result: List[Dict] = handler.search("artist", "Alice")
    assert [t["title"] for t in result] == ["A", "B"]


def test_search_handler_unknown_attribute_returns_empty(library: MusicLibrary) -> None:
    handler: SearchHandler = SearchHandler(library)
    assert handler.search("year", "2020") == []

if __name__ == "__main__":
    import sys
    pytest.main([sys.argv[0], "-v"])
memo.md
# Refactoring memo — Step 4

## Issue
<!-- TODO: name the smell in one sentence. What's wrong with the original code? -->


## Rationale
The variation between `apply_genre_filter` and `apply_artist_filter` is the predicate
(what makes a track a "match"). Everything else — the list comprehension, the sort,
the pagination — is identical. Extracting the common structure into a helper that
accepts a callable predicate captures the duplication exactly, with no over-generalization.

## Invariant
For any library and any field/value pair, calling the original duplicated function
and the new factored-through-`_apply_filter` version produces identical results
(same list, same order, same length). External behavior is unchanged.

## Tests
<!-- TODO: which tests in test_filters.py confirm the invariant? List them by name. -->

Step 4 — Duplicated Code

Min. score: 70%

1. Retrieval / prediction. After Extract Function, which three regions of filters.py will have changed?

  • Imports, function definitions, return statements

    Imports may or may not change (only if Callable gets added). Function definitions and return statements aren’t a useful taxonomy of regions.

  • The new helper definition, the body of the first filter, the body of the second filter
  • Imports, the helper, and the test file

    The test file shouldn’t change at all — that’s the behavior preservation property. Editing tests during a refactor signals that behavior changed, not just structure.

  • Only the body of one filter — the tool handles the rest automatically

    The tool extracts a region into a helper, but the second duplicate still has the old body until it gets replaced with a delegating call. That second replacement is a human task.

New helper, first delegator, second delegator. Each duplicate becomes a single-line delegating call. The test file should not change — if it did, you changed behavior, not just structure.

2. Spacing back to Step 1. Suppose Step 1’s royalty_song, royalty_podcast, royalty_audiobook (with the bug + in two of them) had been refactored before the bug was discovered — extracted into a single _royalty(plays, rate) helper. What would have happened?

  • The bug would have been caught immediately

    Extraction doesn’t run the tests automatically. It moves code; it doesn’t validate it.

  • The bug would have been propagated into the helper — one buggy place instead of three
  • The refactoring would have failed because the duplicates differ

    The duplicates were near-identical — same structure, same operator (or at least, same intended operator). The tool would have happily extracted them; whether the result was correct depends on which of the three the helper inherits its body from.

  • The tests would have caught it because of the extraction

    Existing tests covered only royalty_song. Without coverage of the other two, no extraction or non-extraction would surface the bug.

Fix-then-refactor. This is exactly why Step 1’s lesson is “fix first.” Extracting buggy duplicates into a helper consolidates the bug, which feels like progress but isn’t. The bug is still there — it’s just in a single, harder-to-spot location.

3. You see a piece of code that’s been duplicated once (two copies). Should you extract it?

  • Yes, immediately — duplication is always a smell

    Eager extraction at two duplicates is a real pattern, but it backfires when the third occurrence reveals a different variation than the first two implied (the wrong-abstraction trap).

  • It depends — if the variation is obvious, extract; if not, wait
  • No — the Rule of Three says wait until you see three copies

    The Rule of Three is a default, not an absolute. When the variation is obvious and stable (like a predicate), extracting at two is fine.

  • No — extraction always over-generalizes

    Extraction can over-generalize, but it doesn’t have to. The risk is real but manageable with judgment.

Judgment beats rules. Sandi Metz: “Duplication is far cheaper than the wrong abstraction.” When unsure, wait for the third occurrence. When the variation is one obvious dimension, extract early.

4. What name does the tool assign to the extracted function if you don’t type one?

  • Whatever Python’s pyflakes infers automatically

    Pyflakes is a linter; it doesn’t name functions. Naming is the tool user’s responsibility.

  • A default like extracted_function_1 or similar

    If a tool defaulted to extracted_function_1, students would accept the default and produce code that’s harder to read than the original. The tool deliberately doesn’t do this.

  • The tool requires you to type a name — it cannot proceed without one
  • The tool guesses based on the variable types

    Type-based guessing produces names like process_dict_list — accurate but useless. Names should describe intent, not types.

Naming is the load-bearing decision. The tool refuses to proceed without a name because choosing a name forces you to articulate what the helper is. That articulation is the heart of the refactoring.

5

Long Parameter List → Introduce Parameter Object (with the tool)

Eight parameters and a clump

Learning objective. After this step you will be able to (a) recognize a Long Parameter List and the data clump hiding inside it, (b) use Monaco’s Refactor: Introduce Parameter Object to consolidate the clump into a @dataclass, and (c) explain why type-annotated parameter objects beat dicts for this purpose.

Open track.py. The class TrackCatalog has an add_track method that takes eight parameters:

class TrackCatalog:
    def add_track(
        self,
        title: str,
        artist: str,
        album: str,
        duration_sec: int,
        genre: str,
        release_year: int,
        bpm: int,
        isrc: str,
    ) -> Dict:
        ...

Eight is a lot. But the smell isn’t just the count — it’s that four of the eight always travel together: artist, album, genre, release_year describe the album a track belongs to. Every call site that passes one of them passes all four; every call site that mutates one mutates them as a unit.

That’s a data clump. The fix is Introduce Parameter Object — a small class (here a @dataclass) that names the clump and makes the relationship explicit.

Initial state

TrackCatalog library add_track(title, artist, album, duration_sec, genre, release_year, bpm, isrc)

(One class, one method with eight flat parameters. After the refactor, four of these will be replaced by an AlbumInfo parameter object — and you’ll see a new AlbumInfo box appear in the live UML next to TrackCatalog.)

A 30-second @dataclass primer

Python’s @dataclass decorator generates __init__, __repr__, and __eq__ for you from a list of typed field declarations. It looks like this:

from dataclasses import dataclass

@dataclass
class Point:
    x: int
    y: int

That’s the entire class. Point(x=3, y=4) works. Point(3, 4) == Point(3, 4) returns True. No constructor body, no boilerplate. The tool will generate one for you in a moment — recognize it; don’t try to derive it.

Sketch AlbumInfo BEFORE invoking the tool

Take 60 seconds. In a comment at the top of track.py, write what you’d put in an AlbumInfo dataclass if you were writing it by hand: field names + Python types. Don’t peek at the tool’s output yet. Then invoke the tool and compare. The compare is the lesson — your sketch usually matches the tool’s output, which builds your trust that the tool is doing what you’d do, just faster.

How to drive the tool

  1. Place your cursor in TrackCatalog.add_track’s parameter list. Right-click → Refactor: Introduce Parameter Object…
  2. The tool asks for the class name — type AlbumInfo. Select artist, album, genre, release_year.
  3. Inspect the diff before accepting. The new add_track should accept (self, title, album_info, duration_sec, bpm, isrc) — five parameters (plus self), one of which is structured.
  4. After accepting, look at the live UML in the bottom-left. A new AlbumInfo box has appeared with the four clumped fields, and TrackCatalog.add_track’s signature is shorter.

Run the tests

After you accept the refactoring, run the tests. If they fail, the tool may have missed a call site somewhere — tools rewrite syntax they can see, and dynamic-feeling Python call patterns can slip through static analysis.

Don’t read ahead about which call. Run the tests, watch the failure, find the offending call site yourself, fix it. The closing reflection — “what did the tool do well? what did it not catch?” — is the load-bearing lesson of this step. Behavior preservation remains your responsibility even with a smart tool.

Why a @dataclass, not a dict?

You could also pass {"artist": ..., "album": ..., "genre": ..., "release_year": ...} as a dict. That’s worse for three reasons:

Property @dataclass AlbumInfo dict
Type-checked at definition yes (mypy / IDE) no
Auto-completion in editor yes partial
Typos caught early yes (album_info.titel is an error) no (d["titel"] returns KeyError at runtime)

The dict version “works” but defers every type error to runtime. The dataclass moves the same errors to definition time, which is where bugs are cheap to fix.

Starter files
track.py
"""The track catalog: add a new track to the library."""
from typing import Dict, List


class TrackCatalog:
    """Stores and inserts tracks. Pretend the library list is a database."""

    def __init__(self) -> None:
        self.library: List[Dict] = []

    def add_track(
        self,
        title: str,
        artist: str,
        album: str,
        duration_sec: int,
        genre: str,
        release_year: int,
        bpm: int,
        isrc: str,
    ) -> Dict:
        """Insert a new track into the library."""
        record: Dict = {
            "title": title,
            "artist": artist,
            "album": album,
            "duration_sec": duration_sec,
            "genre": genre,
            "release_year": release_year,
            "bpm": bpm,
            "isrc": isrc,
        }
        self.library.append(record)
        return record
helpers.py
"""Helpers for seeding default tracks."""
from track import TrackCatalog


def seed_library(catalog: TrackCatalog) -> None:
    """Populate a catalog with some default tracks."""
    catalog.add_track("Lullaby", "Alice", "Bedtime", 180, "ambient", 2020, 60, "ISRC001")
    catalog.add_track("Sprint",  "Bob",   "Workout", 120, "electro", 2021, 140, "ISRC002")
memo.md
# Refactoring memo — Step 5

## Issue
`add_track` takes eight parameters, four of which (`artist`, `album`, `genre`,
`release_year`) always travel together as album-level metadata. This is a
**Long Parameter List with a data clump** — the four fields describe the album
a track belongs to, and they always co-vary in call sites.

## Rationale
<!-- TODO: explain WHY introducing AlbumInfo is the right fix.
     Why a @dataclass, not a dict? Why these four parameters and not others? -->


## Invariant
<!-- TODO: which property of behavior must be preserved across the refactor?
     (Hint: same input values → same record dict in LIBRARY.) -->


## Tests
`test_add_track_inserts_record` confirms the record values are unchanged after the
refactor. `test_seed_library_populates_two_tracks` confirms the helper module's
calls to `add_track` continue to work. Both tests must stay green throughout.
test_track.py
"""Tests — verify behavior is preserved across the refactor."""
import pytest
from typing import Dict
from track import TrackCatalog
from helpers import seed_library


@pytest.fixture
def catalog() -> TrackCatalog:
    return TrackCatalog()


def test_add_track_inserts_record(catalog: TrackCatalog) -> None:
    # After the refactor, the call will use AlbumInfo for the album fields.
    record: Dict = catalog.add_track(
        title="Echo",
        artist="Carol",
        album="Reflections",
        duration_sec=200,
        genre="indie",
        release_year=2022,
        bpm=110,
        isrc="ISRC003",
    )
    assert record["title"] == "Echo"
    assert record["artist"] == "Carol"
    assert record["album"] == "Reflections"
    assert record["genre"] == "indie"
    assert record["release_year"] == 2022
    assert len(catalog.library) == 1


def test_seed_library_populates_two_tracks(catalog: TrackCatalog) -> None:
    seed_library(catalog)
    assert len(catalog.library) == 2
    assert catalog.library[0]["title"] == "Lullaby"
    assert catalog.library[1]["bpm"] == 140

if __name__ == "__main__":
    import sys
    pytest.main([sys.argv[0], "-v"])

Step 5 — Long Parameter List

Min. score: 70%

1. Retrieval. Which of add_track’s eight parameters travel together as a clump? (select all that apply)

  • title

    title is a track-level property — every track has its own. It varies independently of the album fields.

  • artist
  • album
  • duration_sec

    duration_sec is track-level. Two tracks on the same album can have different durations.

  • genre
  • release_year
  • bpm

    bpm is track-level. Even within a single album, tempo varies song to song.

  • isrc

    isrc (International Standard Recording Code) is unique per recording, not per album. It’s the most track-specific identifier.

The clump is artist, album, genre, release_year — these describe the album, not the individual track. Two tracks on the same album always pass these four together with the same values. That co-variation is the diagnostic for a parameter object.

2. Why prefer @dataclass over a plain dict for the parameter object?

  • dataclasses are faster at runtime

    Performance is roughly equivalent. Dataclass attribute access compiles to a normal __dict__ lookup; dict access is a hash lookup. Both are fast.

  • dataclasses give type-checked field access; dicts defer typos to runtime
  • dicts can’t hold heterogeneous types

    Dicts hold heterogeneous types just fine — a dict can have str, int, and list values.

  • dataclasses are required by Python’s type checker

    Type checkers don’t require dataclasses; they just produce better diagnostics with them than with dicts.

Type-checked field access is the load-bearing benefit. album_info.titel (typo) is a static error; d["titel"] is a runtime KeyError. Moving errors earlier — from runtime to edit time — is the same theme as test-driven development.

3. The tool rewrites call sites for you. After applying Introduce Parameter Object, your tests fail. What’s the most likely cause?

  • The tool corrupted the AlbumInfo definition

    Tool-generated dataclass definitions are deterministic — they don’t ‘corrupt’. If the dataclass body looks wrong, that’s an undo-and-retry, not a tool bug.

  • The tool missed a positional call site in another file
  • The tests need updating because the refactor changed behavior

    If tests need updating to pass, the refactor changed behavior — that’s not a refactor, that’s a rewrite. The whole point of the safety dance is that tests don’t change.

  • The dataclass decorator is malformed

    If the dataclass were malformed, the import would fail at collection time, not in a specific test.

The tool sees what it can statically analyze. Positional calls in helper modules sometimes get missed — particularly when the helper imports add_track indirectly. The tests catch the miss; you fix it manually. Behavior preservation remains your responsibility, even with a tool.

4. What’s the difference between using @dataclass and writing the class by hand?

  • @dataclass is slower at runtime than a hand-written class

    Dataclasses are essentially the same as hand-written classes at runtime. The decorator runs once at definition; instances behave normally.

  • @dataclass auto-generates init, repr, and eq
  • @dataclass classes can’t have methods

    Dataclasses can have methods. The decorator handles __init__ etc.; anything else can still be added normally.

  • @dataclass is only for inheritance hierarchies

    Dataclasses can be used in inheritance, but they’re more commonly used as flat parameter objects — exactly what we did with AlbumInfo.

Boilerplate generation. @dataclass writes the constructor, equality check, and repr from your field annotations. You get the same behavior as a hand-written class with one-third the code.

6

Feature Envy → Move Method (with the tool)

Methods belong with the data they use

Learning objective. After this step you will be able to (a) recognize Feature Envy by the rule “uses zero state of its host class,” (b) use Monaco’s Refactor: Move Method to relocate the method to the class whose data it actually uses, and (c) explain why type-annotated signatures make moves safer.

Open media.py. There are two classes, Player and Track, and one suspicious method:

class Player:
    def __init__(self, volume: int) -> None:
        self.volume: int = volume

    def compute_remaining_seconds(self, track: "Track") -> int:
        return track.duration_sec - track.current_position

compute_remaining_seconds lives on Player but uses zero state of Player. It only touches fields of Track. That’s Feature Envy — the method “envies” the data of another class.

Initial state

Player volume compute_remaining_seconds(track) Track track_id duration_sec current_position is_finished() uses

(compute_remaining_seconds lives on Player, but its body only reads Track fields. The arrow points the wrong way.)

💡 Diagnostic. Read the body of any method. Does it touch self at all? If not — or if it touches self only as a delegator — the method probably belongs on the other object whose data it does touch.

Run the move

Place your cursor inside compute_remaining_seconds, then Refactor: Move Method… with target class Track. Same flow as Step 4’s Extract Function — preview, accept. Watch the live UML in the bottom-left: the method migrates from Player to Track, and the Player → Track “uses” arrow disappears because Player no longer reaches into Track for this calculation.

Spacing callback — apply Step 3’s lesson here

Track has a sibling method is_finished that suffers from the boolean-return anti-pattern from Step 3:

def is_finished(self) -> bool:
    if self.current_position >= self.duration_sec:
        return True
    else:
        return False

After moving compute_remaining_seconds onto Track, simplify is_finished while you’re there. Same pattern, new context — the smell doesn’t care which class it lives on.

The Memo

Open memo.md. Three of four fields are blank — the Tests field is pre-filled. You write Issue, Rationale, and Invariant based on what you observe in the move.

Starter files
media.py
"""The streaming media classes."""
from dataclasses import dataclass


@dataclass
class Track:
    track_id: str
    duration_sec: int
    current_position: int = 0

    def is_finished(self) -> bool:
        # Sub-task: simplify this boolean (Step 3 callback)
        if self.current_position >= self.duration_sec:
            return True
        else:
            return False


@dataclass
class Player:
    volume: int = 50

    def compute_remaining_seconds(self, track: Track) -> int:
        # Feature Envy: uses no Player state, only Track state.
        return track.duration_sec - track.current_position
playback_ui.py
"""UI helper that formats the player's current state for display.

This caller currently goes through Player.compute_remaining_seconds(track),
which is Feature Envy on Track. After the Move Method refactor, the call
site here MUST be updated from `self.player.compute_remaining_seconds(track)`
to `track.compute_remaining_seconds()`. The tool may rewrite this for you;
if it doesn't, the tests will catch the missed call site.
"""
from media import Player, Track


class PlaybackUI:
    """Renders the player display."""

    def __init__(self, player: Player) -> None:
        self.player: Player = player

    def format_remaining(self, track: Track) -> str:
        """Return a `M:SS remaining` string for the given track."""
        seconds: int = self.player.compute_remaining_seconds(track)
        minutes: int = seconds // 60
        secs: int = seconds % 60
        return f"{minutes}:{secs:02d} remaining"
test_media.py
"""Behavior tests for Player, Track, and the PlaybackUI caller."""
from media import Player, Track
from playback_ui import PlaybackUI
import pytest


def test_remaining_seconds_at_start() -> None:
    t: Track = Track(track_id="t1", duration_sec=300, current_position=0)
    # The student must update this call site after the move:
    # before: Player().compute_remaining_seconds(t)
    # after:  t.compute_remaining_seconds()
    p: Player = Player()
    assert p.compute_remaining_seconds(t) == 300


def test_remaining_seconds_partway() -> None:
    t: Track = Track(track_id="t1", duration_sec=300, current_position=120)
    p: Player = Player()
    assert p.compute_remaining_seconds(t) == 180


def test_is_finished_true_at_end() -> None:
    t: Track = Track(track_id="t1", duration_sec=100, current_position=100)
    assert t.is_finished() is True


def test_is_finished_false_partway() -> None:
    t: Track = Track(track_id="t1", duration_sec=100, current_position=50)
    assert t.is_finished() is False


# ---- Caller tests: PlaybackUI must keep working across the Move Method refactor ----

def test_playback_ui_formats_remaining_at_start() -> None:
    t: Track = Track(track_id="t1", duration_sec=125, current_position=0)
    ui: PlaybackUI = PlaybackUI(Player())
    # 125s = 2:05 remaining
    assert ui.format_remaining(t) == "2:05 remaining"


def test_playback_ui_formats_remaining_partway() -> None:
    t: Track = Track(track_id="t1", duration_sec=125, current_position=60)
    ui: PlaybackUI = PlaybackUI(Player())
    # 125 - 60 = 65s = 1:05 remaining
    assert ui.format_remaining(t) == "1:05 remaining"

if __name__ == "__main__":
    import sys
    pytest.main([sys.argv[0], "-v"])
memo.md
# Refactoring memo — Step 6

## Issue
<!-- TODO: name the smell. Why does compute_remaining_seconds belong on Track instead of Player? -->


## Rationale
<!-- TODO: explain WHY moving the method is the right fix. What's the heuristic? -->


## Invariant
<!-- TODO: which property of behavior is preserved? Be specific about the contract. -->


## Tests
`test_remaining_seconds_at_start`, `test_remaining_seconds_partway`, `test_is_finished_true_at_end`,
and `test_is_finished_false_partway` confirm the invariant — same inputs produce the same
outputs both before and after the move (after updating call sites). The test file should not
need changes other than rewriting `p.compute_remaining_seconds(t)` to `t.compute_remaining_seconds()`.

Step 6 — Feature Envy

Min. score: 70%

1. Prediction (before reading the body). Player.compute_remaining_seconds(track) returns a number of seconds. Without reading the body, which class is most likely to be the right home for this method?

  • Player — the method already lives there

    Existing location is the least trustworthy heuristic — it shows where the method has been, not where it should be. Feature Envy is the smell of methods being in the wrong place.

  • Track — methods that compute properties of a track usually belong on Track
  • User — playback decisions ultimately serve a user

    User-level decisions belong on User-level methods. Computing remaining seconds of a Track is at the Track level.

  • It depends on the body; you can’t predict from the name

    Method names are usually a strong signal. compute_remaining_seconds of a track is a property of the track. Read the body to confirm, but the name pre-commits.

The noun is the receiver. A method named “compute X of Y” is usually a method on Y. Reading the body confirms — uses zero Player state, all Track state. Move Method.

2. What’s the rule that diagnoses Feature Envy?

  • The method is more than 10 lines long

    Length is a different smell (Long Method). Feature Envy can affect very short methods.

  • The method uses zero state of its host class (only foreign state)
  • The method has too many parameters

    Parameter count is the Long Parameter List smell. Feature Envy is independent of parameter count.

  • The method’s name doesn’t match its containing class

    Name mismatch is a heuristic but not the diagnostic. The diagnostic is body inspection — what state does the body actually touch?

“Uses zero state of host class” is the precise diagnostic. A method that doesn’t touch self (other than to read it) is suspicious; a method that touches self only to delegate to another object is suspicious; a method that uses only foreign state is unambiguously Feature Envy.

3. Step 3 callback. Why simplify is_finished from if cond: return True else: return False to return cond?

  • Because it makes the file shorter

    Shorter for its own sake isn’t a goal. The simplification removes redundant cognitive structure, not lines.

  • Because the if/else around a comparison adds no information beyond the comparison itself
  • Because Python’s bytecode runs faster on return cond

    Bytecode performance is roughly identical. The reason to simplify is readability, not speed.

  • Because the original form is a syntax error in newer Python versions

    The original form is valid Python at every version. It’s just bad style.

The condition (self.current_position >= self.duration_sec) is already a bool. Wrapping it in if/else with literal True and False returns is pure ceremony — same anti-pattern from Step 3, now in a method context.

4. Could you instead move the data (Track’s fields) into Player, leaving the method where it is?

  • Yes — moving data to the method is symmetric to moving the method to the data

    Logically symmetric, practically not. Data tends to have many users; methods tend to have few. Moving data breaks more code.

  • Yes — but only if Track has no other methods that use those fields

    Even if Track had no other users right now, designing for the move would create future-coupling that’s hard to undo. Move the method, not the data.

  • No — Track has many other methods that use duration_sec and current_position; moving the data scatters them
  • No — Python’s class system doesn’t allow moving fields between classes

    Fields can be moved technically. The reason not to is structural, not syntactic.

Methods are easier to move than data. A method has one home and a few callers; a field tends to have many readers and writers. Moving fields shotgun-surgeries the codebase. Moving methods tightens it.

5. Variation Theory — non-example. A Catalog.build_default_track() method on a Catalog class uses no self state and only returns a new Track from a few constants. Is this Feature Envy?

  • Yes — it uses no Catalog state, so it should be moved to Track

    The diagnostic is necessary but not sufficient. A method that uses zero host state is a candidate for Feature Envy, but factories, validators, and pure-rule synthesizers can legitimately live on the class that exposes them as API.

  • No — factory methods that don’t use host state but synthesize new objects belong on the class that exposes the API
  • Yes — and the fix is to delete the method since it does nothing

    The method does plenty — it produces a Track. It’s a factory, not dead code.

  • Only if the method takes any parameters

    Parameter count is unrelated. Factories with parameters, factories with none — both can legitimately live on the calling class.

The diagnostic catches Feature Envy candidates, but judgment decides. “Uses zero host state” makes a method a candidate for Move Method, not a guaranteed instance of the smell. Factories, default constructors, and pure-policy synthesizers live where the API surface is, not where the data is.

6. Mid-tutorial mix — Step 1 callback. You’ve just discovered that two functions have the same arithmetic bug. According to what you practiced in Step 1, what’s the correct order of operations?

  • Refactor first to remove the duplication, then fix the bug in the helper

    Fix-then-refactor. Extracting with the bug intact would consolidate the bug into the helper — same bug, harder-to-spot location.

  • Fix the bug in both places, then refactor to remove the duplication
  • Fix in one place; the second one isn’t actually called

    If a duplicate is genuinely uncalled, that’s dead code, a different smell. The Step 1 lesson assumes both copies are reachable.

  • Run the tests; if they pass, the duplication isn’t a problem

    Running tests is the right discipline, but passing tests don’t mean the duplication isn’t a maintenance cost. The bug count will equal the duplicate count when the next bug arrives.

Fix-then-refactor. The Step 1 lesson generalizes: never refactor over a known bug. Fix in each location with green tests, then consolidate. This is the highest-leverage sequencing rule in the toolkit.

7. Mid-tutorial mix — Step 4 callback. A teammate spots two near-duplicate functions and proposes Extract Function. The duplicates were just written this morning and the third occurrence isn’t on the roadmap. What does the Rule of Three suggest?

  • Extract immediately — duplication is always a smell

    Eager extraction at two duplicates is a real pattern, but it backfires when the third occurrence reveals a different variation than the first two implied (the wrong-abstraction trap).

  • Wait for the third occurrence to confirm what’s truly common
  • Refuse to extract until tests are added for both copies

    Tests-first is correct discipline before any refactor, but it’s not what Rule of Three says. Rule of Three is about the third occurrence, not the third test.

  • Extract only if the two functions live in the same file

    Co-location is unrelated to Rule of Three.

Two duplicates is the minimum, three is the safe bet. The Rule of Three exists because two near-duplicates can mislead — what looks like the same pattern may diverge in the third occurrence in a way that breaks the abstraction.

7

God Class → Extract Class (with the tool)

One class, two responsibilities

Learning objective. After this step you will be able to (a) recognize a God Class by its multiple responsibility clusters, (b) use Monaco’s Refactor: Extract Class to move a cohesive field/method cluster into a new class, and (c) explain why a clean extraction reduces change locality — fewer files change for any given feature.

Open streaming_app.py. The StreamingApp class has two distinct responsibilities mixed into one body:

  • Catalog — track index, search history, recommendation cache, search & recommend methods.
  • Billing — subscription tier, payment method, invoice list, charging methods.

The comments in the source file label the two clusters explicitly. That’s a hint: when responsibility clusters need labels to stay legible, the class is doing too many things at once.

Initial state

StreamingApp track_index search_history recommendation_cache .. subscription_tier payment_method invoice_list .. search() record_recommendation() charge_monthly() charge_annual() send_invoice() <b>Two clusters:</b> Catalog (top half) + Billing (bottom half). We will extract the Billing cluster into a separate BillingManager.

Why split? — predict the change-locality, then verify

Before you do any moves, predict: if a new payment method like PayPal needed to be added today, how many places in streaming_app.py would have to change? Skim the file, count, and write your number in a comment at the top of streaming_app.py like # BEFORE: N places change for a new payment method.

After the refactor, count again — how many files / classes change for the same feature. Write # AFTER: M places change.

The point of writing both numbers down is that change locality is the only metric that distinguishes “decomposed” from “renamed.” A renamed class still has the same change footprint. A decomposed class shrinks it.

💡 Expert’s note — recognizing seams. The seam between two responsibility clusters isn’t visible from how methods are named — it’s visible from which self.X fields each method touches. Methods that touch only the catalog field-set and methods that touch only the billing field-set form two clusters. A method that touches both is the boundary case and goes last. Read the methods one at a time, mark the field-sets they touch, and the seam will be obvious.

Use Extract Class

Place your cursor anywhere inside StreamingApp, then choose Refactor: Extract Class…. In the dialog:

  1. Name the new class BillingManager.
  2. Name the delegate field billing.
  3. Select the billing fields: subscription_tier, payment_method, invoice_list.
  4. Select the billing methods: charge_monthly, charge_annual, send_invoice, and the unlabeled notify_payment_due.
  5. Preview the diff, then apply it.

The tool creates BillingManager, moves the selected field/method cluster, replaces the old fields with self.billing = BillingManager(...), and rewrites straightforward typed call sites from streaming_app.charge_monthly() to streaming_app.billing.charge_monthly().

💡 Expert’s note. Extract Class still follows the field-then-method safety rule internally. Fields become the new class’s state first; then methods that use only that field-set can move without leaving dangling self.X references behind.

Spacing callback — Rule of Three from Step 4

Look at charge_monthly and charge_annual carefully. They share most of their logic. Should you Extract Function on the shared body before moving them?

The Step 4 lesson said: extract when the variation is one obvious dimension. Here the variation is the multiplier (12 vs. 1). The bodies are otherwise identical.

Recommended order: Move first, then Extract second. Keeps the extracted helper inside BillingManager, exactly where billing logic should live. (If you Extract first, the helper is on StreamingApp and then has to be moved too — twice the work for the same result.)

The unlabeled stray method

One method, notify_payment_due, isn’t labeled with a comment. Read its body. Which cluster does it belong in? The answer is in the body — this is a small recovery exercise in seam recognition.

Starter files
streaming_app.py
"""The streaming app — currently a God Class.

Two responsibility clusters share one class body. Your task is to
extract the billing cluster into a new BillingManager class.
"""
from dataclasses import dataclass, field
from typing import Dict, List


class StreamingApp:
    def __init__(self, user_id: str) -> None:
        self.user_id: str = user_id

        # ----- Catalog cluster -----
        self.track_index: Dict[str, dict] = {}
        self.search_history: List[str] = []
        self.recommendation_cache: Dict[str, List[str]] = {}

        # ----- Billing cluster -----
        self.subscription_tier: str = "free"
        self.payment_method: str = ""
        self.invoice_list: List[Dict] = []

    # ----- Catalog methods -----
    def search(self, query: str) -> List[str]:
        self.search_history.append(query)
        return [tid for tid, info in self.track_index.items()
                if query.lower() in info.get("title", "").lower()]

    def record_recommendation(self, seed: str, results: List[str]) -> None:
        self.recommendation_cache[seed] = results

    # ----- Billing methods -----
    def charge_monthly(self) -> Dict:
        invoice: Dict = {"period": "monthly", "amount": 9.99 * 1, "method": self.payment_method}
        self.invoice_list.append(invoice)
        return invoice

    def charge_annual(self) -> Dict:
        invoice: Dict = {"period": "annual", "amount": 9.99 * 12, "method": self.payment_method}
        self.invoice_list.append(invoice)
        return invoice

    def send_invoice(self, invoice_index: int) -> bool:
        if invoice_index >= len(self.invoice_list):
            return False
        # Pretend to email the invoice.
        return True

    # ----- Which cluster does this belong in? -----
    def notify_payment_due(self) -> str:
        return f"Payment of $9.99 due on {self.payment_method}"
memo.md
# Refactoring memo — Step 7

## Issue
<!-- TODO: name the smell. What makes StreamingApp a God Class? -->


## Rationale
<!-- TODO: why Extract Class? Why this seam? Why does the field-set define the boundary? -->


## Invariant
<!-- TODO: what behavior must be preserved? What does the public API
     of StreamingApp guarantee that the refactor must not break? -->


## Tests
The behavior tests in `test_streaming_app.py` cover both the catalog API
(`test_search_returns_matching_titles`, `test_search_records_history`) and the
billing API (`test_charge_monthly_creates_invoice`, `test_charge_annual_creates_invoice`,
`test_invoice_list_grows_per_charge`). The catalog tests should require no edits
across the refactor. The billing tests are written defensively to support both the
old (`app.charge_monthly()`) and new (`app.billing.charge_monthly()`) APIs, so they
stay green throughout the migration.
app_runner.py
"""Daily app routine that exercises both the catalog and billing clusters.

Every call site here that touches billing (charge_monthly, invoice_list)
will need to be updated after the Extract Class refactor — from
`self.app.charge_monthly()` to `self.app.billing.charge_monthly()`,
and from `self.app.invoice_list` to `self.app.billing.invoice_list`.
The tool may not catch all of these; the tests will.
"""
from typing import List
from streaming_app import StreamingApp


class AppRunner:
    """Drives a daily flow over StreamingApp's public surface."""

    def __init__(self, app: StreamingApp) -> None:
        self.app: StreamingApp = app

    def daily_routine(self, queries: List[str]) -> int:
        """Run searches, then process the monthly charge.

        Returns the number of invoices accumulated so far.
        """
        for q in queries:
            self.app.search(q)
        self.app.charge_monthly()
        return len(self.app.invoice_list)
test_streaming_app.py
"""Public-API behavior tests for StreamingApp and AppRunner."""
import pytest
from typing import Dict
from streaming_app import StreamingApp
from app_runner import AppRunner


@pytest.fixture
def app() -> StreamingApp:
    a: StreamingApp = StreamingApp(user_id="u42")
    a.track_index = {
        "t1": {"title": "Echoes"},
        "t2": {"title": "Echo Chamber"},
        "t3": {"title": "Bright Lights"},
    }
    # The billing fields will move; the test sets them via whatever
    # path remains correct (direct attribute today, .billing.X tomorrow).
    return a


# ---- Catalog public API ----
def test_search_returns_matching_titles(app: StreamingApp) -> None:
    assert sorted(app.search("echo")) == ["t1", "t2"]


def test_search_records_history(app: StreamingApp) -> None:
    app.search("echo")
    app.search("bright")
    assert app.search_history == ["echo", "bright"]


# ---- Billing public API — written to survive the extraction ----
def test_charge_monthly_creates_invoice(app: StreamingApp) -> None:
    # After refactor, billing.subscription_tier, billing.payment_method, etc.
    if hasattr(app, 'billing'):
        app.billing.payment_method = "visa-1234"
        invoice: Dict = app.billing.charge_monthly()
    else:
        app.payment_method = "visa-1234"
        invoice = app.charge_monthly()
    assert invoice["amount"] == pytest.approx(9.99)
    assert invoice["method"] == "visa-1234"


def test_charge_annual_creates_invoice(app: StreamingApp) -> None:
    if hasattr(app, 'billing'):
        app.billing.payment_method = "visa-1234"
        invoice: Dict = app.billing.charge_annual()
    else:
        app.payment_method = "visa-1234"
        invoice = app.charge_annual()
    assert invoice["amount"] == pytest.approx(9.99 * 12)


def test_invoice_list_grows_per_charge(app: StreamingApp) -> None:
    if hasattr(app, 'billing'):
        app.billing.charge_monthly()
        app.billing.charge_monthly()
        assert len(app.billing.invoice_list) == 2
    else:
        app.charge_monthly()
        app.charge_monthly()
        assert len(app.invoice_list) == 2


# ---- Caller tests: AppRunner must keep working across the Extract Class refactor ----

def test_app_runner_records_searches_and_charges(app: StreamingApp) -> None:
    runner: AppRunner = AppRunner(app)
    invoice_count: int = runner.daily_routine(["echo", "bright"])
    assert app.search_history == ["echo", "bright"]
    assert invoice_count == 1

if __name__ == "__main__":
    import sys
    pytest.main([sys.argv[0], "-v"])

Step 7 — God Class

Min. score: 70%

1. Up-front classification. Which of the following are billing-only fields of the original StreamingApp? (select all that apply)

  • track_index

    track_index is the catalog’s data — what tracks the app knows about. It stays on StreamingApp.

  • search_history

    search_history is queried by the search method, also catalog. Stays.

  • subscription_tier
  • recommendation_cache

    recommendation_cache is the recommendation engine’s data. Catalog cluster.

  • payment_method
  • invoice_list
  • user_id

    user_id identifies the app instance overall (catalog and billing both reference it). Stays at the StreamingApp level.

Three billing fields: subscription_tier, payment_method, invoice_list. Identifying these before using the tool is the responsibility-recognition gate. Without it, you risk moving fields that don’t actually belong to the billing cluster.

2. What’s the safe order for extracting a class via Move Method/Field?

  • Move methods first, then fields

    Methods that reference fields will break when their fields are still elsewhere. Method-first leaves a window where references are wrong.

  • Move fields first, then methods
  • Move them simultaneously to keep the class consistent

    Simultaneous moves aren’t an option in stepwise refactoring — each move is a discrete edit.

  • The order doesn’t matter — the tool handles dependencies

    The tool handles individual references, but it doesn’t reorder your moves. Wrong order = broken intermediate states.

Fields first, methods second. Methods depend on fields; reverse order breaks references during the intermediate state. The tool’s per-move soundness depends on you sequencing correctly.

3. Spacing callback to Step 4. charge_monthly and charge_annual share most of their body. Should you Extract Function on them before moving them to BillingManager?

  • Yes — fewer methods to move = simpler refactoring

    Extracting before moving creates the helper on StreamingApp. Then that helper has to be moved too. Same end state, twice the moves.

  • No — move first; extract after, so the helper lives in BillingManager
  • Yes — extracting first is required by the Rule of Three

    Rule of Three doesn’t dictate ordering — it dictates whether to extract. Ordering is about minimizing rework.

  • No — the helper would have to be on StreamingApp anyway

    Extracting first puts the helper on StreamingApp; extracting after moving puts it on BillingManager. Where the helper lives matters.

Move first, extract second. Same code shape, but the extracted helper ends up where billing logic actually belongs. This is a sequencing optimization — both orderings produce correct code, but one of them avoids redundant work.

4. What does NOT count as fixing the God Class smell, even if it makes the file look cleaner?

  • Extracting a BillingManager class with the billing fields and methods

    This is the fix — that’s what this step did.

  • Renaming the methods so their names sort by cluster (bill_charge_monthly, etc.)
  • Moving the billing logic to a separate file

    Moving to a separate file is a real structural change (it forces an explicit import and decouples module compilation). Often paired with extraction.

  • Splitting the class into a Catalog and a Billing class via Move Method/Field

    Splitting into two classes is structural — the same fix, just with a different starting class kept.

Renaming is local cleanup, not decomposition. A class with 14 methods named with prefixes is still one class with 14 methods. The fields and methods remain coupled to the same self. Decomposition requires a new class to receive the migrated members. Watch for this confusion — “I cleaned it up by renaming” is a frequent self-deception about responsibility decomposition.

8

Replace Conditional with Polymorphism (with the tool)

When if track_kind == is the smell

Learning objective. After this step you will be able to (a) recognize a type-conditional that begs for polymorphism, (b) use Monaco’s Refactor: Move Method to migrate each branch into a subclass’s overridden method, and (c) explain when not to use polymorphism (a small match statement is sometimes fine).

Open tracks.py. The play function has an if/elif chain over track_kind:

def play(track_kind: str, track: Track, user: User) -> str:
    if track_kind == "song":
        # Songs respect repeat mode
        ...
    elif track_kind == "podcast":
        # Podcasts auto-resume from last position
        ...
    elif track_kind == "audiobook":
        # Audiobooks respect playback speed
        ...
    else:
        raise ValueError(f"Unknown track kind: {track_kind}")

Three branches, three different rules, one flat Track data class. This is type-conditional dispatch — the function asks “what kind of thing am I?” and chooses behavior. Every time you add a new track kind, this function grows by one elif.

Initial state

Track track_id duration_sec repeat_mode last_position playback_speed « function » "play(track_kind, track, user)" as play_fn if track_kind == song elif track_kind == podcast elif track_kind == audiobook

(One flat Track class with all the fields any kind of track might need; one play function with three branches dispatching on a string. After the refactor, Track becomes abstract and three subclasses each own their play behavior.)

A 30-second abstractmethod primer

Python’s abc module lets you declare a class as abstract — it can’t be instantiated directly. Subclasses must override the abstract methods to be instantiable.

from abc import ABC, abstractmethod

class Animal(ABC):
    @abstractmethod
    def speak(self) -> str:
        ...

class Dog(Animal):
    def speak(self) -> str:
        return "woof"

Animal()  # TypeError: Can't instantiate abstract class
Dog()     # OK

ABC (Abstract Base Class) is the marker. @abstractmethod says “every subclass MUST override this.” Together they enforce a contract at instantiation time, before any logic runs.

💡 We’re giving you the hierarchy shape on purpose. Designing class hierarchies from scratch is a separate skill (single-responsibility, Liskov substitution, type variance). For this step, we provide the abstract Track base + three subclass skeletons. Your task is the dispatch inversion — moving each play branch into the right subclass — not the hierarchy design.

How to apply this

The starter tracks.py already declares Track, Song, Podcast, Audiobook as a hierarchy with Track.play(user) as @abstractmethod. Run the dispatch micro-tests first to confirm polymorphism is wired correctly. Then, for each branch of the original play function, move the body into the corresponding subclass’s play(self, user) method (replace track.X with self.X). Finally, replace the original play function body with return track.play(user).

Tool-wise, this is just Refactor: Move Method done three times — same flow you used in Steps 6 and 7. Some branches may be easier to copy/paste because the conditional pattern doesn’t always select cleanly for tool-driven moves; that’s fine.

Spacing callback — Step 6’s Feature Envy in disguise

Each if track_kind == "X" branch in the original play function is a bit of code that uses only track-kind-specific data. By Step 6’s diagnostic (“uses zero state of host class”), each branch is Feature Envy on the Track-of-that-kind. The polymorphism refactoring is a special case of Move Method: instead of moving one method to one class, we move N methods to N subclasses dispatched by type.

Comparison — change locality (round two)

How many files change to add a new Live track type?

  • Before: edit tracks.py (add an elif), edit any caller, possibly edit tests for completeness.
  • After: add a new Live(Track) subclass with its own play. The play function and its callers don’t change.

That’s the Open/Closed Principle — open for extension (new subclasses), closed for modification (existing dispatch unchanged).

When polymorphism is not the right answer

Three branches dispatching on a string-valued type can be a small match statement instead. Decide based on:

  • Does the variation extend? If the type set is closed (e.g., HTTP status code categories — there are exactly four), polymorphism is over-engineered. A match works.
  • Does each branch carry its own state? If yes, polymorphism wins (state goes into the subclass). If branches are stateless calculations, match is simpler.
  • Will subclasses share more behavior over time? If the kinds will accumulate shared methods, the hierarchy pays back. If they’ll stay independent, polymorphism is bookkeeping.
Starter files
tracks.py
"""The track hierarchy — to be built up via Replace Conditional with Polymorphism."""
from abc import ABC, abstractmethod
from dataclasses import dataclass


@dataclass
class User:
    user_id: str


@dataclass
class Track(ABC):
    track_id: str
    duration_sec: int
    # All possible per-kind state (used by various subclasses)
    repeat_mode: bool = False
    last_position: int = 0
    playback_speed: float = 1.0

    @abstractmethod
    def play(self, user: User) -> str:
        """Each subclass owns its play behavior."""
        ...


@dataclass
class Song(Track):
    # TODO: implement play() — songs respect repeat_mode.
    # If repeat_mode is True, return f"playing song {self.track_id} on repeat"
    # otherwise return f"playing song {self.track_id}"
    def play(self, user: User) -> str:
        pass


@dataclass
class Podcast(Track):
    # TODO: implement play() — podcasts auto-resume from last_position.
    # Return f"resuming podcast {self.track_id} at {self.last_position}s"
    def play(self, user: User) -> str:
        pass


@dataclass
class Audiobook(Track):
    # TODO: implement play() — audiobooks adjust playback speed.
    # Return f"playing audiobook {self.track_id} at {self.playback_speed}x speed"
    pass


# The original procedural dispatcher — to be collapsed.
def play(track_kind: str, track: Track, user: User) -> str:
    """Procedural dispatch. After polymorphism, this collapses to one line."""
    if track_kind == "song":
        if track.repeat_mode:
            return f"playing song {track.track_id} on repeat"
        else:
            return f"playing song {track.track_id}"
    elif track_kind == "podcast":
        return f"resuming podcast {track.track_id} at {track.last_position}s"
    elif track_kind == "audiobook":
        return f"playing audiobook {track.track_id} at {track.playback_speed}x speed"
    else:
        raise ValueError(f"Unknown track kind: {track_kind}")
memo.md
# Refactoring memo — Step 8

## Issue
<!-- TODO: name the smell. Why is the if/elif chain over track_kind a smell? -->


## Rationale
<!-- TODO: why does Replace Conditional with Polymorphism fix it?
     What does the post-state achieve that the pre-state can't? -->


## Invariant
<!-- TODO: behavior preservation — what does play(...) still guarantee
     for each track kind after the refactor? -->


## Tests
<!-- TODO: which tests confirm the invariant?
     Hint: dispatch micro-tests + per-subclass behavior tests. -->
playback_loop.py
"""Plays a queue of tracks one after another.

Currently each call site has to pass the kind string explicitly:
`play("song", song_instance, user)`. After Replace Conditional with
Polymorphism, every call here can collapse to `track.play(user)` —
one polymorphic call regardless of subclass. Watch the lines shrink.
"""
from typing import List
from tracks import Track, Song, Podcast, Audiobook, User, play


class PlaybackLoop:
    """Drives a sequence of plays for a given user."""

    def __init__(self, user: User) -> None:
        self.user: User = user

    def play_one_of_each(
        self, song: Song, podcast: Podcast, audiobook: Audiobook
    ) -> List[str]:
        """Play one of each kind, returning the result strings.

        After polymorphism, this method can shrink to a single list
        comprehension over `[song, podcast, audiobook]` calling
        `t.play(self.user)` directly.
        """
        return [
            play("song", song, self.user),
            play("podcast", podcast, self.user),
            play("audiobook", audiobook, self.user),
        ]
test_tracks.py
"""Behavior tests for the polymorphic Track hierarchy and PlaybackLoop caller."""
import pytest
from tracks import Track, Song, Podcast, Audiobook, User, play
from playback_loop import PlaybackLoop


# ----- Dispatch micro-tests (run first) -----

def test_track_cannot_be_instantiated() -> None:
    """Track is abstract; instantiation should fail."""
    with pytest.raises(TypeError):
        Track(track_id="t0", duration_sec=10)


def test_subclass_dispatch_calls_subclass_play() -> None:
    """Calling play on a subclass instance dispatches to that subclass's method."""
    s: Song = Song(track_id="s1", duration_sec=180)
    p: Podcast = Podcast(track_id="p1", duration_sec=2400, last_position=300)
    # If the subclass overrides correctly, these don't error and don't return None.
    assert s.play(User("u1")) is not None
    assert p.play(User("u1")) is not None


# ----- Per-subclass behavior tests -----

def test_song_default_returns_plain_play_message() -> None:
    s: Song = Song(track_id="s1", duration_sec=180)
    assert s.play(User("u1")) == "playing song s1"


def test_song_with_repeat_mode_returns_repeat_message() -> None:
    s: Song = Song(track_id="s2", duration_sec=200, repeat_mode=True)
    assert s.play(User("u1")) == "playing song s2 on repeat"


def test_podcast_resumes_from_last_position() -> None:
    p: Podcast = Podcast(track_id="p1", duration_sec=2400, last_position=300)
    assert p.play(User("u1")) == "resuming podcast p1 at 300s"


def test_audiobook_uses_playback_speed() -> None:
    a: Audiobook = Audiobook(track_id="a1", duration_sec=3600, playback_speed=1.5)
    assert a.play(User("u1")) == "playing audiobook a1 at 1.5x speed"


# ----- The dispatcher collapses to a one-liner -----

def test_play_function_delegates_to_subclass() -> None:
    s: Song = Song(track_id="s1", duration_sec=180)
    # The procedural play() should now just call track.play(user).
    # Both call styles must agree.
    assert play("song", s, User("u1")) == s.play(User("u1"))


# ---- Caller test: PlaybackLoop must keep working across the polymorphism refactor ----

def test_playback_loop_runs_each_track_kind() -> None:
    loop: PlaybackLoop = PlaybackLoop(User("u1"))
    s: Song = Song(track_id="s1", duration_sec=180)
    p: Podcast = Podcast(track_id="p1", duration_sec=2400, last_position=300)
    a: Audiobook = Audiobook(track_id="a1", duration_sec=3600, playback_speed=1.5)
    results: list = loop.play_one_of_each(s, p, a)
    assert results == [
        "playing song s1",
        "resuming podcast p1 at 300s",
        "playing audiobook a1 at 1.5x speed",
    ]

if __name__ == "__main__":
    import sys
    pytest.main([sys.argv[0], "-v"])

Step 8 — Replace Conditional with Polymorphism

Min. score: 70%

1. Retrieval / dispatch. When play(user) is called on a Podcast instance, which method runs?

  • Track.play (the abstract base method)

    The abstract method’s body never runs — it’s a contract, not an implementation. Subclasses must override; the override is what runs.

  • Podcast.play (the override)
  • Both — Track.play runs first, then Podcast.play

    MRO doesn’t ‘chain’ calls. Without an explicit super().play(user) in the override, only the override runs.

  • Neither — abstract methods raise NotImplementedError

    Abstract methods raise TypeError at instantiation, not NotImplementedError at call. And only if no override exists.

The override runs. Podcast.play overrides Track.play, so the dynamic dispatch from track.play(user) resolves to Podcast.play. This is the same mechanism C++ calls “virtual”; Python’s methods are virtual by default.

2. Is polymorphism always the right fix for if/elif chains over a type field?

  • Yes — type-conditionals are always a smell

    Always-rules in design are usually wrong. Type-conditionals over closed, stable, stateless type sets (e.g., HTTP status code categories: informational/success/redirect/error) are fine as match.

  • No — for a small closed set of types with no shared state, a match statement is simpler
  • Yes — Python’s match statement is just syntactic sugar for polymorphism

    match and polymorphism are different mechanisms. match is structural pattern matching at the call site; polymorphism is dynamic dispatch via inheritance. They have different change-locality properties.

  • Only if the conditional is more than 5 branches long

    Length is a heuristic, not a rule. Three branches that each carry state can be polymorphism; ten branches that each return one value can be match.

It depends on extension and state. Polymorphism wins when the type set will grow and each type carries its own state. match wins when the type set is closed and the branches are pure dispatch. Both can be valid; choose based on the change vector.

3. What does @abstractmethod actually enforce?

  • Subclasses MUST override it; trying to instantiate a subclass that doesn’t raises TypeError
  • The method body must be empty

    The body can have any content (often a docstring or ...). The body is sometimes used as a default implementation called from a subclass via super().

  • Subclasses cannot call super() on an abstract method

    Subclasses can call super().method() on an abstract method to access a default implementation, if the abstract base provided one.

  • The method can only be called via the abstract base class

    Once a subclass overrides the abstract method, calling on the subclass instance dispatches normally.

Enforcement is at instantiation. @abstractmethod makes the abstract base un-instantiable; subclasses without all required overrides are also un-instantiable. The error message names the missing methods, which is far better than discovering at call time that the method does nothing.

4. Spacing back to Step 6. Why is moving each conditional branch into its corresponding subclass a special case of Move Method?

  • It’s not — polymorphism is a different refactoring entirely

    It is the same shape: a method body that uses only foreign data is moved closer to that data. Polymorphism just generalizes ‘foreign class’ to ‘subtype.’

  • Each branch uses only data of one kind of track — Feature Envy on the subclass
  • Move Method requires concrete classes; abstract subclasses don’t qualify

    Move Method works fine across an inheritance hierarchy. The receiving classes are concrete subclasses (Song, Podcast, Audiobook).

  • Polymorphism is a structural change; Move Method is a behavioral change

    Both refactorings are structural — they change where code lives without changing what it does. Behavior preservation is the rule for both.

Same diagnostic, different host. Step 6’s Feature Envy (“uses zero state of host class”) generalized: each branch of the conditional uses only state of its respective subclass. Polymorphism is Move Method dispatched by type.

9

Hotspots & The Boy Scout Rule

When refactoring earns its keep — and when it doesn’t

Learning objective. After this step you will be able to (a) explain why “always refactor” and “never refactor” are both wrong, (b) name two anti-rules — speculative generality and premature abstraction — that punish over-eager refactoring, and (c) apply the hotspot rule: refactor where the code is already changing.

Eight steps in, you’ve learned six refactorings (Extract Function, Boolean simplification, parameterised Extract, Introduce Parameter Object, Move Method, Extract Class) and one big idea (Replace Conditional with Polymorphism). With this much hammering, every line of code starts to look like a nail.

It isn’t.

Two anti-rules

Speculative generality is the smell of refactoring for an extension that never comes. A class StripePaymentPlugin(PaymentPlugin) with no second implementation, no plan for one, and no tests demonstrating multi-plugin behavior is just complexity. The abstraction might be useful if you ever need a second plugin. Most of the time, you don’t — and the code is harder to read while you wait for that future use.

Premature abstraction (a sibling smell) is over-eagerly extracting on the second occurrence of a pattern instead of the third. The Step-4 quiz hit this: with two duplicates the variation might be obvious or might be misleading. The Rule of Three exists because the third occurrence reveals what’s truly common vs. accidentally similar.

Both anti-rules are reactions to the same human pattern: developers like to feel “future-proof,” and refactoring tools make abstraction cheap. The cost of unused abstraction is paid daily by every reader who has to mentally instantiate the abstract types in their head before understanding the concrete code.

The hotspot rule

A hotspot is code that is already changing — high churn, frequent bug fixes, recent feature work. Refactoring buys you the most when applied to hotspots:

  • You’re going to read the code anyway (because you’re changing it).
  • The cleanup is amortized over many future changes (because the code keeps changing).
  • The behavior preservation tests are most likely to exist on hotspot code (because someone wrote them recently).

Refactoring cold code — files nobody has touched for two years — is rarely worth the time. The code “looks ugly,” but nobody’s reading it; nobody’s changing it; nobody benefits from the cleanup. The cost is paid; the benefit isn’t realized.

The opposite extreme is also wrong. “Never refactor” means every hotspot accrues debt that compounds with every change — bug fixes take longer, new features ship slower, the team’s tolerance for the code degrades. Eventually the codebase becomes the kind of thing you can only rewrite, not edit. That’s not a virtue; it’s a failure mode.

The Boy Scout Rule

Robert C. Martin’s formulation: “Always leave the campground cleaner than you found it.” Applied to code: small, incremental, concurrent improvements bundled with whatever change is already happening — one Extract Method, one renamed variable, one collapsed boolean. The Boy Scout rule (clean while changing) and the hotspot rule (clean where changing) are the same principle from two angles, and together they reject “always” and “never” in favor of opportunistic improvement.

A misconception to avoid: “any cleaner loop is interchangeable”

One refactoring that looks innocuous but routinely breaks behavior: replacing an indexed for loop with a for x in iterable loop without checking what the index was doing. The classic example:

# Original: sums every other element starting at index 0
total = 0
for i in range(0, len(values), 2):
    total += values[i]

# "Cleaner" but broken: now sums every element
total = 0
for v in values:
    total += v

The two loops look similar, the linter might even prefer the second, and the tests on small inputs may pass. But the index-stride semantics is gone. Documented in Oliveira/Keuning/Jeuring (2023) as one of the most common refactoring misconceptions among CS students: students replace for i in range(...) with for v in iterable because the latter is “more Pythonic,” without checking whether the original index pattern was load-bearing.

The rule: before you simplify a loop, ask what the original was doing with the index. If the answer is “nothing” — the index was unused or only used to access iterable[i] sequentially — the simplification is safe. If the index was strided, reversed, paired with another sequence, or used for enumerate-like positional logic, the simplification is a behavior change.

Putting numbers on it

One large empirical study (Palomba et al., 2018) tracked 395 releases of 30 projects. Smelly classes had a median change-proneness of 32 vs. 12 for non-smelly ones; fault-proneness of 9 vs. 3. When three smells co-occurred in a class, change-proneness rose to a median of 54 and faults to 12. The data say: smells are real, smells in hotspots are expensive, and the strongest ROI is fixing co-occurring smells in churning code.

No code task this step — read, internalize, then quiz. Step 10 (the next one) puts the judgment to work on a snippet with multiple smells coexisting.

Starter files

Step 9 — Hotspots & The Boy Scout Rule

Min. score: 70%

1. Retrieval. Which of these scenarios would you defer refactoring for? (select all that apply)

  • Code in a file that hasn’t been touched in two years and isn’t on your roadmap
  • Code you’re about to add a feature to next sprint

    Code about to be changed is the textbook hotspot — refactor while editing it. Boy Scout rule.

  • Code with frequent bug fixes in the last six months

    Frequent bug fixes are a strong hotspot signal. The smelly structure is causing the bugs; refactoring reduces future bug rates.

  • Code with no tests

    Lack of tests is a concern before refactoring (write characterization tests first), but not a reason to defer indefinitely. Address the test gap and then refactor.

  • A class with three different smells that co-occur

    Co-occurring smells in one class are exactly what the empirical research flags as the highest-cost configuration. Don’t defer; address.

Cold code is the only one to defer. Hotspots, soon-to-be-touched code, and co-occurring-smell hotspots all justify refactoring effort. Lack of tests is a side-quest (write tests first), not a deferral.

2. What’s wrong with the rule “always refactor when you see a smell”?

  • It produces too much code churn

    Churn is a consequence of the deeper problem. The deeper problem is that not every smell warrants a fix in every context.

  • It leads to speculative generality — abstractions for extensions that never come
  • It’s too lenient — the rule should be stricter

    Stricter rules don’t help — strict + wrong produces speculative generality faster.

  • Smells are subjective; you can’t always agree what’s a smell

    Smell subjectivity is real but it’s not the failure mode of ‘always refactor.’ The failure mode is that some smells are not actually problems in their context.

Speculative generality and premature abstraction. “Always refactor” produces abstractions that anticipate extension that never arrives. The reader pays the cost of the abstraction every day; the imagined benefit may never come.

3. What’s wrong with the rule “never refactor working code”?

  • It produces ugly code that’s hard to read

    True, but partial — ugliness alone isn’t the structural problem.

  • It lets hotspots accrue debt that compounds with every change

    True — and the most actionable version. Hotspot code keeps changing; without cleanup, each change becomes harder than the last.

  • It violates the Boy Scout Rule

    True — Robert Martin’s rule says leave each campground (any file being touched) cleaner. ‘Never refactor’ is the opposite of that discipline.

  • All of the above

All three are facets of the same failure. Pure “never refactor” produces compounding debt in exactly the places where reducing it pays back the most.

4. The Boy Scout Rule, applied to code, says:

  • Refactor every file you touch, completely, before committing

    Complete refactors are big risky changes. The Boy Scout rule is small improvements bundled with whatever work was already happening.

  • Leave each file you touch a little cleaner than you found it
  • Never commit code that lints with warnings

    Linter compliance is a different discipline (style, not structure). The Boy Scout rule is about structural quality, not surface style.

  • Always rewrite code that you don’t understand

    Rewriting unfamiliar code is dangerous — there’s no safety net of a working baseline. Read, write tests, then refactor incrementally.

Small, incremental, concurrent. The Boy Scout rule trades coordinated big-bang refactors for many small per-task improvements. Combined with the hotspot rule, it covers most of the value of refactoring at almost none of the coordination cost.

5. Loop-replacement misconception. Consider: 

# Original
total = 0
for i in range(0, len(values), 2):
    total += values[i]
A reviewer suggests replacing it with: 
# Suggested
total = 0
for v in values:
    total += v
Is the suggested replacement a safe refactoring?

  • Yes — it’s the same loop with cleaner syntax

    The two loops look similar but the original uses range(0, len(values), 2) — the third argument is the stride. The replacement loses that, summing every element instead of every other one.

  • Yes — the type checker would catch any difference

    Type checkers detect type errors, not semantic ones. Both loops have the same types in and out; the difference is in which elements get summed.

  • No — the original strides by 2; the replacement sums every element
  • Only if values is a list, not a tuple

    The container type is unrelated to the stride semantics. The bug is the same whether values is a list, tuple, or generator.

Strided loops aren’t interchangeable with foreach. Before simplifying a loop, ask what the index was doing. Strided, reversed, paired-with-another-sequence, or used-for-position — all four patterns break under naive for x in iterable replacement. This is a documented misconception (Oliveira/Keuning/Jeuring 2023) that survives all the way into professional code.

10

The Refactoring Memo (Synthesis)

Putting it all together

Learning objective. After this step you will be able to (a) diagnose multiple smells coexisting in a piece of unfamiliar code, (b) choose which refactoring to apply first based on the dependencies between smells, and (c) defend the choice in a four-field refactoring memo before touching any code.

Open playlist.py. The class PlaylistManager has multiple smells stacked together. Your job is to (a) diagnose them, (b) choose one refactoring to apply, (c) write the memo, and only then (d) execute the refactoring with the tool.

Warm-up — interleaved diagnosis

Before you look at playlist.py, name the smell and refactoring that fits each of the four snippets below. No code changes — just diagnose. Each snippet has exactly one dominant smell from earlier steps.

Snippet A

def shuffle_score(
    plays: int,
    age: int,
    last_played: int,
    mood: str,
    weather: str,
    time_of_day: str,
) -> float:
    base: float = 0.0
    if mood == "energetic" and weather == "sunny":
        base = plays / age
    elif mood == "calm":
        base = plays / (age + last_played)
    # ... 8 more elifs ...
    return base

Snippet B

def update_metadata(
    track: Track,
    title: str,
    artist: str,
    album: str,
    year: int,
    genre: str,
    label: str,
) -> None:
    track.title = title
    track.artist = artist
    ...

Snippet C

class Statistics:
    def avg_track_length(self, tracks: List[Track]) -> float:
        total: int = sum(t.duration_sec for t in tracks)
        return total / len(tracks)

Snippet D

def is_hi_res(track: Track) -> bool:
    if track.bitrate >= 1411:
        return True
    else:
        return False

Snippet E (the foil)

class StripeProcessor:
    def charge(self, amount_cents: int, currency: str) -> bool:
        return self._gateway.charge(amount_cents, currency)

class PayPalProcessor:
    def charge(self, amount_cents: int, currency: str) -> bool:
        return self._gateway.charge(amount_cents, currency)

Two near-identical classes — same method shape, same body. Tempting to extract a base class or polymorphism, but this might be fine. Each processor has its own SDK, error handling, and configuration that isn’t shown here. The visual similarity is shallow.

Hold all five diagnoses in your head as you read on. The quiz will check whether you can discriminate real smells from looks-similar-but-fine.

The synthesis snippet — playlist.py

PlaylistManager.add_to_playlist(...) is the function we’ll refactor. Smells stacked in it (in plain English):

  1. Long Parameter List — eight parameters, several of which travel as a clump.
  2. Long Method — three sub-goals (validation, deduplication, persistence) jammed together.
  3. Duplicated Code — the validation block is duplicated in add_to_queue.

Two plausible refactorings — pick ONE

Option What it tackles first Trade-off
A: Introduce Parameter Object on the eight parameters first Long Parameter List Cleaner signatures, but the duplicated validation block is still duplicated
B: Extract Function on the validation block first Long Method + Duplication Smaller methods, both add_to_playlist and add_to_queue deduped, but the eight-parameter signature is still long

Either choice is defensible. The tutorial doesn’t grade which choice you make — it grades the memo you write to defend it.

The memo — fully blank this time

Open memo.md. All four sections are empty. You write all four:

  • Issue — name the smell(s) in plain English.
  • Rationale — explain why your chosen refactoring is the right first step. (You can mention follow-up refactorings, but commit to one first move.)
  • Invariant — what behavior must be preserved across the refactor?
  • Tests — which tests in test_playlist.py confirm the invariant?

A reference card at the bottom of memo.md defines each field in one sentence in case you need to refresh.

Then execute

After writing the memo, invoke the appropriate tool action:

  • For Option A: place cursor in the parameter list, Refactor: Introduce Parameter Object, name it TrackInfo, bundle the four album-related parameters.
  • For Option B: select the validation block in add_to_playlist, Refactor: Extract Function/Method, name it _validate_track_data. Then replace the duplicated block in add_to_queue with a call to the new helper.

Run the tests after the refactor. They must still pass.

What’s next after this tutorial

You’ve covered method-level smells (Long Method, Duplication, Boolean anti-patterns), data-level smells (Long Parameter List, Feature Envy), and class-level smells (God Class, type-conditional dispatch). The next layer is design principles — the rules of thumb that make smells less likely to appear in the first place. Two suggested follow-ups:

  • SOLID design principles tutorial — single-responsibility, open/closed, Liskov substitution, interface segregation, dependency inversion. The vocabulary the smells in this tutorial implicitly invoked.
  • Observer pattern tutorial — one of many design patterns; an example of how design principles solidify into reusable structural recipes.
Starter files
playlist.py
"""The playlist manager — multiple smells stacked together."""
from typing import Dict, List


class PlaylistManager:
    def __init__(self) -> None:
        self.playlist: List[Dict] = []
        self.queue: List[Dict] = []

    def add_to_playlist(
        self,
        title: str,
        artist: str,
        album: str,
        year: int,
        genre: str,
        duration_sec: int,
        bpm: int,
        isrc: str,
    ) -> Dict:
        # Validation
        if not title or not isinstance(title, str):
            raise ValueError("title is required")
        if duration_sec <= 0:
            raise ValueError("duration_sec must be positive")
        if bpm <= 0:
            raise ValueError("bpm must be positive")

        # Deduplication
        for existing in self.playlist:
            if existing["isrc"] == isrc:
                return existing

        # Persistence
        record: Dict = {
            "title": title, "artist": artist, "album": album,
            "year": year, "genre": genre, "duration_sec": duration_sec,
            "bpm": bpm, "isrc": isrc,
        }
        self.playlist.append(record)
        return record

    def add_to_queue(
        self,
        title: str,
        artist: str,
        album: str,
        year: int,
        genre: str,
        duration_sec: int,
        bpm: int,
        isrc: str,
    ) -> Dict:
        # Validation (DUPLICATED from add_to_playlist)
        if not title or not isinstance(title, str):
            raise ValueError("title is required")
        if duration_sec <= 0:
            raise ValueError("duration_sec must be positive")
        if bpm <= 0:
            raise ValueError("bpm must be positive")

        record: Dict = {
            "title": title, "artist": artist, "album": album,
            "year": year, "genre": genre, "duration_sec": duration_sec,
            "bpm": bpm, "isrc": isrc,
        }
        self.queue.append(record)
        return record
importer.py
"""Bulk-imports tracks into a PlaylistManager.

If you pick Option A (Introduce Parameter Object), every call site here
will need to pass a `TrackInfo` instead of the four album-related fields
flat. If you pick Option B (Extract Function on validation), the call
shape stays the same — but the helper inside PlaylistManager is shared.
Either way: the tests below exercise the caller across the refactor.
"""
from typing import Any, Dict, List
from playlist import PlaylistManager


class TrackImporter:
    """Bulk-imports a list of track records into a playlist or queue."""

    def __init__(self, manager: PlaylistManager) -> None:
        self.manager: PlaylistManager = manager

    def import_to_playlist(self, tracks: List[Dict[str, Any]]) -> int:
        """Add each track dict to the underlying playlist. Returns count added."""
        count: int = 0
        for t in tracks:
            self.manager.add_to_playlist(
                title=t["title"],
                artist=t["artist"],
                album=t["album"],
                year=t["year"],
                genre=t["genre"],
                duration_sec=t["duration_sec"],
                bpm=t["bpm"],
                isrc=t["isrc"],
            )
            count += 1
        return count
test_playlist.py
"""Behavior tests for PlaylistManager and the TrackImporter caller."""
import pytest
from typing import Any, Dict, List
from playlist import PlaylistManager
from importer import TrackImporter


@pytest.fixture
def pm() -> PlaylistManager:
    return PlaylistManager()


def _track_args(**overrides: Any) -> Dict[str, Any]:
    base: Dict[str, Any] = dict(
        title="Echoes", artist="Alice", album="Reflections", year=2022,
        genre="indie", duration_sec=200, bpm=110, isrc="ISRC001",
    )
    base.update(overrides)
    return base


def test_add_to_playlist_returns_record(pm: PlaylistManager) -> None:
    # The student may need to switch to a TrackInfo object after the refactor.
    args: Dict[str, Any] = _track_args()
    # Try keyword args first; if Refactoring made it positional, fall back.
    try:
        rec: Dict = pm.add_to_playlist(**args)
    except TypeError:
        # Refactor introduced a parameter object — try the object form.
        from playlist import TrackInfo  # may not exist before refactor
        rec = pm.add_to_playlist(
            title=args["title"],
            track_info=TrackInfo(artist=args["artist"], album=args["album"],
                                 year=args["year"], genre=args["genre"]),
            duration_sec=args["duration_sec"],
            bpm=args["bpm"],
            isrc=args["isrc"],
        )
    assert rec["title"] == "Echoes"
    assert rec["isrc"] == "ISRC001"


def test_add_to_playlist_validates_title(pm: PlaylistManager) -> None:
    with pytest.raises(ValueError, match="title"):
        args: Dict[str, Any] = _track_args(title="")
        try:
            pm.add_to_playlist(**args)
        except TypeError:
            from playlist import TrackInfo
            pm.add_to_playlist(
                title="",
                track_info=TrackInfo(artist=args["artist"], album=args["album"],
                                     year=args["year"], genre=args["genre"]),
                duration_sec=args["duration_sec"], bpm=args["bpm"], isrc=args["isrc"],
            )


def test_add_to_playlist_validates_duration(pm: PlaylistManager) -> None:
    with pytest.raises(ValueError, match="duration"):
        args: Dict[str, Any] = _track_args(duration_sec=0)
        try:
            pm.add_to_playlist(**args)
        except TypeError:
            from playlist import TrackInfo
            pm.add_to_playlist(
                title=args["title"],
                track_info=TrackInfo(artist=args["artist"], album=args["album"],
                                     year=args["year"], genre=args["genre"]),
                duration_sec=0, bpm=args["bpm"], isrc=args["isrc"],
            )


def test_dedup_returns_existing_record(pm: PlaylistManager) -> None:
    args: Dict[str, Any] = _track_args()
    try:
        rec1: Dict = pm.add_to_playlist(**args)
        rec2: Dict = pm.add_to_playlist(**args)
    except TypeError:
        from playlist import TrackInfo
        ti = TrackInfo(artist=args["artist"], album=args["album"],
                       year=args["year"], genre=args["genre"])
        rec1 = pm.add_to_playlist(title=args["title"], track_info=ti,
                                  duration_sec=args["duration_sec"], bpm=args["bpm"],
                                  isrc=args["isrc"])
        rec2 = pm.add_to_playlist(title=args["title"], track_info=ti,
                                  duration_sec=args["duration_sec"], bpm=args["bpm"],
                                  isrc=args["isrc"])
    assert rec1 is rec2  # same dict returned


def test_add_to_queue_validates_title(pm: PlaylistManager) -> None:
    with pytest.raises(ValueError, match="title"):
        args: Dict[str, Any] = _track_args(title="")
        try:
            pm.add_to_queue(**args)
        except TypeError:
            from playlist import TrackInfo
            pm.add_to_queue(
                title="",
                track_info=TrackInfo(artist=args["artist"], album=args["album"],
                                     year=args["year"], genre=args["genre"]),
                duration_sec=args["duration_sec"], bpm=args["bpm"], isrc=args["isrc"],
            )


# ---- Caller test: TrackImporter must keep working across the refactor ----

def test_track_importer_bulk_imports(pm: PlaylistManager) -> None:
    importer: TrackImporter = TrackImporter(pm)
    records: List[Dict[str, Any]] = [_track_args(isrc="A"), _track_args(isrc="B")]
    # If the student picked Option A (Parameter Object), the importer's
    # call site needs to be updated to pass TrackInfo. We support both.
    try:
        added: int = importer.import_to_playlist(records)
    except TypeError:
        pytest.skip("Importer needs updating for Option A — see importer.py")
        return
    assert added == 2
    assert len(pm.playlist) == 2

if __name__ == "__main__":
    import sys
    pytest.main([sys.argv[0], "-v"])
memo.md
# Refactoring memo — Step 10

## Issue
<!-- Name the smell(s) you see in playlist.py. There are at least three. -->


## Rationale
<!-- Which refactoring will you apply FIRST? Why is that the right first step
     given the smells you named? You may mention follow-up refactorings. -->


## Invariant
<!-- What property of behavior must be preserved across the refactor?
     (Hint: external API contract — same inputs produce same outputs.) -->


## Tests
<!-- List which tests in test_playlist.py confirm the invariant. -->


---

### Reference card

| Field | One-sentence definition |
|---|---|
| **Issue** | The smell present in the original code. |
| **Rationale** | Why this refactoring is the right fix, given the smell. |
| **Invariant** | The behavior property preserved across the refactor. |
| **Tests** | The tests that confirm the invariant. |

Step 10 — Synthesis (Cumulative)

Min. score: 70%

1. Cumulative classification. Which smells appear in the original PlaylistManager.add_to_playlist? (select all that apply)

  • Long Parameter List
  • Long Method
  • Duplicated Code
  • Feature Envy

    Feature Envy is when a method uses the data of another class. add_to_playlist uses self.playlist — its own state.

  • God Class

    PlaylistManager has two responsibilities (playlist + queue), but they’re closely related and the class is small. Not a God Class — yet.

  • Speculative Generality

    Speculative generality is over-eager abstraction. add_to_playlist has the opposite problem (under-abstracted).

  • Boolean anti-pattern (return True / return False)

    No if cond: return True else: return False pattern in this snippet.

Three smells stacked: Long Parameter List + Long Method + Duplicated Code. Recognizing them simultaneously is the synthesis skill. Each previous step taught one in isolation; here they coexist.

2. Interleaved warmup retrieval. Snippet A (the shuffle_score function with if mood == "energetic" and weather == "sunny" branches) most resembles which smell from this tutorial?

  • Long Method (Step 2)

    Long Method is the secondary smell, but the dominant pattern is the conditional dispatch — multiple branches choosing behavior.

  • Boolean anti-pattern (Step 3)

    The if/and/elif structure isn’t simplifiable to a boolean return — the branches do different things, not just return different booleans.

  • Long Parameter List (Step 5)

    Six parameters is borderline-long but not the dominant smell. The dominant pattern is the dispatch.

  • Type-conditional dispatch — Replace Conditional with Polymorphism (Step 8)

Type-conditional dispatch. When if mood == "X" chooses behavior, you have a polymorphism candidate. Each mood/weather pair is dispatching to different scoring logic — same shape as the track_kind example in Step 8.

3. Interleaved warmup retrieval. Snippet B (update_metadata with seven parameters that travel together) most resembles which smell?

  • Long Parameter List + data clump (Step 5)
  • Feature Envy (Step 6)

    Feature Envy would require the method to use only foreign data. update_metadata uses both track (foreign) and the parameters; it’s the parameters’ shape that’s the smell, not the method’s location.

  • Long Method (Step 2)

    The method is short — it’s just assignments. Length isn’t the issue.

  • Speculative Generality (Step 9)

    Speculative generality is about unused abstractions. Here the abstraction is missing entirely.

Long Parameter List with a data clump. Six of the seven parameters describe album metadata — they always travel together. The fix is Introduce Parameter Object.

4. Interleaved warmup retrieval. Snippet D (is_hi_res with if track.bitrate >= 1411: return True else: return False) most resembles which smell?

  • Long Method

    Three lines isn’t long.

  • Boolean-return anti-pattern (Step 3)
  • Feature Envy (Step 6)

    Feature Envy might be relevant — is_hi_res uses only track data — but the dominant smell here is the boolean wrapping.

  • Long Parameter List

    One parameter isn’t long.

Classic boolean-return anti-pattern. The condition is already a boolean; wrapping it in if/else with return True / return False adds nothing. Simplify to return track.bitrate >= 1411. (And maybe also Move Method onto Track, but that’s secondary — Snippet D’s primary smell is boolean.)

5. Snippet C (Statistics class) retrieval. Statistics.avg_track_length(tracks) uses zero Statistics state — it iterates over the parameter tracks and computes a sum. Which earlier-step smell does this most resemble?

  • Long Method (Step 2)

    The body is short — three lines of computation. Length isn’t the issue.

  • Long Parameter List (Step 5)

    One parameter — the opposite of long parameter list.

  • Feature Envy (Step 6)
  • God Class (Step 7)

    A class with one method isn’t a God Class; it’s the opposite (small, focused, possibly Lazy).

Feature Envy. The method uses zero state of its host (Statistics) and only data from the parameter tracks. Step 6’s diagnostic — “uses zero host state” — fires cleanly here. The likely fix is Move Method onto whichever class owns the track collection.

6. Snippet E (the foil) — discrimination check. Two near-identical processor classes (StripeProcessor, PayPalProcessor) with the same charge signature and similar body. Should you extract a common base class or apply Replace Conditional with Polymorphism?

  • Yes — duplicated code is always a smell

    Visual similarity isn’t bug-coupling. Step 1’s currency-rounding non-example showed the same pattern: three look-alike functions with independent domain rules. Extract only when the variation is one stable dimension and a real change forces lockstep edits in both copies.

  • Yes — the Open/Closed Principle requires polymorphism here

    Open/Closed says be open to extension; it doesn’t require every type-similar pair to share a base class. If the processors evolve independently, a shared base is over-coupling, not abstraction.

  • Probably not — each processor likely has different SDK/error/config concerns the visual similarity hides
  • Yes — this is exactly the case Step 8 trained for

    Step 8 trained for cases where each branch reads/writes different fields and the dispatch on a string-typed kind is the smell. Two classes that already exist, with their own state and SDK, aren’t the same shape.

Variation Theory in action. “Looks duplicated” can mean three different things: (1) actual duplication that should be extracted; (2) visual similarity over independent domain rules (currency rounding from Step 1); or (3) parallel infrastructure with hidden differences (this snippet). Discriminating between them is the judgment Step 9’s hotspot/Boy-Scout framing was for. When in doubt, wait for the third occurrence and a real bug-coupling signal before extracting.

7. Step 1 callback — fix-then-refactor. During the synthesis you spot a bug in one of the duplicated validation blocks of playlist.py. What’s the right discipline?

  • Refactor first to extract the validation; the helper now needs only one fix

    Extracting with the bug intact consolidates the bug into the helper — same bug, harder-to-spot location. The helper now contains corrupt logic that future readers will trust.

  • Fix the bug in both duplicated locations first; then refactor to extract
  • Fix one location and leave the other; it’s about to be deleted by the refactor anyway

    Tempting reasoning, but the moment between ‘one fix done, refactor in progress’ and ‘refactor complete’ is exactly when a teammate might pull the branch and run the buggy duplicate.

  • Run the tests; if they pass, the bug isn’t real

    Passing tests don’t mean the bug isn’t real — they mean the bug isn’t in the test cases yet. Step 1’s lesson was that test coverage and bug presence are independent.

Fix-then-refactor. Step 1’s load-bearing rule generalizes across the entire toolkit: never refactor over a known bug. Fix in each location with green tests, then consolidate. This sequencing is one of the strongest rules in the discipline.

8. Memo synthesis. Why is the memo important — couldn’t you just refactor and let the diff speak for itself?

  • The memo is required for the assessment in this tutorial

    True for this step, but it’s not the deeper reason. The memo is also valuable in industry settings — code reviewers, future maintainers, and a future self all benefit.

  • The memo captures the reasoning behind the change, which the diff alone cannot show
  • Without the memo, the tool refuses to apply the refactoring

    The tool doesn’t read memos. The memo is for humans.

  • Memos are a CS convention; every PR must include one

    Memo conventions vary by team. The discipline of naming the invariant is what generalizes.

The memo answers questions a diff can’t. What smell? Why this refactoring? What invariant? Which tests confirm? A reviewer reading the diff sees what changed; the memo tells them why the change is correct. That’s the difference between a reviewable refactor and a trust-me one.