Course notes: Code Review: Best Practices
By Andrejs Doronins
Mix of course notes, and my own thoughts.
Good intro and reminders of how we should work together. Nice to see what we’re already doing, and somethings we can expand. Trust and respect are key. If the team works well together, then comments are less likely to be taken badly. We can assume everyone means well.
Make the reviewers life easier; small PRs, you’ve added comments, linted the code, etc. Consider what sort of review you want, e.g. wip structure, refactoring, “normal” change.
Make the comments about the code, and not personal attacks. Comments with OIR & examples are pretty great. Consider standard labels to show the importance of comment. e.g.
blocker. Comment on the good things too!
Some reviews might lead to bigger conversations, which should be taken offline.
Seek ways forward. If there are two different options, discuss and make a choice. Team votes if needed, escalate if really needed.
Why Code reviews exist
- Catch bugs
- Code quality
- Exchange ideas and better practices
- Requestor and reviewer(s)
- Check in with other work
Good culture and people = Good reviews
Opposite is also true
Course focusses on the “soft skills” of code review.
- How to raise a Good MR
- How to do a good job reviewing
- How to approach MRs
Code styling issues can make it harder for people to read and understand.
- Create a style guide
- Make IDE config(s) for the rules.
- Lots of options to do it automagically
- e.g. pre commit hooks
- IDE plugins for static analysis
- e.g. sonarlint
- Use automated tools to check the rules for you
- e.g. Sonarque
- Two reviewers is generally good
- Someone needs to the business domain and logic well
- PRs are always a chance for others to learn
- Even reading and asking questions is useful
- Make personal preferences clear in comments
- Different valid ways to do things.
Submitting a great Pull Request
No comment on a non-trivial change? Doesn’t smell right
You don’t own the code. The team does.
Exceptions to all rules
- Small PRs
- If bigger change, try break them up
- And/or raise earlier as a ”draft”
- Change is split into atomic commits
- Commit should contain related changes
- Review your code in
- You’ll likely spot things, and can fix quickly
- Feel you need to clarify things?
- Suggests the code could be improved
- Add comments to code
- PR comments make sense for short term info
- e.g. “as discussed with X, we did Y because of Z change”
- Respond to every comment
- Even if not accepting the comment
Providing effective feedback as reviewer
- Don’t be an arsehole
- Use requests or questions
- Feedback is about the code, not the person
- OIR rule (Observe, Impact, Request)is useful for comments
- Explains why you’re asking for a change
- Examples are great addition to OIR
- Make it clear that comment is minor/major
- Add praise comments too!
- Don’t let the review drag on. We want to provide fast feedback.