> But do you really want to block someone’s change because they wrote some awkward, hacky code? After all, they’ve solved some problem for the business and it might only take an hour to clean up later.
Isn't this precisely the time to do it? You reject with a note that says the idea is good, but code needs to be improved. "Doesn't fit the style" type of response. Make the contributor make the update so that it doesn't need an hour later. Take the hour now.
In my experience these hacky changes are usually a consequence of some impedance mismatch in the codebase. Something is poorly factored, the interface (whether technical or personal) isn't suitable for purpose.
It's easy to push back on a hacky change if there's an elegant solution close at hand. But often the business needs and the architecture of the codebase are at odds with each other.
It might take me an hour, but communicating the idea to the engineer, convincing them it’s better, guiding them to the solution, and then having them build it will take far longer wall clock time.
And devs that have this line of thinking is precisely why there's a back log when it can only be done by one person. Instead, create style guides or other structured documentation that can be referenced in the rejection so you don't have to do it would be time much better spent. It helps contributors to be better contributors. It makes contributors want to continue contributing. Having every submit rejected gets to the point of not wanting to contribute.
The biggest cause of these issues isn’t style-related or something that can be easily documented ahead of time.
A recent example: an engineer wrote a custom caching layer for a service call, then called the service wrapper every time the data was needed, relying on caching to avoid hitting our dependency too often. I suggested fetching the data once and passing it around, but the engineer pushed back, citing the feature’s launch deadline and the effort required to update multiple interfaces. Their solution ultimately has more failure modes and is harder to test (requiring mocking of the service wrapper in several places), but it isn’t terrible and we probably won’t encounter cache overflow issues.
Another persistent example is function parameter length. Inevitably, there’s always “just one more argument” needed until we hit our configured linter limit. About 70% of the time, engineers add a suppression and push the change through as-is. Refactoring to reduce parameters can require significant work (and simply stuffing them into a parameter object doesn’t solve the underlying problem).
I could respond to these patterns by expanding our coding standards guide. It could document that caching shouldn’t be relied upon within a single request’s scope, or reinforce that functions should have fewer than 7 parameters (already enforced by our linter). But in my experience, these guidelines are rarely consulted before contributing. As the parameter example shows, people often push for exceptions rather than follow standards rigidly.
I do think standards guides can work well for open source projects, where contributions should never block someone from delivering value. Contributors always have the option to fork and use their changes as-is, which undermines arguments for “just getting something in.” Internal service codebases don’t have that luxury. When you’re changing a service to launch your feature before a major sales event, delays have real costs, and there’s no “I’ll fork this and maintain it myself” alternative.
Isn't this precisely the time to do it? You reject with a note that says the idea is good, but code needs to be improved. "Doesn't fit the style" type of response. Make the contributor make the update so that it doesn't need an hour later. Take the hour now.