Over the last 3 years, I’ve reviewed more than 1,000 pull (merge) requests. During that time I learned a lot – mostly about how not to review code, how to make the process less painful, what makes good-quality code and so on.
Pull request needs to do just one thing
This is the single most important thing to watch out for!
While doing a code review, you need to keep a lot of things in your head. “What is the intention behind this?”, “How does this comply with the rest of the code?” and “Will this perform well?” are just some of the questions that need to be answered. So, when you have a focused pull request that is trying to solve a single problem, some of those questions become easier to answer.
Another important aspect is the size of the pull request. Bigger requests require exponentially more time to review. And when I know that I’ll need to spend more than 15 minutes on the request, you’ll have to wait up to a couple of hours.
Larger pull requests also have more mistakes, so the time to get approved will also increase significantly. That means that you could have code waiting to get approved for days. And if your company is agile, that increases the chances for merge conflicts which are just painful.
The best thing to do is split pull requests into meaningful pieces – one pull request should solve just one thing.
So, watch out for the common traps like renamings, code generalizations, and such. Although innocent and done with good intent, it removes focus from important parts – raising code quality and reducing error count. Just do your genius idea in another pull request.
Automate as many checks as possible
Continuing on the notion that a reviewer has to keep a lot of things in his head, that also includes checking code formatting, the existence of appropriate documentation, passable tests, and so on. I remember the time when I had to take all that into consideration – it was distracting and time-consuming.
Good thing is that the solution is pretty simple – integrate all the checks into the CI pipeline. That basically runs all the checks whenever someone submits a pull request, and won’t allow merging before all the checks pass. You as the reviewer will never have to worry about formatting again.
Automated tests help make sure that the author didn’t break anything, and that the tests are still passing. Depending on your approach to tests, this is easily the most important check to have in your pull request CI.
Automated code formatting makes all the arguments around ideal line-length or where to add newline disappear. Just pick a set of formatting rules as a team and give it to the auto-formatter. This will rid you of a lot of nuisance. If you’re having problems agreeing to a format, take a look at how Google, Facebook, or some other big companies are doing it.
Automated lint checker is also very important for languages like Python where code formatting affects code logic. Most code formatters for Python will only format code for which they know for sure won’t affect any logic. By adding the lint checker you’ll cover everything.
Some honorable mentions are type checker and documentation checker, but the most important thing is to automate checks that make sense for you and your team. If you think something would help, talk it out and try it for a week or two.
Sit with the author to review the code
It makes the review process faster since you’re able to iterate faster over the code with the author and share your point of view. The author can also better explain the reasoning behind his approach, like if he tried something already and why it didn’t work.
This can also be a great opportunity to connect with people more, which is very important. When your fellow developers see that you’re invested in their personal and business growth, they will appreciate it. This is also a great opportunity for you and the author to practice your communication which is crucial for well-functioning teams.
Watch out not to over-do this though. Some pull requests are too small and too simple to have any impactful conversation. In that case sitting down with the author might be a waste of time.
Be considerate when writing reviews
If you’re more experienced than the author of the code, you need to take into account that how you say things matters – a lot. A criticism written nicely can empower a developer to become better in the future, but it can also crush someone’s dreams.
What I found that works best is to ask open-ended questions – it’s not aggressive and even encourages the developer to think critically. Will this take more time than just telling someone what the solution is? Yes, short-term. But long-term you’re helping them grow and they’re less likely to repeat their mistakes.
So, next time someone opens the file inside the for-loop and not before it, instead of plainly pointing that out, ask “How would you reduce complexity here?“. It will mean a lot.
Add a flag called I ran this code locally
The thing that bugs me the most is finding an error in some small pull request because of which a functionality doesn’t work at all. That means that the developer didn’t even run the code – probably thinking it’s not necessary since the change is so small.
After that happened a couple of times, I added a flag called I ran this code locally which solved the problem completely. I stopped reviewing code that wasn’t run locally and people didn’t lie about doing it. There were no hard feelings because we all actually knew that it should be done.
Bonus: this is our template that every developer has to fill out when creating pull requests:
Merge request description
What is new?
- Implemented...
What has changed?
- Changed...
Checklist
- [ ] I ran this code locally
- [ ] I wrote the necessary tests
- [ ] I covered my code with type-hints
- [ ] I updated CHANGELOG
Trello card
Should know about
- Is there anything else that should be known?
- Any deployment notes?
- Any additional documentation?
In other news
I've been inspired by dev.to to write more, so I've started a blog. If you're interested in this kind of content, feel free to check it out and subscribe
Top comments (29)
Very good points, I like the "I ran this locally" - might be adding that flag :)
Turns out I don't also have to be a QA tester when I'm approving pull requests... this is a great idea.
It's a life-saver :D
I think it's OK to be direct about problems and suggest specific improvements, provided you phrase them right. For example "this variable name violates our coding standards [link] because [reason]. How about $wibbly_boing instead?" (but not "I see you haven't read our coding standards, this should be $wibbly_boing"). You should also say stuff like "this is great, I wouldn't have thought of doing it that way, hey @everyone take a look" sometimes.
When I'm writing documentation on working practices I put it something like this: "Code review is not a competitive sport with winners and losers, it is a never-ending conversation aimed both at making improvements but also pointing out best practice.".
As for "I ran this locally" I usually won't even bother reviewing until the automated tests have passed, which makes most of the problem go away. There's still the issue of non-existent (or flawed) tests which mean the code still hasn't been run locally, but that's the sort of thing I'd expect code review to catch. One of the automated checks I like to see (although most places don't have it) is that the test coverage is the same as or higher than the coverage of the merge target. That is again just a guideline, but a dev can expect Questions if he makes the coverage go down!
From Linus Torvalds <>
Date Fri, 6 Jul 2012 13:30:01 -0700
Subject Re: Regression - /proc/kmsg does not (always) block for 1-byte reads
Kay, this needs to be fixed.
Suggested fix: just use the 'seq_printf()' interfaces, which do the
proper buffering, and allow any size reads of various packetized data.
Of course, I'd also suggest that whoever was the genius who thought it
was a good idea to read things ONE F☠ BYTE AT A TIME with system
calls for each byte should be retroactively aborted. Who the f☠ does
idiotic things like that? How did they noty die as babies, considering
that they were likely too stupid to find a tit to suck on?
Linus
😇
Hahahahaha we can let him slide I guess
from my point of view, you should do this only when you have question to PR or if it is quite big, otherwise it is just waste of time.
i don't see how 'sitting with author to review code' it can help both grow.
this looks really great
I completely agree that if a MR od straight forward, there is actually no need to sit together. Will update to make it explicit.
For the growth effect, it's more relevant to learn how to better communicate, teach and take real-life criticism.
Voice feedbacks is always great :) But of course, as mentioned the PR should be "important" (yeah, it's relative... But if the PR only changes a very small thing it's not necessary).
I'd also add something to improve the conversation: I always read the PR before (maybe two times) & try to understand briefly, if I can't I write a question. In brief, I prepare the meeting.
I had a tech-lead that didn't read any PR before the sit, and so he was reading the code along with me. It was such a pain and a wast of time... That's why I use this process for me to review PR :)
Nice post :-) One thing I changed in my code review style - In the past, I always reviewed the code just in e.g. GitHub, but I soon switched to actually checking out the code locally and having a bit more of an explore. Found that I would spot things in my IDE that I never would have flagged just by looking on GitHub. Seems so obvious really but it really made a big difference!
I can confirm from my experience that those are really important points! Thanks for the article.
A have a concern to add to the point "Sit with the author to review the code". I need some time alone for every PR. With just few exceptions. I need some time to go over the code line by line, mark conspicuous parts, and check them. This may take more than 30 minutes for many pull requests because I really make sure to check everything. I already found many important bugs like this. If the author is sitting next to me for the whole time, it may be boring for him/her. And I feel under time pressure.
Nevertheless, I find this is a good point and I will try to sit more together with the author.
Thanks for your input. I agree that you need to have deep concentration to fully review code
Thanks for this post, it highlights things that are very important and yet often remain unspoken.
If your org is doing code reviews, it's worth writing something like that down and referring everyone to it.
It will not only push reviewers to be considerate (as is recommended here) but it will also make people who send PRs feel a lot more comfortable. You gotta admit that sending your first PR can be pretty intimidating sometimes, that pressure needs to go.
I am really up for the "I ran this locally" concept. Remember when I had to implement this for a dev team by running all unit tests automatically and update a file before completing a successful git push command when triggered.
"Bigger requests require exponentially more time to review." I disagree with this. It really boils down to how complex the changes are, but most of the time it's much easier to review 1 PR that's 20 files than 2 PR's that are 10 files each. Each PR takes some setup time to get the code running and understand what changes are happening. And even if the bigger PR required more time, it wouldn't be "exponentially" more.
Hmmmm maybe we're looking at it differently. It seems to me that you are talking about having one logical change (e.g. feature) in 1 MR vs. 2 MRs. I agree that 1 MR is preferable.
But I often see someone develop a feature and do a refactor or an unrelated bug fix, contributing to the file changed count. In that case I would rather have 2 MRs than one, because I'm losing focus from the important things.
These are great tips. I like the template that everyone uses when creating a pull request! 👍🏻
And still there are pull requests with just a title. 🤦♂️
In an ideal world the PR would automatically tested on a production like environment so there should be no need to run the code locally.
Great post! So many useful insights
Should one split a feature into multiple PRs to make them feasible for review? Like PR per task within the feature scope.
Ideally, the feature would be small enough to make for a reasonable PR. Breaking up the PR would split the context - it's hard to see the big picture.
For example, let's say that you're developing a web app and you are adding comments section. Maybe it would make sense to split the PR into backend and frontend, especially if those two are reviewed by different people.
Thanks for reply! Sure, feature slicing is always a good idea but I am talking about situations when feature cannot be further sliced down.
Yeah, then it's better to have it as one PR. What helps me the most then is to sit with the author and speak my thoughts out loud. It makes the process go faster
Nice points!
Small typo found: ... we all actually knew that it should be ...
Updated :)