Having fellow developers check your code (and checking theirs in turn) helps eliminate mistakes, clean the codebase, and share knowledge across the team. Code review is a purely human communication process. And a good way to approach humans (especially the ones you need to work with) is with empathy. This includes the obvious: politeness, kindness, and respect. But you can go a step further and remember that when you're assigning reviewers to look through your code, you're requesting another person's time and energy. Therefore, it's nice to make the process easier for them, and doing so will probably result in a more efficient code review. So, keeping empathy in mind, let's discuss some practical tips that can make your code reviewers happier.
The branch name is your first opportunity to give your task context. In case your reviewer decides to check the code out locally, they will need to find that branch among many others. Prefix the branch with the ticket number for tooling and automation, but by also using a naming convention that includes a description also helps when switching your own branches regularly, as you don't need to remember or look up the ticket number.
Keep commit messages concise.
Use an imperative approach for your commit messages. Phrase it as what will happen when you apply this commit.
One idea is to write a commit name before you start coding. This will help you stay focused on one step at a time, provide more clarity on what you need to do, and implicitly make your commits atomic.
By just reading the commit history of the pull request, the reviewer can already gain some understanding of what they will be reviewing, even before seeing a single line of code.
The first thing to keep in mind is that you, unlike the reviewer, are currently immersed in the context of your task and your solution. But when the reviewer opens your pull request, all they have is what you give them. They didn't write the code, and they didn't spend X hours or days working on the problem. Therefore, it's important to give as much context as possible in order to make the review more useful.
This should be covered by any repo-specific PR template, but generally:
- What? What is the task that is accomplished by this pull request?
- How? How is it implemented (an overview of your solution)?
- Why? If applicable (e.g., if several valid approaches exist), why did you choose this approach, or what are the impediments of the other approaches?
Thanks to this description, the reviewer will know what to look for in the code and why it was written the way it was. This enables them to provide more relevant comments.
Evaluating the code while knowing what the actual result looks like is much easier than trying to imagine it. If your task is related to changes in the UI, adding an image of the end result (or a video) will greatly benefit the reviewer's comprehension of the code. If your task involves complicated logic, you can attach a sequence diagram that explains the algorithm.
The bigger the pull request, the poorer the code review will be. Code review is a tough process mentally - you need to read code, figure out what it's doing, understand how it's doing it, and look for potential problems. The more lines of code you need to keep in mind, the higher the chance that you'll overlook something.
- Find and remove all of the debug logs and unused or commented-out code.
- Decide what to do with the
TODO:- Implement it.
- If you can't implement it right away, create a task or leave at least some kind of estimation on when it will be done.
- Delete it if it's either done or not relevant anymore.
- Exclude code-generated files, these files do not need to be reviewed. So seeing them explicitly in a diff doesn't add any benefit. Add a
.gitattributesfile to your project and list all the file types you want to ignore by adding the-diffpostfix.
Whether providing or receiving feedback, it is always helpful to keep in mind the following points:
- Accept that the same problem may have more than one good solution and yours may not be the best one; discuss tradeoffs, risk, impact, and reach a resolution quickly.
- Ask good questions that avoid judgment about the author's perspective, do not make demands.
- Do not make assumptions, ask for clarification.
- Avoid selective ownership of code: "mine", "yours"
- Be humble and respectful to keep it real and professional. In general, avoid using terms that could be seen as referring to personal traits.
- Talk synchronously if there are too many "I didn't understand" or "Alternative solution:" comments, and post a follow-up comment summarizing the discussion.
- Mention colleagues or teams that you specifically want to involve in the discussion, and mention why you would like their involvement.
- Before even look at the code, focus on understand what was changed, why it was changed, and how it was changed.
- Do not change the code while reviewing.
- Use a checklist to try to catch any mistakes, errors, violations against conventions, performance risk, etc.
- If you disagree strongly, consider giving it a few minutes before responding; think before you react.
- Explain your reasons why code should be changed. For example: the code is not in line with the style guide or even a personal preference.
- Offer ways to simplify or improve code.
- If discussions turn too much philosophical or academic, move the discussion offline to a regular Friday afternoon technique discussion. In the meantime, let the author make the final decision on alternative implementations.
- Aim to develop professional skills, group knowledge and product quality, through group critique.
- Be aware of negative bias with online communication. (If content is neutral, we assume the tone is negative.) Can you use positive language as opposed to neutral?
- Use emoji to clarify tone. Compare "Looks good :)" or "Looks good! 👍 " with "It's fine."
- Be grateful for the reviewer's suggestions. "Good call. I'll make that change."
- A common axiom is "Don't take it personally. The object under review is of the code, not you." Assume the best intention from the reviewer's comments and be aware of how hard it is to convey emotion online. If a review seems aggressive or angry or otherwise personal, consider if it is intended to be read that way and ask the person for clarification of intent, in person if possible.
- Explain why the code exists. ("It's like that because of these reasons. Would it be more clear if I rename this class/file/method/variable?")
- Extract some changes and refactorings into future tasks and stories.
- Try to respond to every comment.
- If there is growing confusion or debate, ask yourself if the written word is still the best form of communication. Talk (virtually) face-to-face, then mutually consider posting a follow-up to summarize any offline discussion (useful for others who be following along, now or later).
- Merge once you feel confident and continuous integration tells you the test suite is green in the branch in the code and its impact on the project.
Each team has their own tolerance for what is and is not a reason to reject a PR. This may be rooted in process, fairness, and expediency, and may be a company or team decision. Below is a list of reasons you might want to reject a PR. As a team, you can move through the list and decide your own reasons for rejecting a PR. Remember too, that there are multiple ways to handle PRs and your team may be more comfortable with comments and conversations than rejections. It's up to each team's individual style
- Breaks the app
- Fails to compile
- Introduces a bug
- Is likely to introduce a bug in the future
- Is likely to allow a bug to be introduced in the future
- Does not display correctly
- Does not display correctly on all platforms/browsers
- Adds in unnecessary functionality not approved. "You ain't gonna need it." (YAGNI)
- PR does not consider migration from existing customer to new feature
- Will slow down the user experience - i.e. N+1 query, non-performant code
- Does not consider certain edge cases
- Introduces a security issue
- Could introduce a security issue
- Privacy concerns, such as storing personally identifiable information (PII) unnecessarily or without proper authority, not properly securing PII, and accidentally logging PII in the system
- Don't have permissions for the code
- Code is legally contentious
- Code is patented
- Code doesn't include necessary copyright
- Not GDPR compliant
- Changeset is too large and needs to be decomposed into smaller PRs which could be considered independently.
- Failure to document what the PR is doing and why
- Wrong timing, needs other work to be done in the app first
- Lack of tests
- Tests fail to test feature
- Introduces a test that takes too much time for not enough value
- Tests implemented incorrectly (implemented an integration test inside a unit test framework)
- Breaks tests
- Non-descriptive naming
- Logic not clear from reading the PR
- Functionality is already implemented elsewhere in codebase
- Needs greater legibility
- Unlikely that someone could fix a bug in this code due to legibility
- Introduces too much technical debt
- Is not DRY enough, needs a refactor
- Too DRY, optimizing in advance and may be difficult to change later
- Lack of documentation
- Outside technology being introduced is not approved
- Outside technology being introduced is too costly monetarily
Eventually the pull request author and the reviewer will disagree. That is perfectly fine and actually a sign of good team dynamics. However, what is not fine is having endless discussions about it or one developer overriding the other because of reasons like personal opinions, seniority, access rights, etc.
It's better to prevent these situations from happening and have a conflict resolution strategy. One particular method that works well is to always require at least two code reviewers. In those cases, it's simply the majority vote that counts.
Any top-level comment on pull request ought be tagged with one of four emojis:
-
❓ for a non-blocking comment that asks for clarification. The pull request author must answer the question before the pull request is merged, but does not have to wait for the comment author to re-review before merging.
-
🎨 for a non-blocking comment that proposes a refactor or cleanup. The pull request author does not have to address the comment for the pull request to merge.
-
⚠️ for a blocking comment that must be addressed before the pull request can merge. The comment's author should leave aRequest Changesreview, and is responsible for re-reviewing once the pull request author has addressed the issue. -
😻 for a comment that compliments the author for their work.
Note, this policy does not apply to follow-up comments in threads.
Your comment likely falls into one of these categories. If you feel that it does not, default to :art:, as such a comment requires the least amount of work from the pull request author.
Your comment would probably be better served with a single focus. For instance:
-
❓ + 🎨 -- you likely cannot confidently propose a refactor when you have a related, unanswered question. You should likely just ask a question, and propose a refactor later.
-
❓ +
⚠️ -- if you are not confident that the pull request can merge, you should always mark your comment as:warning:. -
🎨 +
⚠️ -- this is impossible, as:art:comments are definitionally non-blocking, and:warning:comments are definitionally blocking. It is your responsibility to decide which it is. -
😻 + something else --
:heart_eyes_cat:is reserved for exclusively positive comments, that do not propose a change, ask a question, nor identify a problem. If you do not leave any:heart_eyes_cat:comments, you are likely not accurately representing your opinions of the pull request.
Someone left a :warning: comment on my pull request that I do not believe is blocking. What do I do?
Your first instinct should be to trust your teammates -- if they believe an issue is blocking, they are likely making that determination intentionally, based on their unique experience. You should respect their feelings.
If you feel like they have misunderstood the issue, ask for clarification, and answer any questions they may have.
It is the pull request author's prerogative to ignore :art: comments. If you feel like your comments require attention before merging, mark them as :warning:.
If you feel like your comments are non-blocking but should still be addressed, you should introspect about why you feel like that is the case. Perhaps the comments are indeed blocking, or your sense of what is blocking has drifted from the team's.
I want to mark my comment as :warning:, but I do not want to be responsible re-reviewing the PR. What do I do?
If you believe that code is dangerous to merge into master, preventing it from doing so is one of the most important things you can do as an engineer. You should try to make time in your schedule to be a part of that process.
In rare cases, you may need to prevent a pull request from merging, but do not have time to re-review the pull request. (i.e. reviewing a pull request the day before you go on vacation). In such cases, you should explicitly delegate that responsibility to someone else. i.e.
⚠️ I'm pretty sure this line of code introduces an n plus one! we need to fix that before merging. @raorao can re-review the PR once you've pushed your change.
I want the pull request author to take my feedback seriously, but I do not want to mark it as :warning:. What do I do?
One of the great advantages of this approach is that pull request authors do not have to guess as to what you think is important or not. You are not insulting their work by leaving a :warning: comment, you are simply communicating how important you feel the issue is.
If you feel like your feedback is not being taken seriously, this may be a symptom or larger issues on the team, and should be addressed outside of the pull request process.
- https://alexkitchens.net/better-pull-requests
- https://leoneperdigao.medium.com/pull-request-best-practices-fa20f7daeb3c
- https://blog.carbonfive.com/changes-requested/
- https://gist.github.com/hazmatzo/331afb7d94cebd565ef9ddca131c9dee
- https://gist.github.com/raorao/a8f01657ef600157958180832bdc28fe
- https://blog.codemagic.io/10-tips-for-better-pull-requests-and-code-review/
- https://twitter.com/mkobuolys/status/1489610640335515648
- https://dev.to/oumaymasghayer/guidelines-for-better-pull-requests-gj0