Cardinal Rules Developers Break on Pull Requests
This is why teams don’t move as fast as they could
Writing code is a collaborative team effort. Good code may be written by a single developer, great code is reviewed by many. Therefore, the way a team approaches the code review process tells a lot about its culture.
Strangely enough, code reviews are often viewed as a slowdown or an unnecessary burden. And while it is true that they act as a quality gate, many developers do not seem to understand the most important purpose of a review.
Code reviews build knowledge circles
Consider this: Never ask yourself if your software has bugs. Better ask yourself: How many?
Your odds at discovering a fatal flaw before moving a feature to production (or later on when debugging the defect caused by it) dramatically increase with the number of people involved in a code review.
But pull requests are not just the place to judge the mere quality of code.
Much more importantly: You are building a knowledge circle.
Every person who is part of the review process will — when push comes to shove — be able to troubleshoot, maintain or extend the code much quicker than any developer on the team who was not involved.
Hence, you want to keep the cognitive load for a review as low as possible to make sure everyone involved gets a fair chance at internalising your code changes.
Mistake #1: The change is too big
A classic problem in writing software is an engineer’s noble desire to deliver a feature as a whole. This enables us to roll back the entire commit if necessary and keeps everything in one place.
However, due to bad system design, sloppy story splitting or miserable planning, things get out of hand more often than we would like them to.
Have you ever looked at the file count of a PR and thought to yourself: “Nope. I’m out.”?
This is not healthy and is to be avoided at all costs. A changeset so big requires too much cognitive bandwidth to be properly reviewed and internalised.
You are bound to make mistakes and people will sign off on the code just to be done with it.
The general rule of thumb you should follow is:
A good PR contains a few dozen lines of human-written code at max.
If you are facing this issue constantly, try to produce smaller stories through more rigorous splitting or by creating incremental PRs.
Mistake #2: Unrelated changes
Do you know that feeling when you are working on a codebase and you get the sudden urge to fix something else “while you’re already at it”?
It’s just one line anyway, right?
While the intention is certainly well-meaning, these are the type of changes that divert the attention of your reviewers to less important aspects of the code.
Instead, keep your pull request minimal and address only one feature or change at a time:
Write concise, single-concern pull requests. Every other fix ends up on a separate branch.
Make this your mantra and you will soon live a fulfilled life as a minimalist PR monk. The enlightenment will flood your body with pure happiness. Trust me.
Mistake #3: The senior syndrome
Senior members of a team should guide the lesser experienced, facilitate complex discussions and remove roadblocks.
However, being software engineers they are —naturally — more on the opinionated side of the spectrum.
In an engineering team made up of different experience levels, always be alert to the senior syndrome where the opinion of the most senior developer is the gold standard and overrules others without questioning.
In the interest of healthy team culture, everyone should internalise what the “real” gold standard is:
In code review discussions, everyone is equal. The best arguments win. Nobody pulls rank on another team member.
If all developers are on equal footing, it is much easier to innovate and raise the bar together.
Mistake #4: Too many pull requests
While this may sound somewhat counterintuitive to what‘s written above, you can, in fact, have too many pull requests.
If everyone around you complains that they are drowning in code reviews, it is probably time to take the foot off the gas and slow down a little.
Teams can negotiate strategies to allow a limited number of PRs which can be in review at any given point in time before adding new code (much like Kanban).
Another viable option is mob programming which will — by its nature — produce reviewed code and involve developers much deeper in the knowledge sharing process.
Write the code you would want to maintain yourself
Staying away from the four fundamental caveats outlined in this article should give you a good baseline to operate on.
On top of that, make sure you get the basics such as readability, test coverage and documentation right. Bad or poorly reviewed code hurts the most when you need it the least.
And when that happens, you really want everyone to be able to step in and help fix the mess.
Hi, I’m Friedrich! I’m a manager and developer at work, a hobby-chef, musician, data and language enthusiast at home. You can find me on LinkedIn.
If you want to support my — and other authors’ — writing, consider becoming a Medium member.
