Code Review¶
Before you create PR¶
make sure:
- acceptance criteria from task are met
- you wrote automated tests and they pass
- remove unnecessary debugging code
- you assigned at least 2 persons as reviewers to your PR
- code meets guidelines
If you are a reviewer:
- Do code reviews as soon as possible. Of course your tasks have higher priority, but put yourself in another person’s place - you want to create a PR and get reviewed.
- Check code according to the following guides.
- Click “Approve” when you accept changes.
Basics¶
- Identify places which were over-engineered.
- Merge is done by the reviewer who approves the PR as the last one, except for a situation when deployment of the code in PR requires some additional configuration in target environment (setting env variables, migrations etc).
- Identify ways to simplify the code while still solving the problem.
- If discussions turn too philosophical or academic, move the discussion face to face or hipchat. The outcome of this discussion should be placed in the comment.
- Offer alternative implementations, but assume the author already considered them.
- What do you think about changing it one-line if-statement?
- In our guidelines we us ‘ instead of “, is there any particular reason you used “ here?
- Try to understand the author’s perspective.
- Don’t be sarcastic
- You can mark you comment as optional using [opt]
When your code is reviewed:¶
- Don’t take it personally. It’s a review of the code, not you.
- Try to understand the reviewer’s perspective
- You should respond to almost every comment - this means that every comment from a reviewer should result in a response or a code change.
- Can merge if you have at least two accepts and all comments and discussions are exhausted.
When you review someone’s code¶
- Remember that you can ask for clarification.
- Avoid using terms that could be seen as referring to personal traits. [dumb, stupid]. Assume everyone is intelligent and well-meaning.
- Be explicit. Remember people don’t always understand your intentions.