Refactor selectively

What you will be able to do

Change the structure of code only where the requested change needs it. Keep every other improvement for its own task. A focused change is easier to review, to test and to undo.

Before you startWhat to know and to have readyAbout 40 minutes

What you need to know

• What a refactor is: a change of the structure of code that does not change what it does. You did one in Function contracts in Python for Practical AI Engineering.
• How a test that fails first proves a fix, from Choose meaningful checks.
• How to read a diff, and the review checklist, from Inspect the diff.

What you need on your computer

• The project ~/projects/ticket-cleaner-assist at the end of Module 4, on the branch feature/category-option, with its environment active. python -m pytest prints 39 passed, and git status --short prints nothing.
• The labs folder ~/projects/nextia-learning-labs, with C15/transcripts/.
• If the project is missing or broken, follow the reset steps of Context and boundaries with the snapshot end-of-m04 in place of end-of-m01. They make the project at the end of Module 4, with main at the course’s start.
• No account, no key and no money. The assistant’s changes in this lesson come from recorded sessions.

A shelf, not a new kitchen

You ask a carpenter to put up a shelf in your kitchen. Behind the place for the shelf, a wall plug is loose. The carpenter fixes the plug first, because the shelf hangs on it. That is part of the job.

Now imagine that the carpenter also paints the cupboards and moves the cooker, “because it looked better”. The kitchen may look nicer. But you asked for a shelf. Now you must check the cupboards and the cooker too, and you cannot tell whether the shelf is safe until you do.

The loose plug supported the job. The paint did not. This lesson is about that difference, in code.

The problem: “make it cleaner”

In Context and boundaries, you met the recorded session refactor-open. The course team gave an assistant the files of ticket_cleaner/ and one sentence:

Prompt

Refactor the code in ticket_cleaner/ to make it cleaner and easier to maintain.

Before its edits, the assistant said what it would do:

What the assistant printed (part)

I’ll simplify record validation, use `Counter` for category totals, and make JSON writes safer with unique temporary files and cleanup. Public function names
and the report format will stay unchanged.

The change was 3 files, +40 −21, and all 24 tests passed before and after. Now look at each edit, and ask which task needs it.

The five edits of the real session refactor-open. None of them is wrong on its face, and no task asks for any of them. The line that had a real problem did not change.

Text description of the diagram

One row per edit, with what it changed and which task needs it:

• files.py, write_json(): rewritten with NamedTemporaryFile and try/finally. Every report goes through it. No task needs it. The assistant called it “safer”, but no test shows that.
• parsing.py, split_records(): the loop is rewritten with continue. No task needs it. It is in the same file as Grace’s bug.
• parsing.py, find_problems(): a type hint, raw: dict → raw: object. No task.
• report.py, count_by_category(): a loop → collections.Counter. No task.
• files.py, read_rows(): open(path) → path.open(), two times. Style only.
• Not in the diff: clean_category(). The line with Grace’s bug did not change, so her export still shows Billing, LOGIN and Login next to billing and login.

Each edit may be a small improvement. That is not the problem. The problem is that you cannot review them against anything. “Cleaner” has no acceptance criterion. No test fails before and passes after. So you cannot say when the work is finished, or whether it is right. You can only say that nothing you test has broken.

Two edits carry a real risk. write_json() saves every report that Grace reads, and it now works in a new way that no test checks. And the loop in split_records() decides which records are rejected. Its tests pass today, but a reviewer must now read 28 changed lines of parsing.py to be sure. Meanwhile, the code that Grace complained about stayed as it was.

In Inspect the diff, you saw the same pattern from the other side. The task brief plus “improve the code wherever you can” (candidate B) passed all 8 acceptance checks. It also added unrelated edits, and a test that fails in an 80-column terminal.

Tomás still likes the idea:

“While we are in report.py anyway, let the assistant clean up the whole package. It is cheap, and the tests pass.”

The tests pass because no test checks what changed. And it is not cheap. You pay for it in review time, in risk, and in conflicts, as the failure case below shows.

The idea: refactor only what the change needs

A refactor changes the structure of code and keeps its behaviour. You did one in the Python course: the output was the same before and after. A refactor can make a name clearer, split a long function, or remove duplication: the same rule written in two places.

To refactor selectively is to refactor only where it supports the requested change. Ask one question about each improvement that you want to make:

Does the requested change need it, to be correct, simpler, or safe to add?

• Yes: do it in this task. If it changes what the program does, write a test that fails first. Give it its own commit.
• No: write it down as a separate task. Do not change it now, even if it is small, and even if an assistant offers it.

Two questions decide what happens to an improvement. Only the requested change decides the first one.

Text description of the diagram

1. Does the requested change need it, to be correct, simpler, or safe to add? If no, write it down as a separate task. The edits of refactor-open go here.
2. If yes: does it change what the program does? If no, it is a pure refactor, and the existing tests must pass, unchanged. If yes, write a test that fails first, then make the change, and record the decision.

Either way, give it its own commit, and run the tests and ruff again.

Why is this worth the discipline? A reviewer can check a focused change against its task. A focused commit can be undone on its own. And a small diff leaves the rest of the code as your teammates know it.

A tiny example: two rules for one idea

Duplication is the most common reason for a refactor that a change needs. Here is one in ticket-cleaner, at the end of Module 4. Two places clean a category name, with two different rules:

• clean_category() in parsing.py cleans each ticket’s category. It removes the outer spaces, uses small letters, and changes the web form’s names to ours: Sign-in becomes login.
• build_summary() in report.py cleans the names that you give to --category. It uses only name.strip().lower().

The author ran both rules on four names:

Output (the author's run)

'Sign-in'    report.py: 'sign-in'  clean_category: 'login'
' LOGIN '    report.py: 'login'    clean_category: 'login'
'Invoice'    report.py: 'invoice'  clean_category: 'billing'
'biling'     report.py: 'biling'   clean_category: 'biling'

For " LOGIN " and biling, the two rules agree. For the web form’s names, they do not. The tickets say login, but the option looks for sign-in, and no ticket has that category.

Two rules for one idea give two answers for the web form's names. The refactor that the change needs is one rule in one place.

Text description of the diagram

What --category NAME selects in Grace’s export, with each rule:

--category	Module 4 rule (report.py)	clean_category()
Sign-in	'sign-in' → selected 0	'login' → selected 4
Invoice	'invoice' → selected 0	'billing' → selected 3
" LOGIN "	'login' → selected 4	'login' → selected 4
biling	'biling' → selected 0	'biling' → selected 0

The selective refactor: report.py calls clean_category(). One rule, in one place.

Larkfield: Grace types the form’s name

Grace tries the new option on her export. She thinks in the web form’s names, because her team sees them every day. So she types Sign-in:

1. Run Grace’s command in your project:

   python -m ticket_cleaner data/web-form-export.csv --category Sign-in

   Output

   12 rows: 12 valid, 0 rejected.
   Report: reports/summary.json

   The exit code is 0, so nothing looks wrong. Open reports/summary.json:

   reports/summary.json (part)

     "selected": 0,
     "by_category": {},
     "average_priority": null,
     "rejected": [],
     "categories": [
       "sign-in"
     ]

   The same command with --category login selects 4 tickets. Three of them came from the form as Sign-in, Login and LOGIN.

2. Find every place that cleans a name. grep searches files for a text. On Windows, use Select-String (not tested):

   macOS and Linux

   grep -n "strip().lower()" ticket_cleaner/*.py

   Windows

   Select-String -Pattern "strip().lower()" -SimpleMatch ticket_cleaner\*.py

   Output

   ticket_cleaner/parsing.py:15:    return str(value).strip().lower()
   ticket_cleaner/report.py:38:        chosen = sorted({name.strip().lower() for name in categories})

   Line 15 is inside clean_text(), which clean_category() calls. Line 38 is the second rule.

3. Write the test first. Add it at the end of tests/test_report.py. The tickets fixture has one open login ticket:

   tests/test_report.py (add at the end)

   def test_web_form_names_work_in_the_category_option(tickets):
       summary = build_summary(tickets, [], "open", categories=["Sign-in"])
       assert summary["by_category"] == {"login": 1}
       assert summary["categories"] == ["login"]

4. Run the tests. The new test must fail, for the reason that you expect:

   python -m pytest

   Output (part)

   E       AssertionError: assert {} == {'login': 1}
   E
   E         Right contains 1 more item:
   E         {'login': 1}
   FAILED tests/test_report.py::test_web_form_names_work_in_the_category_option
   ========================= 1 failed, 39 passed in 0.05s =========================

5. Make the refactor in ticket_cleaner/report.py. Add one import at the top, and change one line in build_summary():

   ticket_cleaner/report.py

   from ticket_cleaner.parsing import clean_category
   from ticket_cleaner.records import Rejected, Ticket

   ticket_cleaner/report.py, in build_summary()

   chosen = sorted({name.strip().lower() for name in categories})
   chosen = sorted({clean_category(name) for name in categories})

   Update the docstring too, so that it says which rule is used. The result is in the snapshot end-of-m05, if you want to compare:

   ticket_cleaner/report.py, the docstring of build_summary()

       """Return the report as a dictionary that can be saved as JSON.
   
       With categories, only the tickets in one of them are selected. The names are
       cleaned as the tickets' categories are (clean_category). The "categories"
       key is added only then, so the report without it is unchanged.
       """

6. Run the checks:

   python -m pytest
   ruff check .
   ruff format --check .

   Output (the end of each command)

   ============================== 40 passed in 0.04s ==============================
   All checks passed!
   21 files already formatted

   The 39 old tests still pass. The baseline test still passes too, so the report without --category has not changed.

7. Run Grace’s command again, and open the report:

   python -m ticket_cleaner data/web-form-export.csv --category Sign-in

   The report now has "selected": 4, "by_category": {"login": 4} and "categories": ["login"]. With --category " LOGIN " --category Invoice, it has {"billing": 3, "login": 4}.

8. Check the scope, then commit:

   git diff --stat
   git add -A
   git commit -m "Clean the --category names with the same rule as the tickets"

   Output

    tests/test_report.py     | 6 ++++++
    ticket_cleaner/report.py | 8 +++++---
    2 files changed, 11 insertions(+), 3 deletions(-)
   [feature/category-option 23d515b] Clean the --category names with the same rule as the tickets
    2 files changed, 11 insertions(+), 3 deletions(-)

   Your commit hash is different.

Two files, eleven lines added. Compare that with +40 −21 in three files for “make it cleaner”, which left Grace’s problem where it was.

Be honest about one thing. Strictly, this change is more than a refactor, because one behaviour changes: --category Sign-in now finds tickets. That is why it got a test that failed first. In the next lesson, it also gets a line in DECISIONS.md. A “refactor” that changes behaviour without a test and a record is a hidden feature.

What Amira did not change

While she was in report.py, Amira saw other things that she could improve. She wrote each one down, and she changed none of them:

• count_by_category() could use collections.Counter, as refactor-open did. The task does not need it.
• write_json() could use a unique temporary file. It might be safer, but it changes how every report is saved. It needs its own task, its own tests, and a reviewer who knows why it matters.
• filter_by_status() compares the status exactly. --status accepts only open, pending and closed, so the task does not need a cleaning rule there.

Each line goes to the team’s task list, with a sentence about why. If the team agrees, each one becomes a small task with its own review. Nothing is lost, and nothing is mixed into Grace’s change.

When to refactor, and what it costs

Refactor in a task when the change is wrong or hard to add without it. Duplication that makes two answers for one question, as here, is the clearest case. So is a function that you must change but cannot understand. Rename or split it first, in its own commit. The tests must pass before and after.

Do not refactor in a task because the code is old, because you would write it differently, or because an assistant offers it. A request such as “improve wherever you can” turns the assistant into the decision maker. With an assistant, extra edits feel free, because you did not type them. They are not free. Each one costs review time, and each one can conflict with a colleague’s work.

Make a refactor its own task when it is large, or when it touches code that many people use, such as write_json(). Then it gets its own brief: the behaviour that must not change, and the tests that show it.

A selective refactor has a cost too. The code stays a little less tidy than it could be, and the list of separate tasks grows. That is a fair price for a change that a reviewer can check in a few minutes.

Common mistake

git apply says that a patch does not apply

Symptom: you keep an assistant’s large refactor for later, as a patch file. Weeks later, you try to apply it to your project:

git apply --check ../nextia-learning-labs/C15/transcripts/refactor-open.patch

Output

error: patch failed: ticket_cleaner/parsing.py:21
error: ticket_cleaner/parsing.py: patch does not apply
error: patch failed: ticket_cleaner/report.py:1
error: ticket_cleaner/report.py: patch does not apply

Cause: the patch was made on the code of start. Since then, Module 4 changed clean_category() in parsing.py, near line 21, and this lesson added an import at the top of report.py. A patch finds its place by the lines around each change. Those lines are different now, so Git refuses. --check only tests the patch, and it changes no file.

Diagnose: git apply --stat shows what the patch touches, without applying it:

Output

 ticket_cleaner/files.py   |   27 ++++++++++++++++++++-------
 ticket_cleaner/parsing.py |   28 +++++++++++++++++-----------
 ticket_cleaner/report.py  |    6 +++---
 3 files changed, 40 insertions(+), 21 deletions(-)

Fix: do not force it. A broad refactor touches many lines, so it conflicts with almost any other change. Take only what a task needs, and ask again with the current files and a bounded task. This is the hidden cost of a large refactor: on a team, other people change the same files while you wait.

Your turn: sort four suggestions

Amira gives an assistant the Sign-in task. Imagine that it returns four suggestions. For each one, decide: do it in this task, or write it down as a separate task? Use the two questions of the diagram, and give a reason.

1. In report.py, call clean_category(name) for the names of --category.
2. Rename chosen to wanted_categories in build_summary().
3. Move CATEGORY_ALIASES from records.py into parsing.py, “because only parsing.py uses it”.
4. Add --category support to the remote input.

Then predict, before you run it: what does --category Delivery select in Grace’s export now? Run it and check.

Show the answer

1. Do it in this task. The change needs it: without it, --category Sign-in selects nothing. It changes behaviour, so it needs a test that fails first. That is the change that you made.
2. Separate task, or not at all. A clearer name may help, but the task works without it. It adds lines to the diff that a reviewer must check.
3. Separate task. The claim is true: only parsing.py uses the list. But the task works without the move. The README points to ticket_cleaner/records.py for the list, and the web form’s team edits it there. A move would change their habit and the README too.
4. Separate task, and not needed. --category already works with remote: cli.py gives the option to build_summary() after the records are read, wherever they come from. The suggestion is a guess about code that the assistant did not read.

--category Delivery: Delivery is one of the web form’s names, and clean_category() changes it to shipping. So the report has "selected": 2, "by_category": {"shipping": 2} and "categories": ["shipping"]. With the Module 4 rule, the same command selected 0. The author ran both versions and got these results.

Knowledge check

Check your understanding

The session refactor-open changed 3 files (+40 −21), and all 24 tests passed. Why is that not enough to accept it?

Show the answer

The tests show only that nothing that they check has broken. The request “make it cleaner” had no acceptance criterion, so no test can show that the work is done or right. Some edits change code that matters, such as write_json(), which saves every report, and no new test checks them. The assistant’s word “safer” is a claim without evidence. And the real problem, Grace’s bug in clean_category(), was still there. Each edit needs a task that asks for it, and evidence for it.

Check your understanding

Predict: you replace clean_category(name) in report.py with name.strip().lower() again. Which tests fail, and what does that tell you?

Show the answer

Only the new test fails: test_web_form_names_work_in_the_category_option. The author ran it at the end of the module, with 42 tests: 1 failed, 41 passed. The other tests use names that both rules clean in the same way, such as " Billing ". So that one test is the only guard of the refactor. That is why you wrote it first, and why you must not delete it as “redundant”.

Check your understanding

Tomás asks: 'This change makes Sign-in work. So it is a feature, not a refactor. Did we break the rule that a refactor does not change behaviour?'

Show the answer

He is right that the behaviour of --category changes for the web form’s names. So it is treated as a change of behaviour. It gets a test that fails first, its own commit with a clear message, and a decision in DECISIONS.md. The refactor part is the means: one cleaning rule in one place instead of two. Call it what it is in the commit and the pull request, so that nobody is surprised.

Recap

• A refactor changes structure and keeps behaviour. Refactor selectively: only where the requested change needs it.
• “Make it cleaner” (real, refactor-open) gave five edits that no task asked for, +40 −21, all tests green, and Grace’s bug untouched.
• Ask two questions: does the change need it? Does it change behaviour? If it changes behaviour, write a test that fails first.
• Duplication was the real reason here: two cleaning rules gave --category Sign-in 0 tickets. One rule, clean_category(), gives 4.
• Write every other improvement down as a separate task. A broad refactor costs review time, and it conflicts with other work.
• Your project: 40 passed, and one new commit.

Next, you write down what you decided, and why, so that the next person does not need to guess.

Next: Document decisions