Big PRs are hard to review. Everybody knows that. I recently read two great articles (thanks @sandeep) and I want to summarize them here with some of my own thoughts.
The articles are How to Scope Down PRs and An Opinionated Guide To Handling Pull Requests (PRs) On My Team.
Why should we make PRs smaller?
The first question is why we should even take care of that?
If you've ever reviewed code you know how hard it is to understand code that you didn't write. PRs should be small to be able to understand this one feature that gets implemented.
Code Flow over everything
Smaller PRs get merged faster. That is almost a fact. For me the main intention of a code review process is fast code flow. The faster the code goes safely to production the better. No PR should sit there for 5 weeks with lots of back and forth in the communication tab.
By focussing only on Code Flow it is possible to get fast feedback and iterate on your code.
Code Flow is a basic concept that is well known from DevOps principles (e.g. The DevOps handbook) and goes back to manufacturing (like always in IT). The principle says that not the single coding process is important but the overall flow into production is important.
That means the sole focus should be on getting code merged instead of focussing on the single task a developer is doing.
Code Flow > Everything
Testing in Production
With smaller PRs it is way easier to iterate on. That means you can implement some basic functionality, and improve it with the next one. If you see a major flaw in the basic functionality already you've won because you didn't put too much work into polishing work. There are always edge-cases in production you cannot think of. Merging the most basic functionality and testing in production is the fastest way of getting feedback.
Tips for Code Reviews
Here is my list of tips for code reviews. The list is in no order. These are my thoughts for code reviews:
- Code Flow is everything! Switch to PRs immediately even if you need to switch context. Code Flow > Context Switching
- All engineers should review code (not for every PR but in general)
- Master / Main Branch needs always to be deployed
- Only submit PRs that you self reviewed in the browser and not in the IDE
- Keep PRs small
- One PR has one functionality
- Use feature flags for bigger PRs and merge incomplete features also
- The Reviewer is not responsible for the function of the code
- The creator of the PR should hit the merge button and check the deployment
- Remove the merged branch in GitHub
- Ping Discord channels when you need a PR
- Ping PR creator that PR was reviewed
- Nobody checks emails or GitHub /JIRA notifications
- A PR needs to be associated with a ticket
- Leverage PR templates fully
- Describe your changes in a PR with images and videos
- Only review PRs if you are asked to do so
- Senior engineers / tech leads / PMs need to prepare Stories in a way that the engineer knows which outputs are expected
This article is basically a summary of the two mentioned articles + my own thoughts 🙂