Why Moving Code into a Service Doesn’t Improve the Design by Itself

A controller contains 400 lines of business logic.

Someone moves them into OrderService.

The controller now has six lines. The service has the same database writes, external calls, request dependencies, and failure paths as before.

The extraction may make the code easier to navigate. It may also be a useful first step.

But when I review the change, I want to know what became easier to reason about.

Moving code changes its location. Improving the design requires clearer responsibility, explicit dependencies, and ownership of what succeeds or fails together.

A smaller controller is evidence of extraction. I still need to inspect the operation behind it.

1. The service can remain coupled to the request

Consider this controller:

public function placeOrder(Request $request): Response
{
    return $this->orderService->place($request);
}

And its service:

public function place(Request $request): Response
{
    $customerId = $request->getSession()->get('customer_id');
    $items = $request->request->all('items');
 
    // Validate, calculate, persist, charge, notify...
 
    return new RedirectResponse('/orders');
}

The service still depends on HTTP input, session state, and response behavior.

Running the same operation from a command or worker requires reconstructing that environment or creating another implementation.

A useful improvement is to define the operation’s inputs explicitly.

public function placeOrder(Request $request): Response
{
    $input = $this->orderInput->fromRequest($request);
 
    $result = $this->placeOrder->execute(
       actor: $this->currentActor->requireAuthenticated(),
       input: $input,
    );
 
    return $this->redirectToRoute('order_show', [
        'id' => $result->orderId(),
    ]);
}

The application operation now receives an authenticated actor and validated input. It must still enforce authorization and business rules.

Symfony recommends keeping business logic out of controllers and using dependency injection. Those are useful foundations, but they do not define the business boundary for us.

For plain PHP, the same principle applies: parse $_POST at the entry point and pass the resulting input into the operation.

The examples here use illustrative application classes and omit surrounding setup.

2. Explicit dependencies can reveal an unclear responsibility

Replacing container lookups with constructor injection makes dependencies visible.

Sometimes the result is this:

public function __construct(
    private OrderRepository $orders,
    private InventoryService $inventory,
    private PricingService $pricing,
    private PaymentGateway $payments,
    private Mailer $mailer,
    private InvoiceGenerator $invoices,
    private ShipmentService $shipping,
    private AuditLogger $audit,
) {}

I do not reject this simply because the constructor is long.

An application operation can legitimately coordinate several collaborators.

I ask why each dependency is needed and which decisions this class owns.

Does it coordinate order placement? Or does it also contain discount algorithms, invoice formatting, shipment-provider rules, and email templates?

Those responsibilities change for different reasons.

I might keep the sequence in an application service while giving a pricing policy ownership of calculation:

$quote = $this->pricing->quote($customer, $basket);
 
$order = Order::place(
    orderId: $orderId,
    customerId: $customer->id(),
    lines: $quote->lines(),
    total: $quote->total(),
);

The order can protect its own invariants. The pricing policy can own pricing decisions. The application service can coordinate the use case.

I would not extract every method into a separate class. If understanding one operation requires following a dozen trivial forwarding methods, the extra structure may have made the work harder.

The question is whether each boundary gives a decision a clear owner.

3. Transaction ownership is often still hidden

This looks readable:

$order = $this->orders->create($input);
$this->inventory->reserve($order);
$this->discounts->recordRedemption($order);

But what does each method do to the database?

If they commit independently, a failure during discount redemption can leave an order and reservation behind.

That might be intentional. If so, the application needs explicit partial states and recovery.

If the business requires all three changes to succeed together, the transaction boundary belongs around that complete operation.

return $this->transactions->run(function () use ($input) {
    $order = $this->orders->create($input);
 
    $this->inventory->reserve($order);
    $this->discounts->recordRedemption($order);
 
    return $order->id();
});

This example assumes every database change participates in the same transaction. The runner must commit on success and roll back on failure; with an ORM, it must also handle flushing and failed entity-manager state correctly.

I check those assumptions rather than trusting the wrapper’s name.

With Doctrine, flush() normally executes queued writes within a transaction when no explicit transaction is already controlling the operation. A flush inside a larger explicit transaction is not automatically an independent commit.

There is another subtlety: a shared entity manager can contain pending changes outside the repository method currently being called. A method named saveCustomer() that invokes flush() may therefore write more than its name suggests.

I want transaction and flush ownership to be understandable from the application flow.

4. A database transaction cannot cover every dependency

A common next step is to put everything inside the transaction:

$this->transactions->run(function () use ($order): void {
    $this->orders->save($order);
    $this->payments->charge($order);
    $this->mailer->sendConfirmation($order);
});

The database can roll back its writes. It cannot roll back a successful remote charge or an email already sent.

The transaction also remains open while external systems respond.

Before changing this sequence, I ask what “order placed” means.

Does it mean payment completed? Does it mean the order was accepted and payment is pending? Can inventory remain reserved while payment is unresolved?

Those answers determine the design.

For a workflow that accepts pending payment, one option is to persist the order, the payment-attempt identity, and the processing intent together:

$this->transactions->run(function () use ($order, $attempt): void {
    $this->orders->saveAwaitingPayment($order);
    $this->paymentAttempts->save($attempt);
 
    $this->outbox->append(
        eventId: $attempt->processingEventId(),
        type: 'PaymentRequested',
        payload: ['attempt_id' => $attempt->id()],
    );
});

The outbox and business records must share the transaction. Subsequent processing needs duplicate handling and reconciliation.

This is one possible workflow, not a universal replacement for synchronous payment.

I would not introduce asynchronous processing merely to make the service look cleaner. Delayed completion changes what the application can promise to its users.

5. A service name can hide an ambiguous result

This signature leaves important questions unanswered:

public function process(Order $order): bool

What does true mean?

The order was saved? Payment succeeded? A message was published? Every notification was delivered?

What does false mean if some of those steps already happened?

I prefer results that reflect the operation’s actual contract.

$result = $placeOrder->execute($actor, $input);
 
return new JsonResponse([
    'order_id' => $result->orderId(),
    'status' => $result->status(),
]);

A typed result helps only if its states have precise meanings.

For example, awaiting_payment should correspond to durable state the system knows how to process or reconcile. It should not mean that an exception was caught and ignored.

I also distinguish expected business rejection from infrastructure failure. “This discount is not applicable” and “the database connection disappeared” require different handling.

The controller can translate those outcomes into HTTP responses without owning the business decision.

6. I test the boundary the refactor claims to create

If the change claims to isolate pricing, I expect pricing tests that do not require HTTP or persistence.

If it claims atomic persistence, I want an integration test that fails after an earlier write and verifies that no partial state committed.

For example:

$this->redemptionWriter->failOnNextWrite();
 
try {
    $this->placeOrder->execute($actor, $input);
    self::fail('Expected the operation to fail.');
} catch (RedemptionStorageFailure $e) {
    // Expected failure injected inside the transaction.
}
 
self::assertFalse(
    $this->database->orderExists($orderId),
);
 
self::assertFalse(
    $this->database->reservationExistsFor($orderId),
);

The database assertions must inspect persisted state, not the same in-memory entities used by the operation.

This test establishes atomicity for that failure path. It does not prove correctness under concurrent requests.

If the service also claims to prevent overselling or duplicate redemption, I want separate evidence for the database constraints, conditional writes, or locking that enforce those guarantees.

Mocking three repository calls and checking their order cannot establish transaction behavior.

What I Look for in the Refactor

Moving code into a service can be a valuable first step. I want the review to be honest about what that step achieves.

The next questions are:

Can I identify the operation’s inputs? Can I find the business decision without tracing HTTP state? Do I know who commits the database changes? Can I explain the outcome if an external call succeeds and the next line fails?

I do not need every service to be small. I need its responsibility and guarantees to be clear.

When those answers improve, the design has improved.

Otherwise, I would describe the change as an extraction—and use it as the starting point for the next decision.