The following is meant to be a set of guidelines, not dogma. Use your best judgement. The goal is threefold, to have a practice for engineers to learn about what we collectively care about. To maintain quality of the code base as well as knowledge sharing about our systems.
- Create PR, make sure to include JIRA ticket if such exists in the description or title of PR
- Assign 2 developers as minimum but can include the whole team if the author feels it's needed.
- Answer any questions the reviewer has.
- Push changes after review is done as a new commit and make sure reviewer and yourself are satisfied with the change.
- Reviews the PR and provides with comments. The review should be prioritised over development.
- Once author has changed needed changes (this includes both reviewers and authors perspective), merge the PR.
- If there is disagreement between author and reviewer raise it with technical lead or the whole team to find a solution. Do not let the PR sit and rot.
- Can you read and understand the code?
- Are there any obvious errors?
- Will the code have any undesirable effect on other parts of the system?
- Are there any reasonable improvements or refactoring that can improve it?
- Does the code change require any additional tests?
- Rubber-stamp code reviews without thought are without value.
- Code reviews need to actively evaluate the design(don't confuse this with general architecture) and clarity of the code, not just whether the variable names and layout are compliant to a standard.
- A feature can be written in quite different ways. Different isn't worse, don't criticize code unless you can make it measurably better.
- Code reviews are useless unless you follow up on the recommendations quickly. PR's shouldn't be merged unless comments have been addressed and everyone involved are satisfied.
- If there is disagreement between author and reviewer raise it with technical lead or the whole team to find a solution. Do not let the PR sit and rot.