A client’s team spent a full week adding a CSV export to their admin panel. Two engineers, clear requirements, maybe a day of actual work. The rest of the time went to understanding existing code well enough to change it safely. That’s what I call codebase drag: when the codebase makes every task take longer than it should. It doesn’t show up in any dashboard or sprint report.
I find most bad codebases exist because of a culture that isn’t focused on quality, and I’m not talking about bug counts or code coverage. Clean codebases stay clean by being proactive about keeping them clean. This should include meticulous peer reviews, establishing design patterns, enforcing best practices, and taking initiative to leave things better than you found them (we used to call that boy scouting).
If your teams PR comments only contain LGTM, and the average time spent reviewing them is 5 minutes, your team isn’t focused on quality. If a PR contains more files than an average person can keep in their mental context window, it won’t get the attention it needs to be properly reviewed. If there is no accountability to keep a clean codebase, you’ll end up with 2 hours of work taking 5 days to complete.
The signal-to-noise ratio of reviews is nearly zero in my experience. It’s for the least productive people on the team to argue about spaces or gotos or grind other ideological axes.
I find PRs really dumb things down, but not in a way that makes code more understandable. And it certainly doesn’t improve quality.
If your team is only focused on tabs/spaces or soapboxing during code reviews, you have bigger issues to take care of.
Show me a place where this isn’t the case. Because I’ve never seen it not be the case in 20+ years in the field.
You must work in dreadful places. I’ve seen it a few times, but most places have been productive.
It needs a good lead dev to set the culture though.
Whitespace change debates can be avoided by using rule sets in IDEs and agreeing standards within the team.
Good static code analysis tools in pipelines and IDEs handle most technical issues leaving reviewers to focus on design, maintainability, clarity and readability.
You can avoid pickiness if you communicate why, so they learn and understand. If you use PRs as a training and learning tool they’re quite productive. If not sure, ask why something was done.
And if you get picky comments respond with “personal preference and not part of team rules”. But also, you cannot be defensive in your PRS. You have to be open to feedback and points and happy to discuss. Be polite even when feedback is invalid. Defendivesness kills constructive feedback and no matter how old you are and how long you’ve been doing it, you can still improve. Oh and if you been doing it that long, you’re a senior or lead and can influence how things are done.
15+ years in engineering here. 10+ in leadership.
Code formatting hasn’t been an issue since the early '10s. Tabs or spaces? Who cares. Your editor can make it look like whatever you want and it won’t effect the code.
As for other asshole-ish behavior or gatekeeping, I open it up to a vote. Let the team determine best practices. Don’t like what your team decides? Find another team to shitlord over.
My last job was pretty good about code reviews, when people actually spent time on them. My front end code got much better when the front-end expert actually reviewed it.
My current job, code reviews are a rubber stamp farce and I’ve seen total garbage sail though. The code base is a tire fire. These things are related.
A good alternative is code presentations.
You present your changes to a group of engineers. Then discuss it.
Yes, it happens too often. That’s a failure of leadership or a social problem.
Techies often try and fix human and social issues with technology, but that doesn’t always work.
Code review helps spread knowledge about the code base through the team. Without it, you easily end up with disjointed fiefdoms ruled by petty code lords that don’t share information.
Spreading knowledge and context sharing are exactly why I like code reviews. It should also be something done by more than one person so that information is better disseminated throughout the team.
Code presentations are great for that.
One or two people present their code before the merge. Others watch, ask questions, etc. Small changes and improvements can be done immediately. Ideally the change is merged after the presentation. It can speed up things immensely and more people feel ownership. If a simple ticket stays in review for a week, it can be very detrimental.
I mean, what we have now is a clique of ideologically-aligned people who insta-approve each other’s bad PRs outside their domain and ignore or jam-up the PRs of people outside their clique.
You can say it’s a failure of management, but this is the primary tool used by the ideologues. And I’ve seen it used so at various places.
What I haven’t seen is a real dissemination of knowledge about the code via review. At least not above and beyond what can be achieved by looking at the code and using blame to see the changesets and looking at the associated issues.
This kind of social behavior is corporate politics and a failure of management of course.
Okay, I’ll go tell management they’ve failed. 🙄
No, you tell about management the problem and how only their amazing social and people skills will be able to do something about it.
That’s an engineering culture problem. Not a PR problem.