Lesson Objectives¶
- Summarize code review best practices
- Demonstrate code review on a pull request
Code review best practices¶
Code review, like peer review, enhances the quality of the work being submitted. All involved parties are collaborating to enhance the final product of the pull request.
When performing code review, you are using your expertise to weigh in on a contribution. You aren't going to have expertise on everything, but your experience in the code and using it is important and relevant to the reivew.
As a person getting code reviewed, appreciate the reviewer taking time to give you feedback on your work! They are both/either invested and interested in the work you're contributing.
I'm going to be talking about a few different perspectives this morning. Let's define them.
maintainer -- a person who keeps things running in a software project. Sometimes maintainers aren't focused on new features but instead focus on overall project infrastructure and usability.
contributor -- a person submitting a PR, issue, or feedback to a project. They may have never interacted with the project, or maybe they do so frequently.
user -- people who use the code but don't contribute upstream. I try to think about how changes in my code might affect these people since they're often not weighing in in code review.
What is code review?¶
I like this description from gitlab.
Code review as a reviewer¶
My approach in a review:
- Read the PR description. What does the author want this code to do? Is it useful/applicable to the code it's being submitted to (is it in scope)?
- Read the documentation submitted with the code. Do I understand how to use it now? Is there anything that's ambiguous that coould be enhanced by clarification? Are there docs missing that need to be added?
- Look through the code. Do I follow the logic of what's being done? I ask questions if I don't.
- Check the tests. Have tests been added for any new features? Do they cover the scope of the new feature? Do they help me understand the code in any way?
- Consider the user. Is the API of the code changing from what already exists? Will it impact users significantly?
- Look through the code. Suggest constructive improvements. Do I see a lot of functionality that's repeated that can be put into a single function? Do I see a place where a design pattern could be used? I try to add suggestions for improvement as code snippets.
- Try to use the feature. I don't always do this, but for new features I do. Do I run into any errors when I use it? Does it make sense in my workflow?
I also like to use a good code review checklist so I don't forget things. Here's the one my group uses: https://arfc.github.io/manual/guides/pull_requests
Other considerations¶
- When you accept the code, it's possible the contributor will not return. If you're the maintainer of the project, is this code you are comfortable maintaining moving forward?
- Adding dependencies should be done mindfully.
- If you notice something that you'd like changed that isn't directly related to the PR improvement, open an issue and suggest it as a later fix.
Dos in code review¶
- Thank the contributor for their contribution
- State what things you particularly like about what was submitted.
- Be positive whenever possible.
- Ask questions about things you're confused about. You're probably not the only one.
- If doing multiple rounds of review, try to not let things stagnate.
- Use suggestions to show your thought process in code changes.
Don'ts in code review¶
- Nitpicking can really set the tone of a review. Is this small thing really important to the code or is it personal preference?
- Don't reject code for stylistic or small changes. This is very demotivating for a contributor.
Tips for smoother code reviews (and development!) in your project¶
- Your reivews are there to enhance the code that was submitted for other maintainers, developers, and users. You are collaaborating with the author.
- Using a linter and formatter mean that reviews avoid style bikeshedding.
- Having good test coverage and CI mean that the a new contributor gets automatic feedback on how their code fits into your project. This also allows for code reviewers to focus on code readability and applicability rather than helping the author debug.
- Having good developer documentation (what linter and formatter do your code use? What is your docs style) helps make the contributor experience a bit less iterative.
- Having your issue tracker with clearly described issues helps people other than you understand what's going on in the project.
- Labeling issues with things like
good first issue
and what type of fix the issue is (e.g.documentation
,testing
, or something feature-specific means that potential contributors can more easily choose a contribution they are comfortable with. This also ensures that the contribution somebody addresses from the issue tracker is relevant to the project. - Consider having a project decription in your README. What is the purpose of your code? What is it? What is it *not*?
A few more things¶
- If you know the contributor and you have done a few rounds of review, consider scheduling a call or sitting down with them to try to resolve things.
Code review as a contributor/developer¶
Getting your own code reviewed can be exciting! But it can also be a little nervewracking.
Let's talk a bit about the process of responding to a code review.
My approach:
- Read through the reviewer's comments. Consider their perspective as I read the comments. If I don't understand a comment, I ask a question as a reply to the review comment.
- Answer reviewer questions, if there are any.
- Incorporate any code suggestions the reveiwer made that I like through the github interface. This helps preserve authorship of their contributions.
- Make code changes locally and push them to my feature branch. Address review comments in seperate commits to help split things up.
- Notify the reviewer once I've addressed all comments and all open discussions have been resolved.
More Resources:¶
- https://google.github.io/eng-practices/review/reviewer/
- https://stackoverflow.blog/2019/09/30/how-to-make-good-code-reviews-better/
- https://mtlynch.io/human-code-reviews-1/
- https://mtlynch.io/code-review-love/
- https://developers.redhat.com/blog/2019/07/08/10-tips-for-reviewing-code-you-dont-like#code_review_strategies
- https://kickstarter.engineering/a-guide-to-mindful-communication-in-code-reviews-48aab5282e5e