I haven’t always been a person with “good student syndrome.” However, the more I immersed myself in the software world, the more I became a rule follower. After gaining more experience, I am now becoming less of a rule addict again. Simply put, I have realized that most rules are situational. So, while I can’t give you a definitive set of rules to follow for your Pull Requests (PRs), I can share my recommendations and the principles I envision for my team.
Keep Your PRs Small
No matter how much experience I gain, I still fear XL code reviews when I see them.
However, I still think small PRs are a myth.
Why?
In our project, small code reviews consist of up to 400 lines of change. This can vary for your project. Setting a hard cap on the number of lines is impossible. Most of our requirements lead to changes across the data layer, business layer, and presentation layer. While it might seem possible to separate these changes into different PRs, doing so often results in dependencies between PRs. This can make it difficult to understand the historical context of a change because its decision might be tied to another PR.
So, can we create (almost) independent PRs instead of a single large PR?
At least try. While some changes must be pushed together, many PRs implement more than one thing. If the scope is large, there is no avoiding a big PR.
If a PR is large, the reviewer should divide the code mentally and review it in multiple sessions. If you are reviewing 2000 lines of code in one sitting, you are almost certainly missing something important, which could lead to costly issues down the line.
Do the First Round of Review Yourself
I don’t usually send audio messages. But when I do, I always listen to them first. My sister loves to make fun of me for that.
Call it professional deformation.
When I create a PR, I always conduct the first round of review myself, and I always catch simple issues. Your default PR state should be ‘draft.’ Once you’ve reviewed your code, set it to ‘ready’ for review.
This is a great habit, but if your company incorporates AI-powered code reviews, it may not be necessary. Use the best tools available. At the moment, the best tool is still human review for my team.
React to Every Comment
Once your PR gets a code review, it’s a great opportunity to communicate. Always acknowledge feedback—even a simple thumbs-up is enough. If a comment is ignored, it can lead to confusion and wasted time when changes are made later.
Most code review tools divide feedback into two categories: comments and tasks. Their purposes differ:
- Tasks: Mark them as completed, provide an explanation for why they weren’t done, or raise concerns.
- Comments: Acknowledge them. If you implemented a change based on a comment, say so. If you disagree, provide reasoning.
Respect the Workflow
A code review process involves responsibilities for both the reviewer and the reviewee. If a workflow is defined, follow it. If not, initiate a discussion to establish one in your team. You should at least clarify:
- Who resolves a comment or task?
- Who merges the PR?
- How many approvals are needed before merging?
I know that each company has it different, so respect yours.
Provide a Good PR Title
Once a PR is merged, its title becomes the commit title, regardless of the number of commits in the PR. Usually, commits are squashed, but in some cases, all commits may be retained. I won’t go into that here.
Commit titles should quickly identify changes in a Git log. Keep them meaningful yet concise—about 70 characters is ideal. Some projects may have naming conventions already.
Bad examples:
- Bug fix
- FIX ISSUE #1234
- wtf
- do stuff
- Fix price format pattern issue where reports work without locale but fail for all other cases with locale, causing fallback to default format; ensure consistent formatting
Even so-called ‘senior’ developers sometimes use vague titles. Don’t be like them. Use explicit, short titles that clearly describe the change without unnecessary details.
Good examples:
- Fix modal alignment issue on small screens
- Replace image compression library to compressX
- Add unit tests for campaign threshold validation
- Improve performance of grid rendering in dashboard
I think you get the idea.
If your project follows a multi-repository/module approach, consider adding a module alias to the beginning of the commit title. This is especially useful if your Git client aggregates commits from multiple repositories.
Use Imperative Form for Commit Body
Use the commit body for further details, preferably in bullet list format. We use the imperative form because we consider each commit as a set of “actions” applied to the codebase. When applied, they should have the described impact.
Some examples:
- Alter EMPLOYEE table to add a constrain on the EMPLOYEE_AGE column
- Adapt the pom file to be able to use the new profiles from parent pom file
- Change the Colors Enum to support the material library
The imperative form is an effective approach. If necessary, add remarks for special considerations at the end of the commit body, it might be important in the future.
Provide Meta Information
Developers often make design decisions during implementation. Instead of waiting for reviewers to ask questions, preemptively add comments explaining those decisions.
During reviews, I sometimes encounter “magic” details with no explanation. When I check the ticket, I don’t find any mention of them either. After asking the developer, I often find out that the decision was made in a discussion with a business analyst. This kind of information should be included as meta information.
However, don’t overdo it. Avoid stating the obvious, like, “I made this parameter final because I don’t want it modified inside this function.” Instead, focus on hidden details and design decisions.
Key Takeaways
- Keep PRs as small as possible while balancing the need for logical grouping of changes.
- Review your own PR first before submitting it for review.
- Acknowledge and respond to every review comment to ensure smooth communication.
- Follow your team’s review workflow and clarify responsibilities.
- Write meaningful and concise PR titles for better commit history tracking.
- Use the imperative form in commit messages and provide a structured commit body.
- Include meta information when necessary to give context to reviewers and future maintainers.
