A service-layer refactor needs six checks before you approve it: confirm the stated reason, trace every call site, verify that transactions and error handling landed somewhere explicit, assess coupling, confirm tests followed the logic, and leave a verdict that names what you covered. Done carefully, it takes thirty to sixty minutes depending on the size of the diff.
This situation comes up constantly. A teammate extracts a fat controller action into a service class, or pulls logic back out because it grew too abstract. The PR looks tidy on the surface, but the most common failures are invisible in a quick scan: a caller that wasn't updated, a transaction boundary that quietly split, or tests that stayed in the wrong file and now pass for the wrong reasons.
The steps below apply to any stack. Where examples are language-specific, they're labelled.
1. Confirm the reason for the move is stated explicitly
If the PR description doesn't name the motivation, ask before reviewing anything else. A service-layer refactor can mean at least three different things: the author is separating concerns, fixing a testability problem, or preparing logic for reuse across controllers. Each has different review criteria, so you can't judge whether the move was done well without knowing what "well" means for this specific change.
Check the PR title and description first. If neither names the motivation, leave a comment requesting it. One sentence is enough: "What's the primary reason for moving this, and is it intended for reuse elsewhere?"
When this step is done: the PR description, or a follow-up comment from the author, states a clear reason. You have enough context to hold the rest of the review against something concrete.
2. Trace every call site that touched the moved logic
Every place that called the old location needs to call the new one correctly, or the refactor is silently broken.
2a. Find all callers before reading the new service
Read the diff with call sites in mind first, not the service itself. Search the codebase for the old method name, the old class, or the old import path. If the PR shows the new file and one updated controller but nothing else, ask whether other callers exist. In larger Rails or Django codebases, a method name is often called from mailers, background jobs, and admin panels that aren't obvious from the diff view.
When this step is done: you have a complete list of callers, confirmed against a codebase search, not just the files changed in the PR.
2b. Check that each caller passes the right arguments
Compare the argument shape each caller passes against the new service method's signature. Type mismatches and missing parameters are common here, especially when the author extracted the method from one specific controller context and didn't generalise the inputs. In typed languages like TypeScript or Java, the compiler will catch some of this. In Python, Ruby, or plain JavaScript, it won't.
When this step is done: each call site's arguments match the new signature, including optional parameters and defaults.
3. Check where transactions, error handling, and side effects now live
Moving logic to a service layer often displaces the code that manages state, errors, and external calls. Those pieces need to land somewhere explicit.
3a. Verify transaction boundaries didn't split across layers
If database writes that used to happen together inside a controller action now span a controller and a service, the transaction boundary may have broken. Check that commits and rollbacks still wrap the complete unit of work. In ActiveRecord this means a single transaction block containing all writes. In Spring Boot it means @Transactional sits on the right method. A split transaction is one of the harder bugs to catch in production because it only surfaces under failure conditions.
3b. Check that error handling is owned by exactly one layer
Look for catch blocks that appear in both the controller and the service for the same exception type. Double-handling either swallows errors silently or produces duplicate log entries. One layer should catch and decide; the other should let exceptions propagate. If the service catches an exception and returns a null or a boolean, the controller often can't tell a failure from a legitimate empty result.
For PRs where logging behaviour changed as a result of the move, reviewing the observability impact is worth a separate pass.
3c. Confirm side effects aren't duplicated
If the controller fired a welcome email before the refactor and the service now also fires one, users get two emails. Grep for notification calls, queue dispatches, and cache writes around the moved code on both sides of the diff. This is especially easy to miss when the service was created by copying the controller action and trimming it down.
When this step is done: you can point to exactly one place where each transaction, each exception type, and each side effect is owned.
4. Assess whether the service is over-coupled or too thin
A service that takes a request object as its argument signals the move went too far toward the framework. A service that does a single line of work signals it didn't go far enough.
Over-coupling shows up when the new service imports from the HTTP layer: request objects, session helpers, or response formatters. A service should take plain data. Flag any import from a routing or request namespace inside the service file. In Laravel this might be Illuminate\Http\Request inside a service class. In Express it might be req and res passed directly.
A service that's too thin, say a method that only calls one repository method and returns the result unchanged, adds indirection without adding value. Point this out as a design question, not a blocker. Either give the method a reason to exist or suggest removing it and calling the repository directly.
When this step is done: you've noted whether the service boundary makes sense as a long-term home for this logic, and left a comment if it doesn't.
5. Verify the tests moved with the logic, and cover the new boundary
Tests that stayed in the controller spec after logic moved to a service are testing the wrong layer. Any new service boundary also needs its own tests.
5a. Check that unit tests follow the logic, not the old location
If the PR moves a calculation into a service but leaves all its tests in a controller test file, those tests are now integration tests by accident. They'll pass, but they'll be slow and fragile because they depend on the full request stack to exercise what is now a standalone unit. Ask the author to move or rewrite them targeting the service directly.
5b. Confirm the new boundary has a test for the failure path
A service extracted from a controller usually gains new failure cases: what happens when a dependency is unavailable, when inputs are invalid at the service level, or when a downstream call fails. Check the diff for at least one test that covers a non-happy path introduced by the move. If every test in the new service file uses the same valid inputs and expects success, the failure cases are untested.
For PRs that introduced a new external dependency as part of the refactor, reviewing the integration points deserves its own checklist.
When this step is done: unit tests live in the service spec, and at least one test covers a failure path the new boundary introduced.
6. Leave a verdict that names what you checked, applying code review best practices
A review comment that says "looks good" after a service-layer refactor tells the author nothing about what you actually verified.
Name the things you confirmed: call sites, transaction boundaries, test coverage, coupling. If you approved, say what you checked. If you requested changes, link each request to one of the categories above so the author knows exactly what to fix. "I checked all callers against the new signature and found one in the admin module that still passes a raw request object" is more useful than "service shouldn't take request objects."
A specific approval also tells the next reviewer what was already covered. Over time, it builds your reputation as someone who reviews carefully rather than quickly. That reputation compounds: authors share context with you earlier, flag edge cases proactively, and write better PR descriptions because they know you'll read them.
For a broader view on which code review skills compound over a career, that's worth reading separately.
When this step is done: your review comment lists at least three things you checked by name, and any change requests point to a specific concern rather than a vague feeling.
Frequently asked questions
Does this checklist apply when logic moves out of a service, not into one?
Yes. The direction reverses but the risks are the same: callers change, transaction ownership moves, and tests may not follow. Step 1 matters more in that direction because moving logic out of a service is less common and the reason is often less obvious to reviewers scanning the diff.
What if the PR also deletes the old location entirely?
Deletion adds one more concern: verifying nothing relied on the old location that wasn't caught by the call-site search. Reviewing PRs that delete code covers that specific angle in more detail.
How do I review this kind of PR faster without skipping steps?
The bottleneck is usually step two, finding all callers. Running a project-wide search for the old method name before opening the diff viewer takes under a minute and answers the hardest question upfront. Steps three through six go faster once you know the full scope of the change.