FreeCampus Python

Refactor in Small, Verified Steps

Improve names, functions, conditionals, duplication, and side-effect boundaries through small diffs that preserve checked behavior.
python-foundations code-quality-maintainability refactoring maintainability
Open in Colab
  • Level: Python Foundations
  • Estimated time: 4–5 hours
  • You will learn: Name a concrete maintenance cost, make one structural change, and use tests, types, lint, formatting, and diffs to show that observable behavior stayed stable.
  • Practice in: The complete Midnight Museum Git project and quality gate

Open this lesson in Google Colab

1. Refactoring changes structure on purpose

The Midnight Museum scorer works and its tests pass. A new requirement is coming: different galleries will apply different bonus rules. The current function combines validation, scoring, sorting, and text output:

def build_board(attempts):
    rows = []
    for attempt in attempts:
        if not attempt["team"].strip():
            raise ValueError("team cannot be empty")
        if not 0 <= attempt["clues_found"] <= 8:
            raise ValueError("clues_found must be between 0 and 8")
        score = attempt["clues_found"] * 30
        score += 25 if attempt["bonus_clue"] else 0
        score -= max(0, attempt["elapsed_minutes"] - 45) * 2
        rows.append((max(0, score), attempt["team"]))
    rows.sort(reverse=True)
    return "\n".join(f"{team}: {score}" for score, team in rows)

Before editing, answer:

  1. Which observable behaviors already belong to the contract?
  2. What specific maintenance task is expensive in the current structure?
  3. Which smallest structural move would reduce that cost?
  4. Which focused check should run immediately after the move?
  5. Which final gate and diff will show the complete result?

Refactoring changes internal structure without intentionally changing observable behavior. These are not the same activity:

  • changing the bonus from 25 to 40 is a behavior change;
  • repairing a wrong tie order is a bug fix;
  • formatting layout is a mechanical style pass;
  • renaming a private local without changing output is a refactor; and
  • replacing the whole project from memory is a rewrite, not a sequence of controlled refactors.

Separate behavior changes from structural changes when possible. A reviewer can then ask one clear question of each patch.

2. Freeze the behavior you intend to preserve

Start from a known commit and run the complete gate:

git status --short
pytest -q
ruff check src tests
ruff format --check src tests
mypy src

Write down the exact result. Then identify behavior that matters to callers:

  • public function names and accepted arguments;
  • returned values and types;
  • exception types and messages promised by the interface;
  • output text and ordering;
  • whether caller-owned collections are mutated;
  • file, terminal, or collaborator side effects; and
  • performance only when it is part of a real requirement.

A characterization test records important existing behavior when the original intent is unclear. It is not an endorsement of the behavior; it creates a safety boundary for structural work. If the behavior is wrong, change it later with an explicit new requirement and test.

def test_build_board_preserves_current_tie_order() -> None:
    attempts = [
        {"team": "Amber", "clues_found": 2, "elapsed_minutes": 40, "bonus_clue": False},
        {"team": "Blue", "clues_found": 2, "elapsed_minutes": 40, "bonus_clue": False},
    ]
    assert build_board(attempts) == "Blue: 60\nAmber: 60"

The descending team tie order may be surprising. Preserve it during the refactor. A later product decision can change it to alphabetical ascending with a separate failing test and release note.

Tests protect selected behavior, not intention by magic

A weak suite may miss input mutation:

def test_board_text() -> None:
    attempts = [
        {"team": "Moon Moths", "clues_found": 6, "elapsed_minutes": 42, "bonus_clue": True},
    ]
    assert build_board(attempts) == "Moon Moths: 205"

Add the ownership boundary if callers rely on it:

from copy import deepcopy


def test_build_board_does_not_mutate_attempts() -> None:
    attempts = [
        {"team": "Moon Moths", "clues_found": 6, "elapsed_minutes": 42, "bonus_clue": True},
    ]
    original = deepcopy(attempts)
    build_board(attempts)
    assert attempts == original

Do not write assertions for every private line. Protect the behavior a caller or collaborator can observe.

Checkpoint: define the change boundary

3. Make one named move and stop

A safe rhythm is deliberately small:

  1. state the maintenance problem;
  2. choose one refactoring move;
  3. make only that move;
  4. run the smallest relevant check;
  5. inspect the diff;
  6. run the broader gate at a meaningful checkpoint; and
  7. commit or continue only when the state is understood.

Each verified move returns to a known state before another structural decision begins.

flowchart LR
  A[Green baseline] --> B[One named refactor]
  B --> C[Focused check]
  C -- Fail --> D[Inspect earliest difference]
  D --> B
  C -- Pass --> E[Review diff]
  E --> F[Full gate]
  F --> G[Next move or stop]

Rename for the represented concept

Rename a private variable and all its uses together:

def overtime_penalty(elapsed_minutes: int) -> int:
    overtime_minutes = max(0, elapsed_minutes - 45)
    return overtime_minutes * 2

Avoid search-and-replace across unrelated time names. Editor rename tools and static checks help, but inspect the diff to ensure strings, docs, public names, or other scopes did not change accidentally.

Extract a named intermediate value

This expression is correct but makes three rules compete:

score = max(0, attempt["clues_found"] * 30 + (25 if attempt["bonus_clue"] else 0) - max(0, attempt["elapsed_minutes"] - 45) * 2)

Expose the stages:

clue_points = attempt["clues_found"] * 30
bonus_points = 25 if attempt["bonus_clue"] else 0
overtime_minutes = max(0, attempt["elapsed_minutes"] - 45)
overtime_penalty = overtime_minutes * 2
score = max(0, clue_points + bonus_points - overtime_penalty)

The goal is not more locals. The next bonus/overtime change now has named locations and separately inspectable values.

Extract a cohesive function

After named stages are stable, extract scoring:

def score_attempt(attempt: Attempt) -> int:
    clue_points = attempt["clues_found"] * 30
    bonus_points = 25 if attempt["bonus_clue"] else 0
    overtime_minutes = max(0, attempt["elapsed_minutes"] - 45)
    overtime_penalty = overtime_minutes * 2
    return max(0, clue_points + bonus_points - overtime_penalty)

Keep validation either inside this function by contract or in a clear boundary function; do not accidentally remove it during extraction. Run focused scoring and invalid-input tests immediately.

Introduce a named constant when the name explains policy

BONUS_POINTS = 25
OVERTIME_START_MINUTES = 45
OVERTIME_POINTS_PER_MINUTE = 2

A constant is valuable when the name exposes why a number matters or several uses must stay synchronized. Replacing every 0, 1, and 2 with a constant can make ordinary operations harder to read.

4. Simplify decisions without changing their edges

Nested code can often become guard clauses:

def validate_attempt(attempt: Attempt) -> None:
    if not attempt["team"].strip():
        raise ValueError("team cannot be empty")
    if not 0 <= attempt["clues_found"] <= 8:
        raise ValueError("clues_found must be between 0 and 8")
    if attempt["elapsed_minutes"] < 0:
        raise ValueError("elapsed_minutes cannot be negative")

The order is observable when more than one field is invalid because the first exception wins. Preserve that order unless a behavior change is intended.

Replace duplication only after identifying what is truly shared

These branches repeat text shape:

def badge_message(position: int, team: str) -> str:
    if position == 1:
        return f"Gold badge for {team}"
    if position == 2:
        return f"Silver badge for {team}"
    return f"Blue badge for {team}"

Extract the changing value:

def badge_message(position: int, team: str) -> str:
    if position == 1:
        color = "Gold"
    elif position == 2:
        color = "Silver"
    else:
        color = "Blue"
    return f"{color} badge for {team}"

A lookup table can be clearer for stable data:

BADGE_BY_POSITION = {1: "Gold", 2: "Silver"}


def badge_message(position: int, team: str) -> str:
    color = BADGE_BY_POSITION.get(position, "Blue")
    return f"{color} badge for {team}"

Do not force conditions into data when the branches perform distinct behavior or need different validation. Compare the next likely change: adding a third special color is simple in the table; adding a branch with a notification side effect may deserve explicit control flow.

Shorter can be less clear

This one-liner combines validation, defaulting, selection, and formatting:

def label(record):
    return f"{record.get('team', '').strip() or 'UNKNOWN'}:{max(0, record.get('score', 0))}"

A few named statements give failures and decisions visible homes. Refactoring should reduce reasoning cost, not win a line-count contest.

Checkpoint: choose a useful move

5. Separate pure calculation from effects

A function that reads a file, calculates scores, prints, and writes output is hard to check without arranging all effects. Move boundaries outward:

from pathlib import Path


def build_report(attempts: list[Attempt]) -> str:
    ranked = rank_attempts(attempts)
    return render_leaderboard(ranked)


def write_report(input_path: Path, output_path: Path) -> None:
    attempts = load_attempts(input_path)
    report = build_report(attempts)
    output_path.write_text(report, encoding="utf-8")

Now build_report is a pure value transformation. write_report owns file interaction. This is a structural improvement only if paths, encoding, output, exceptions, and call behavior remain compatible with the existing contract.

Refactors can move side effects accidentally

Compare:

def announce_and_score(attempt: Attempt) -> int:
    print(f"Scoring {attempt['team']}")
    validate_attempt(attempt)
    return score_attempt(attempt)

Moving validation before printing changes output for invalid input. That may be a better design, but it is observable. Decide explicitly and test the intended order rather than calling it an invisible cleanup.

Public names are contracts; private names are freer

Renaming _calculate_bonus is usually internal. Renaming score_attempt breaks imports and callers unless the project updates them or provides a migration. This course is pre-release and can approve route/ID resets, but an application API still needs an intentional contract decision. A public rename is not a behavior-preserving local refactor merely because tests were edited at the same time.

6. Use the diff as the review artifact

After each move:

git diff --stat
git diff -- src/midnight_museum/quest.py
git diff --check

Ask:

  • Does the diff contain one explainable structural purpose?
  • Did a formatter rewrite unrelated files?
  • Did expected values change without a requirement change?
  • Did public names, exceptions, ordering, mutation, or output move?
  • Are new abstractions named from the domain rather than implementation trivia?
  • Can the old and new behavior be compared with the same tests?

Then run the gate:

ruff check src tests
ruff format --check src tests
mypy src
pytest -q

A small diff plus a green gate is strong evidence, not certainty. Explain what the tests and checks cover and what remains a judgment.

Know when to stop

An abstraction has a cost: another name, file, call, and navigation step. Stop when the present maintenance problem is solved and the code has a coherent shape. Do not add a strategy class, plugin registry, or generic framework for a single fixed bonus rule. Future requirements may arrive differently from the one you imagined.

Checkpoint: review and stopping

7. Lab: refactor the museum ranking pipeline without a rewrite

Use the complete build_board function from Section 1 and the existing green tests. Preserve:

  • public name build_board;
  • exact report text;
  • zero score floor;
  • validation exception types/messages/order;
  • descending score and descending team tie order;
  • no mutation of input; and
  • Python 3.10-compatible typed interfaces.

Perform these moves separately:

  1. add missing characterization checks for tie order and mutation;
  2. extract named score stages;
  3. extract validate_attempt without changing exception order;
  4. extract score_attempt and rerun only scoring/invalid tests;
  5. introduce only constants whose names expose policy;
  6. extract ranking and rendering boundaries;
  7. annotate the completed private/public boundaries honestly;
  8. run Ruff and review its formatting separately;
  9. inspect the full diff for an accidental behavior edit; and
  10. run the complete local gate.

Keep a change log:

Move Maintenance problem Focused check Diff observation
Add tie test Current order was implicit tie test passes No production edit
Extract scoring Bonus rule mixed with report scoring tests pass Public output unchanged
… … … …
Hint A: freeze unusual behavior first

Use the descending Blue then Amber tie example and a deep copy of attempts. Do not “improve” the tie policy during this structural lab.

Hint B: extract in two stages

First create named intermediate scoring values inside build_board. When the focused tests pass, move those same statements into score_attempt. This makes a failed extraction easier to compare.

Hint C: preserve tuple sorting exactly

ranked.sort(reverse=True) on (score, team) tuples implements descending order for both fields. Keep that operation if exact current behavior is the contract. A key using (-score, team) would change ties.

Show one complete behavior-preserving result 
from collections.abc import Sequence
from typing import TypedDict

CLUE_POINTS = 30
BONUS_POINTS = 25
OVERTIME_START_MINUTES = 45
OVERTIME_POINTS_PER_MINUTE = 2


class Attempt(TypedDict):
    team: str
    clues_found: int
    elapsed_minutes: int
    bonus_clue: bool


def validate_attempt(attempt: Attempt) -> None:
    if not attempt["team"].strip():
        raise ValueError("team cannot be empty")
    if not 0 <= attempt["clues_found"] <= 8:
        raise ValueError("clues_found must be between 0 and 8")


def score_attempt(attempt: Attempt) -> int:
    validate_attempt(attempt)
    clue_points = attempt["clues_found"] * CLUE_POINTS
    bonus_points = BONUS_POINTS if attempt["bonus_clue"] else 0
    overtime_minutes = max(
        0,
        attempt["elapsed_minutes"] - OVERTIME_START_MINUTES,
    )
    overtime_penalty = overtime_minutes * OVERTIME_POINTS_PER_MINUTE
    return max(0, clue_points + bonus_points - overtime_penalty)


def rank_attempts(attempts: Sequence[Attempt]) -> list[tuple[int, str]]:
    ranked = [
        (score_attempt(attempt), attempt["team"])
        for attempt in attempts
    ]
    ranked.sort(reverse=True)
    return ranked


def build_board(attempts: Sequence[Attempt]) -> str:
    return "\n".join(
        f"{team}: {score}" for score, team in rank_attempts(attempts)
    )

This solution intentionally preserves the original validation shown in Section 1: team and clue count. If the project contract also rejects negative elapsed minutes, add that as a separate behavior change with a failing example first. The constants and extra functions are justified by the planned bonus-rule change and independently checkable stages.

Key points

  • Refactoring changes structure without intentionally changing observable behavior; features and bug fixes need separate requirements and evidence.
  • Freeze important output, exceptions, ordering, mutation, and side effects before moving code.
  • Make one named move, run a focused check, inspect the diff, and return to the full gate at useful checkpoints.
  • Rename, extract values/functions, introduce policy constants, simplify branches, remove real duplication, and isolate effects only when they solve a stated maintenance problem.
  • Shorter and more abstract are not synonyms for clearer.
  • Stop when the problem is solved; every new layer has a reading and navigation cost.

References

Continue to the Unit Challenge

Back to top