Code Review Best Practice
When you add the sign-off member to your Pull Request, please not only add this member to reviewers, but also add
Adding to assignees will make us count the sign-off correctly. Thanks!
Why code review
- Ensure more than one person would be familiar with each piece of code.
- Enhance readability
- Knowledge stays within the company
Every pull requests requires:
- Code-reviewers (1-to-many volunteer nominations)
- One sign-off member (limited committee)
Each commits requires:
- Write git commit message in English
Having your code reviewed
- Review your own code and look at your own diffs before submitting code for review
- Make sure they pass all builds as well as all tests and code quality checks before assigning reviewers
- Try to keep the code review changes small (ex. under 300 lines)
- Be grateful for the reviewer’s suggestions (“Good call. I’ll make that change.”)
- Explain why and how the implementation works at a high level. Simple outlines or diagrams are very helpful.
- Push commits based on earlier rounds of feedback as isolated commits to the branch.
Do not squash until the branch is ready to merge.
Reviewers should be able to read individual updates based on their earlier feedback.
- Try to respond to every comment
While you are reviewing code
- Understand why the change is necessary (fixed a bug, improved the user experience or refactored the existing code)
- Identify ways to simplify the code while still solving the problem
- If discussions turn too philosophical or academic, move the discussion offline. In the meantime, let author make the final decision on alternative implementations
- Offer alternative implementations, but assume the author already considered them. (“What do you think about using a custom validator here?”)
- Sign off on the pull request with a thumbs up, “Ready to merge”, or “LGTM” comment
- Remember that you are here to provide feedback, not to be a gatekeeper
How to write a git commit message
- Separate subject from body with a blank line
- Limit the subject line to 50 characters
- Capitalize the subject line
- Do not end the subject line with a period
- Use the imperative mood in the subject line
- Wrap the body at 72 characters
- Use the body to explain what and why vs. how
Here is an example: