How to Make the Most out Giving Code Reviews
Code reviews are important to our work as software engineers. Just like writing code, it’s critical that we practice reviewing code and giving feedback on it. Below are a few thoughts from engineers at VTS on objectives of a code review and how we can get better at them.
Code reviews are about empowering each other to write really great code.
The two main objectives of a code review are to:
Improve code quality
Reviews can improve code quality by:
- Finding and fixing code smells. We want quality code, especially if the app is not a prototype.
- Validating that proper short-term/long-term balance considerations are exercised.
- Catching bugs and edge cases. Not all bugs nor edge cases will be caught, but a review can reduce them. Any that slip through are a good learning opportunity.
- Ensuring good commit practices are followed.
Share technical and domain knowledge.
There should be teaching and learning for both the author and reviewer.
Tips for Giving a Useful Review
The practices below can help you achieve the above objectives when doing a code review.
Ask what angle you bring to this code review
Take time to reflect on why your (or your team’s) review was requested for this pull request. You are likely being asked because of either your domain knowledge or your technical knowledge. Knowing this can adjust how you approach the code review. You may need to review the entire request, or you may only have a few lines that are pertinent.
Once you know your angle then it’s easier to know how you can contribute to the code. Can you improve the design of a new class? Can you help refactor some of the code? Are there edge cases you know from experience that are not addressed?
If the code addresses a problem, you can use your angle to think of how you would have solved the problem. Does your solution differ? You solution could differ because you:
- Have a different skill and/or experience level
- Lack context on the problem being solved
- Bring a different domain knowledge to the problem (e.g. data quality vs. performance)
- Have a different level of exposure and experience with the code base
Different solutions does not necessarily imply one is better than another — it’s just a different way to approach the problem. If you feel a different solution is better, be sure to explain why. These differences can lead to questions about the author’s implementation and can start a constructive conversation.
The outcome of this practice is that you offer perspectives the author didn’t have or maybe didn’t think about.
Do not approve what you do not understand
This applies to all levels of engineers. If you need more information from the author, then ask. If an author wants a pull request approved, then they will take the time to explain the code to you.
You should not feel pressured into approving a pull request. You should also not feel foolish for asking questions to clarify what you do not understand. It’s important for every workplace to foster psychological safety so that everyone can feel free to share ideas and ask questions.
There is also the chance that if you do not understand it then the author doesn’t understand it either. Maybe it was a copy/paste from Stack Overflow. Maybe they stepped away from the code and can’t remember what they were trying to do. Maybe it’s just too complicated. Ensure understanding before merging.
The outcome of this practice is possibly a simpler solution that’s easier to maintain and reason about.
You may need to checkout the request locally
Sometimes you need to see how the code is being used in the system as a whole. If this is the case, you likely need to check out the request locally and look through the files in your editor. Ask questions like:
- Is this method being used properly?
- Does this adhere to the same design as other classes like it?
- Are there even more opportunities to refactor and do they belong with this pull request?
- Are the tests comprehensive enough?
- How does this integrate with the rest of the system?
The outcome of this practice is that you better understand how the code fits with the software as a whole. This can lead you to find further opportunities to refactor or fix code that aligns well with the pull request.
Avoid criticism without a suggestion
If there truly is a flaw in a pull request, then it’s important to point it out. What is equally important is to follow that up with a suggestion on how to fix or avoid that flaw. It’s important to do so respectfully and in a non-degrading way. You can do so by fully explaining why something may not work and what a better solution is. You can also link articles or Stack Overflow answers.
Admittedly, there may be times where you know there is a code smell or a way to improve, but are unsure what to do. If that is the case, be sure to leave an open ended question for the author so you can start a conversation on how to fix it.
The outcome of this practice is that the author understands why the criticism was given and what a solution could be.
Find opportunities for celebration
We all need positive feedback to shore up our confidence. Putting your code out there for others to critically analyze is a daunting task. We need to find ways to keep encouraging each other to do our best. Do you know if the author is trying to master a refactoring technique? Point out the progress they are making if you see that technique. Is code now more readable? Praise them for it. This applies to all levels of engineers.
Another benefit is that it shows you took the review seriously. Sometimes we do not have any feedback and simply approve the request. How does the author know you really looked at it? Celebrating something in the code is one way.
The outcome of this practice is the author feels encouraged to continue contributing to the code and to keep learning and get better at their craft.
Keep the feedback loop tight
Feedback is important through the whole process of writing code. When appropriate, seek for feedback before you put up your pull request. In the beginning stages, it doesn’t generally have to be about the code. You can ask if there is an idea you haven’t considered or if this is the best path forward. Talk with your teammates or those with domain/context knowledge early on.
Once the request is up, be sure to respond quickly to any feedback. It’s important to keep working on the pull request so it is not abandoned. It will also help you to not context-switch too often.
You should also balance asynchronous and synchronous conversations. Sometimes it’s just faster to talk in person and ensure understanding among all parties. If a decision is reached there, be sure to record it in writing.
The outcome of this practice is merging your pull requests quicker. If you’re working on a big code base with lots of merges, this is particularly important.
A note about commits
This short section deserves its own post, but it’s very important to mention here as part of code review practices. To help the reviewer, the author needs to maintain a clean and informative commit history. Good commits can help you get the most useful feedback possible
A good commit history means the commits are broken up vertically instead of horizontally. This means that each commit passes all tests and doesn’t break the main branch. A good commit history also means the commits tell one cohesive story as part of the pull request. The person reviewing your code can follow your thinking and decision-making. A good commit history also leaves detailed notes on why you did what you did. Simply stating that you did something does not expand on anything the code does not already do. A good commit history can act as documentation for your application. Lastly, good commits should be self-contained, meaning they add value on their own and do not require a reviewer to look at other commits to understand them.
As a reviewer, be sure to keep on the lookout for opportunities to create good commits.
The outcome of this practice is a code base with informative commits that document how your application works for generations of developers to come. Another positive outcome is it makes for an easier code review.
- Jacobo Blasco
- Caley Brock
- Andrew Marshall
- Clemens Park
- Jess Kane
- Eric Firth