The Professor
Index Course index

Module 5 · Architecture and maintainable code

Lesson 28 — Refactoring without breaking anything

Test-backed refactoring: changing the inside without moving the contract.

Published
In this lesson
  1. Exercise 1 — The net audit
  2. Exercise 2 — Guided extraction
  3. Exercise 3 — The forbidden experiment
  4. Exercise 4 — Contract guard
  5. Exercise 5 — The job extraction
  6. Professor's summary

Exercise 1 — The net audit

Expected answer (with the course's real gaps so far): unit tests of the state machine and resumen() (24/25), integration of the locked transaction (10), contract of the 409's problem+json (26) — and the classic gap: nobody tests that the outbox is written AFTER the reservation's commit. The test pinning that order:

python
def test_outbox_solo_si_commit(self):
    with patch("core.db.transaction.atomic", side_effect=IntegrityError):
        with self.assertRaises(DomainError):
            reservar(self.user, self.event, ["A1"], clock=self.clock)
    self.assertFalse(OutboxEvent.objects.exists())   # the rollback took the event with it

If anyone publishes before the commit, this red test says so in 2 seconds.

Exercise 2 — Guided extraction

The expected log (the hashes are yours; what matters is the granularity):

a1b2c3d test: characterization of the outbox after commit (red→green, no code touched)
e4f5a6b refactor: extract _validar_asientos() (suite green, 0 tests touched)
b7c8d9e refactor: extract _cobrar() behind the PaymentGateway Protocol (0 tests touched)
c9d0e1f refactor: ReservacionService absorbs transaction+outbox (0 tests touched)
d1e2f3a refactor: deprecate the loose function, delegate to the service (1 new test: deprecation)

The exercise's metric: 0 characterization tests touched across the 4 steps (if you touched one, the step was two). Step 4: the old function dies when the grep of its call sites returns empty ONE sprint later; the deprecation test (assertWarns(DeprecationWarning)) is the living reminder.

Exercise 3 — The forbidden experiment

Expected red: the contract tests (26) pinning the response's shape (the new field), the rounding test if it existed (if it didn't, the bug in passing goes UNNOTICED: that is the lesson — the mix slips in when the net has holes, not because the refactor is bad), and possibly the full-purchase E2E (35). Concrete number: 3-5 reds for one "improvement in passing".

The well-made version:

commit 1: rounding regression test (red) → minimal fix (green)      [bug]
commit 2: new field's test (red) → field + version note (14)        [feature]
commits 3-6: the pure refactor (0 tests touched)                    [refactor]

Three intentions, three commits, each revertible without dragging the others. Git's bisect (06) will thank you the day something breaks in production.

Exercise 4 — Contract guard

python
class ContractGuardTest(TestCase):
    def test_shape_de_reserva(self):
        res = self.client.get(f"/api/v1/reservations/{self.ref}")
        body = res.json()
        self.assertCountEqual(
            body.keys(),
            {"public_ref", "event", "seats", "total", "expires_at", "expires_in_seconds", "status"},
        )
        self.assertEqual(res.status_code, 200)

    def test_409_es_problem(self):
        res = self.client.post("/api/v1/reservations", {...})   # seat taken
        assert_problem(self, res)
        self.assertEqual(res.json()["type"], BASE + "seat-unavailable")

Renaming seat_refs → seats makes test_shape_de_reserva fail in <2s (fast suite) with a diff saying exactly which field went missing. The guard's value: it turns "changing the contract" into something that demands a conscious commit (the test gets edited on purpose), not an accident.

Exercise 5 — The job extraction

python
# services/reservations.py
def expirar_reservas(clock: Clock) -> list[str]:
    vencidas = list(Reservation.objects.select_for_update(skip_locked=True).filter(
        status=Reservation.Status.ACTIVE, expires_at__lt=clock.now()
    ))
    refs = []
    for r in vencidas:
        r.expire()                      # state-machine transition (00b/10)
        outbox.publish(ReservationExpired(ref=r.public_ref, user_id=r.user_id))
        refs.append(r.public_ref)
    return refs

# management command: TRANSPORT only
class Command(BaseCommand):
    def handle(self, *args, **opts):
        refs = expirar_reservas(SystemClock())
        self.stdout.write(f"{len(refs)} reservas expiradas")

The test with FakeClock and skip_locked (10): if two workers run it at once, they don't step on each other — 29's test will add the Celery worker as a new transport without touching the logic. The trigger's mock verifies the delegation: patch("reservations.management.commands.expirar_reservas.expirar_reservas") and assert of a single call + exception propagation (29's worker needs to see the failure to retry).

Professor's summary

  • Refactor = the inside changes, the observable doesn't; your suite defines the observable, and if it has holes, characterization plugs them before anything moves.
  • Atomic commits by intention: bug, feature and refactor never travel together; bisect and revert demand it.
  • Extract the logic from the mechanism first: the service stays stable and the transport (cron → Celery → whatever comes) is a boundary detail.

^ top