My ticket started small: convert five Web API endpoints to our shared async pattern, as a low-risk batch to prove it worked. Code review came back with a fair question. Why stop at five, and leave the controller half on one pattern and half on another? So I finished the whole controller, and then took on the other straggler, which had 39 synchronous endpoints of its own.
All told, it came to more than 40 endpoints. The thing that made that reviewable wasn't care or stamina. It was noticing that the 39 fell into five shapes, and converting each shape the same way. This post is about refactoring legacy controllers that way: group the changes by shape first, convert each group mechanically, and let review check each shape once instead of 39 times.
This is Part 5 of Legacy .NET That Still Pays. It's more my own work than the earlier parts, which were about the framework we build on. The controller and base-class names are left out, and the examples are neutral.
Two stragglers on the wrong pattern
Our service exposes a Web API for the admin page, the tills and third-party systems. A colleague had already moved most of the controllers onto a shared base class, with one async pattern for running an action and turning its result into an HTTP response.
That pattern gives every endpoint the same behaviour. The action returns a result object, success or failure with a message. The base class maps that to the right status code, logs the call consistently, and handles exceptions in one place.
Two controllers had been left behind. They were still a mix of sync and async endpoints, each building its own responses in its own way.
I want to be honest about what "async" bought us here. Most of these endpoints do quick, synchronous work: validate a value, save a setting. Making them async doesn't make that work faster.
The value was consistency: one way to return results, one way to log, and one way to fail, across the whole API. This is the easy end of async, nothing like the ordering problem in Part 4, and that's exactly why it's a good candidate for doing mechanically.
The weak way: 39 conversions, 39 reviews
Here's a typical endpoint before, made generic:
[HttpPost]
public IHttpActionResult SetRetryCount(int value)
{
if (value < 0)
return BadRequest();
var error = _settings.SetRetryCount(value);
return error == null ? (IHttpActionResult)Ok() : BadRequest(error);
}And after:
[HttpPost]
public Task<IHttpActionResult> SetRetryCount(int value) =>
RunActionAsync(() => value < 0
? Result.Failure("Retry count must be zero or more")
: Result.FromError(_settings.SetRetryCount(value)));Converting one is easy. The weak way to convert 39 is to start at the top of the file and work down, each one slightly differently depending on how that particular method was written. You end up with a diff where every endpoint is its own little puzzle, and a reviewer has to solve 39 of them.
That's where mistakes hide. Not in the hard endpoints, which everyone reads carefully, but in the twenty-third easy one, where the reviewer's attention has gone and a guard got inverted.
The better way: name the shapes first
Before converting anything in the second controller, I read all 39 and sorted them. They fell into five groups:
| Shape | What it does | |---|---| | Plain setter | Calls a setter, maps the error string to a result | | Single numeric guard | Checks one number is in range, then sets | | Single string guard | Checks one string is present or valid, then sets | | Double guard | Two checks before the set | | Special cases | Three endpoints that didn't fit any group |
Once the groups had names, each conversion was mechanical. Every plain setter became the same one-liner. Every single-guard endpoint became the same conditional. The special cases got individual attention, because they needed it.
Review could then check each shape once. A reviewer reads one plain setter carefully, confirms the pattern, and then only has to check that each other plain setter really is one. That's a very different job from reading 39 bespoke changes, and it's one people can do well.
The shapes also made inconsistencies stand out. The double-guard methods had been written with if/else, while everything else used early returns. I turned them into early returns so every method in the controller read the same way. One of them needed its condition inverted to fit, which is exactly the kind of change that's safe when you're looking at the shape and risky when you're looking at one method in isolation.
The three that didn't fit
The special cases, and the odd ones inside the groups, are where the real review attention should go. One endpoint didn't match the method the plan had named for it, because the plan's signature was wrong, so I followed the pattern of its neighbour instead and said so in the PR. And one of the double-guard methods needed its condition inverted to fit the early-return style.
None of them was hard. All of them were the kind of thing that slips through when it's buried among thirty-odd routine changes.
Writing the shapes into the PR
The PR description did a lot of the work. It listed the five shapes, gave one example of each, and named the special cases with a line on why each one was different. A reviewer could read the example, agree with the shape, and then scan the rest of that group for anything that didn't match.
That's the difference between asking someone to review a diff and asking them to review a design. The design was five shapes. The diff was just the design applied.
What review added
Review pushed the scope out further, and most of it was right.
Guard failures had returned an empty failure. Review asked for descriptive messages, so a caller gets "retry count must be zero or more" instead of a bare Bad Request. That's more work per endpoint, and it's the kind of thing that's much easier to add per shape than per method.
A shared helper that passed a delegate around to build some response text got inlined. The indirection had made both of its callers harder to read, for the sake of saving a few lines.
A colleague asked me to promote an endpoint validation helper from another controller into the base class, so three endpoints could use it instead of their own inline address and port checks. While I was there, I fixed a bug flagged on an earlier PR. Called with both of its checks switched off, the helper used to return success silently, with a loopback address. Now it fails, and a small test class around the base controller covers the success and failure cases.
And the query endpoints needed typed results to serialise properly on .NET 8, which the project also targets. Each got its own result type rather than one shared type for everything.
Then the tests
The follow-up ticket asked me to bring every controller's tests into line with the new ones. It read like a tidy-up. When I looked, only 9 of the 21 controllers had a test file at all.
So it wasn't one job, it was two. Align the 9 existing suites with the new pattern, then write new suites for the dozen or so controllers that had none. I split it into two PRs on one branch, the second branching from the first, so the careful restructuring of existing tests could be reviewed on its own before a much bigger diff landed on top of it.
I also checked each untested controller's base class before planning its suite. The new test pattern only fits controllers that use the shared async helper. Where one didn't, the plan was an honest suite for its real shape, not a forced copy of the pattern.
The ticket also linked two earlier PRs as "outstanding points". I pulled them, and they were about something else entirely. The real source was a couple of review comments on an unrelated test file. I wrote that down as a deferred item with the reason, rather than let it quietly widen the scope.
The tests I'd write first next time
If I did this again, I'd write a thin layer of tests before converting anything. Not full suites, just one test per shape that pins the current behaviour: a valid value succeeds, an invalid one fails with the right status. Characterisation tests like that are cheap, and they'd have made the conversion provably behaviour-preserving instead of reviewably so.
Where it falls over
The tests came well behind the refactor. The 39 conversions went in on review and the shared pattern, not on new tests of their own. The controller's existing tests only covered methods that were already async. The follow-up closed that gap, but after the fact, and for a stretch most of the newly async endpoints had no test of their own. Grouping by shape makes review better; it doesn't replace tests.
Scope growing in review worked out here. It still made the PR much bigger than planned, and it happened halfway through. Next time I'd settle "finish the whole controller?" before starting, not after the first batch was already up for review.
Grouping has a risk of its own, too. A method that looks like a shape but isn't gets converted as one, and the reviewer, primed by the pattern, is more likely to wave it through. The special-cases list is the defence, and it only works if you're honest about what goes on it.
And the "small" test ticket turned into a coverage audit across 21 controllers. That was the right call, and it would have been easier to plan for if someone had looked before estimating. A test-alignment ticket deserves an honest audit before anyone puts a size on it.
The async conversion itself didn't make anything faster, and I wouldn't claim it did. It made the API consistent, which is worth a lot, but it's a different benefit, and it's worth being clear about which one you're buying.
Sort before you convert
My position: a large mechanical refactor stays safe when you group the changes by shape before you make any of them. Name the shapes, convert each one the same way, handle the special cases on their own, and review can check five patterns instead of forty methods.
Before your next big refactor, read everything you're about to change and sort it into groups. If a group has one member, it's a special case, and it deserves its own look. Write the shapes into the PR description, so the reviewer knows what to check. And if the tests are going to come later, say so in the PR, then make sure they do.
Next in the series: a validator that started life as a private helper in one service, and only moved into the shared library once it had proved itself. The rest of my writing is on the blog.