PR/Code Review Best Practices

My Other lists

• JavaScript/Node Best Practices
• Software Development General Best Practices
• Defensive Programming / Application Security Best Practices

---

I have collected some points and created this list for PR and Code reviews. I intend to refer to this post frequently to help me become a better reviewer.

Organization

1. Organizations with good code reviews ensure that all engineers take part in the code review process
2. Good code reviews use the same quality bar and approach for everyone, regardless of their job title, level, or when they joined the company.
3. Better code reviews pay additional attention to making the first few reviews for new joiners a great experience. Reviewers are empathetic to the fact that the recent joiner might not be aware of all the coding guidelines and might be unfamiliar with parts of the code. Provide relevant documentation (guidelines, style guides, etc) to help engineer improve their code.
4. Ensure no code makes it to production without a code review
5. PR and reviews should be time-bound. Ensure that PRs get reviewed, approved/rejected within a scrum cycle to avoid stale code
6. Foster a good code review culture in which finding defects is viewed positively, Give respectful and constructive feedback

Reviewers

1. Prioritize Review Over Coding
2. Integrate code review into your daily routine
3. Review at least part of the code, even if you can't do all of it
4. Review fewer than 400 lines of code at a time
5. Take your time while reviewing someone else's code, do not depend on others to catch errors
6. Do not bunch PRs for review. Instead, scatter reviews tasks throughout the working hours
7. Ensure the PRs comes with tests
8. Build and Test — Before Code Review
9. Ensure external documents if any (API, user manual, etc.) are updated
10. Give Feedback That Helps (Not Hurts)
11. Create and follow a code-review checklist.
12. While reviewing code, be mindful about the following:
    • Security best practices
    • Manageability (Readability, structure, style)
    • Architecture
    • Maintainability (Logic, Test Coverage)
    • Correctness (Functionality)
    • Invalid input/states
    • Usability (Performance)
    • Reusability
    • Object-Oriented Analysis and Design (OOAD) Principles
13. Verify that the defects if any -are actually fixed
14. PR Rejections must always include a better explanation for why the PR got rejected

Author/Reviewee

1. There are various git workflow, get yourself acquainted with different git workflows: https://www.atlassian.com/git/tutorials/comparing-workflows
2. You should annotate PRs with self-explaining comments before the review
3. Create Small Change Lists, Change Lists Should Be Complete on Their Own
4. Never self-approve a PR, always ask your peers for approval after code review
5. Before opening a PR, make sure you squash all your commits into one single commit using git rebase (squash). Instead of having 50 commits that describe 1 feature implementation, there must be one commit that describes everything that has been done so far.
6. Prior to making PRs always check to see you have merged the most updated code from the master branch. Update all affected tests and or create new tests for files you changed.
7. Do not push any code with runtime errors thrown by dependencies or source code. When one appears, you should fix that bug ASAP—even if everything else seems to be running ok. There’s a reason that error was thrown, so fix it and don’t leave it for someone else to worry about it.
8. Remove all unnecessary console logs prior to making a PR

PR Checklist
☐ Unit Test provided
☐ Deprecated code removed
☐ Syntax & Formatting is correct (Linting should help with this)
☐ Is the approach to the problem appropriate?
☐ Can anything be simplified?
☐ Is the code too specific to the product and needs generalization?
☐ Do forms need to sanitized
☐ Any obvious security flaws or potential holes
☐ Are the naming conventions appropriate?
☐ Are there enough comments inline with the code?
☐ Is there documentation needed?
☐ Is there anything included that should not be?
☐ Are all dependencies declared?
☐ Are there any code smells? https://blog.codinghorror.com/code-smells/

NOTE: If you want to update this list, please comment, I'll incorporate your changes.

Ref.
https://www.ibm.com/developerworks/rational/library/11-proven-practices-for-peer-review/index.html
https://www.michaelagreiler.com/code-review-best-practices/
https://ardalis.com/github-pull-request-checklist/
https://gist.github.com/sherakama/0ba17601381e3adbe0cad566ad4d80a5

Comments

[Antonio Miguel Matioli Junior]

Nice article!

Do you use code owners, as the responsible for the revisions? Witch criteria do you use to choose the reviewers?

[German Chyzhov]

That is a really great list of recommendations!
Thanks for sharing.

What I would like to add: reviewers should have enough experience in both tech and business domains. This way a code review would be more efficient for detecting tricky issues.

Organizations
p.1 - it is really hard to achieve this. I tried many times as a team lead to engage people, but simply most people don't like to look at others code. Or just don't have a time to do that because of a rush in development.

Reviewers
p.7 - in our teams we could accept PR without tests first to check if initial implementation is done in a proper architectural style.

Again, thanks for sharing!