A few months ago, I wrote about testing legacy Laravel code without refactoring first. We took an ugly controller method, didn’t touch a single line, and wrapped it in integration tests.
That article ended with a promise: once the tests exist, you can finally start cleaning up.
So let’s do it. Same controller, same tests. No new architecture, no design patterns, no big rewrite. Just small moves, one at a time, with green tests after every single one.
Previous article in this category: https://codecraftdiary.com/2026/08/30/refactoring-the-database-layer-replacing-query-scope-bloat-with-repository-query-objects/
The code we’re starting with
Here’s the method again, exactly as it was:
public function processOrder(Request $request)
{
$order = new Order();
$order->product_id = $request->product_id;
$order->quantity = $request->quantity;
$order->user_id = auth()->id();
$order->status = 'pending';
$order->save();
// business rules mixed directly inside controller
if ($order->quantity > 10) {
$order->priority = 'high';
}
if ($order->product_id === 999) {
// fetch external pricing
$response = Http::post('https://external-api.com/prices', [
'product_id' => $order->product_id,
]);
if ($response->ok()) {
$order->external_price = $response->json('price');
}
}
// logging inside business logic
Log::info('Order processed', [
'order_id' => $order->id,
'user' => auth()->id(),
]);
// send email
Mail::to(auth()->user())->send(new OrderCreatedMail($order));
// update stock
$product = Product::find($order->product_id);
$product->stock -= $order->quantity;
$product->save();
return response()->json([
'id' => $order->id,
'status' => $order->status,
'priority' => $order->priority ?? 'normal',
], 201);
}It works. Nobody wants to touch it. That’s the definition of legacy code in most teams.
The only rule: small steps, green tests
Before we start, one rule that matters more than any technique in this article:
Change one thing. Run the tests. Commit. Repeat.
If a step breaks something, you know exactly which step it was, and git checkout . costs you two minutes instead of an afternoon. It feels slow. In practice, it’s the fastest way to refactor code you don’t fully understand.
Step 0: Pin down the behavior you’re about to touch
Our existing tests cover the happy path and the external price. But the priority rule isn’t tested at all, and it’s exactly the kind of thing that breaks quietly during refactoring. So before changing anything, I add one more characterization test:
public function test_priority_depends_on_quantity()
{
Mail::fake();
Http::fake();
$this->actingAs(User::factory()->create());
$product = Product::factory()->create(['stock' => 50]);
$this->postJson('/api/orders/process', [
'product_id' => $product->id,
'quantity' => 10,
])->assertJson(['priority' => 'normal']);
$this->postJson('/api/orders/process', [
'product_id' => $product->id,
'quantity' => 11,
])->assertJson(['priority' => 'high']);
}Note the boundary. The rule is > 10, not >= 10, and “10 is normal” is exactly the kind of detail you’d accidentally change while “cleaning up”.
The test passes. Commit.
Step 1: Give magic values a name
10 and 999 mean something to someone. Probably to someone who left the company in 2021.
class OrderController extends Controller
{
private const HIGH_PRIORITY_QUANTITY = 10;
private const EXTERNALLY_PRICED_PRODUCT_ID = 999;
// ...
if ($order->quantity > self::HIGH_PRIORITY_QUANTITY) {
$order->priority = 'high';
}
if ($order->product_id === self::EXTERNALLY_PRICED_PRODUCT_ID) {
// ...
}
}This is the most boring refactoring there is, and one of the most useful. The next person reading this code no longer has to guess why product 999 is special. The name tells them.
Tests: green. Commit.
Step 2: Stop asking for the same thing three times
The method calls auth() three times. It’s not a performance problem, it’s a readability one: every call makes you wonder whether it could return something different.
$user = auth()->user();
$order->user_id = $user->id;
// ...
Log::info('Order processed', [
'order_id' => $order->id,
'user' => $user->id,
]);
Mail::to($user)->send(new OrderCreatedMail($order));One variable, one source of truth. Tests: green. Commit.
Step 3: Extract methods until the main method reads like a story
This is the step that changes how the code feels. The comments in the original method are a big hint: every // fetch external pricing or // update stock is a method that wants to exist.
public function processOrder(Request $request)
{
$user = auth()->user();
$order = $this->createPendingOrder($request, $user);
$this->applyPriority($order);
$this->applyExternalPrice($order);
Log::info('Order processed', [
'order_id' => $order->id,
'user' => $user->id,
]);
Mail::to($user)->send(new OrderCreatedMail($order));
$this->decreaseStock($order);
return response()->json([
'id' => $order->id,
'status' => $order->status,
'priority' => $order->priority ?? 'normal',
], 201);
}
private function createPendingOrder(Request $request, User $user): Order
{
$order = new Order();
$order->product_id = $request->product_id;
$order->quantity = $request->quantity;
$order->user_id = $user->id;
$order->status = 'pending';
$order->save();
return $order;
}
private function applyPriority(Order $order): void
{
if ($order->quantity > self::HIGH_PRIORITY_QUANTITY) {
$order->priority = 'high';
}
}
private function applyExternalPrice(Order $order): void
{
if ($order->product_id === self::EXTERNALLY_PRICED_PRODUCT_ID) {
$response = Http::post('https://external-api.com/prices', [
'product_id' => $order->product_id,
]);
if ($response->ok()) {
$order->external_price = $response->json('price');
}
}
}
private function decreaseStock(Order $order): void
{
$product = Product::find($order->product_id);
$product->stock -= $order->quantity;
$product->save();
}Two things worth noticing.
First, the comments are gone, and nothing is lost. The method names say the same thing, and unlike comments, method names don’t go out of date when someone changes the code.
Second, I kept the order of operations exactly the same. It’s tempting to “tidy up” the sequence while you’re in there. Don’t. The order of side effects is behavior, and we’re not changing behavior yet.
Tests: green. Commit. (Honestly, this could be three commits, one per extracted method. Smaller is better.)
Step 4: Flatten with guard clauses
applyExternalPrice has two levels of nesting. Guard clauses turn “if this, then if that, then do the thing” into “if not this, leave”:
private function applyExternalPrice(Order $order): void
{
if ($order->product_id !== self::EXTERNALLY_PRICED_PRODUCT_ID) {
return;
}
$priceResponse = Http::post('https://external-api.com/prices', [
'product_id' => $order->product_id,
]);
if (! $priceResponse->ok()) {
return;
}
$order->external_price = $priceResponse->json('price');
}I also snuck in a rename: $response became $priceResponse. In a controller that also returns an HTTP response, a variable called $response holding the response of a different HTTP call is a small trap. Renaming costs nothing and removes it.
Tests: green. Commit.
Step 5: The bug you’ll find along the way
Now look at the main method again, the way it reads after Step 3:
$order = $this->createPendingOrder($request, $user); // save() happens here
$this->applyPriority($order);
$this->applyExternalPrice($order);In the original 40-line method, this was easy to miss. Now it jumps out: the order is saved before priority and external price are set, and it’s never saved again.
That means priority and external_price never reach the database. The API response still says "priority": "high", because it reads the value from memory, not from the database. So the client is told one thing, and the database stores another.
This is the part nobody tells you about refactoring: making code readable is also how you find the bugs that were hiding in it.
And here’s the rule that matters: don’t fix it in the same commit. Everything up to now was a pure refactoring, with no behavior change. Fixing the bug is a behavior change. Keep them separate, so that if something downstream turns out to depend on the old behavior (it happens more often than you’d think), you can revert just the fix.
Before fixing, it’s worth a quick check with whoever owns this feature. Is there a report that treats null priority as normal? An admin screen that shows the priority? Five minutes of asking beats a surprised product manager.
Then the fix goes the proper way: red test first.
public function test_large_orders_are_stored_with_high_priority()
{
Mail::fake();
Http::fake();
$this->actingAs(User::factory()->create());
$product = Product::factory()->create(['stock' => 50]);
$this->postJson('/api/orders/process', [
'product_id' => $product->id,
'quantity' => 11,
]);
$this->assertDatabaseHas('orders', [
'product_id' => $product->id,
'priority' => 'high',
]);
}It fails. Good, that proves the bug is real. Now the fix, which after our refactoring is almost trivial: build the order first, save it once at the end.
$order = $this->buildPendingOrder($request, $user); // no save() inside anymore
$this->applyPriority($order);
$this->applyExternalPrice($order);
$order->save();Test: green. Commit, with a message that says what changed and why: Fix: persist order priority and external price.
Try making this fix in the original method. It’s doable, but you’d be moving a save() call around inside a wall of mixed concerns and hoping nothing else depended on it. After the refactoring, the fix is three lines and obviously correct.
Where to stop
The controller is still doing too much. It’s creating orders, calling an external API, sending email and managing stock. Stock updates aren’t atomic, and Product::find() can return null.
All true. None of it needs to be fixed today.
This is the point where you could move the logic into an action class (the fat controller article walks through that), or wrap the stock update in a transaction. But each of those is its own change, with its own tests and its own commit. Refactoring doesn’t have to end with a pattern. Sometimes the right move is to leave the code clearly better than you found it and go ship the feature you actually came here for.
Before and after
We started with one 40-line method full of comments explaining what it does. In the end, the main method reads top to bottom in ten seconds, and four small private methods with honest names do the actual work. In addition, the magic numbers are gone, a real bug is fixed, and the test suite grew from two tests to four. Above all, we didn’t need a single new class, interface or pattern.
A checklist for your next legacy refactoring
- Make sure the behavior you’re touching is covered by tests. If it isn’t, write characterization tests first.
- Replace magic numbers and strings with named constants.
- Pull repeated calls into a single variable.
- Turn comments into method names by extracting methods.
- Flatten nested conditions with guard clauses.
- Rename anything that could mislead the next reader.
- When you find a bug, write it down, finish the refactoring, then fix it in a separate commit, test first.
- Run the tests after every step. Commit after every step.
- Stop when the code is clearly better, not when it’s perfect.
Legacy code rarely gets fixed in one heroic pull request. It gets fixed like this: one boring, safe, green step at a time.