Code Review Best Practices
Summary
When you’re on a development team with multiple contributors working on a shared code base, code reviews are an essential tool to ensure consistency and quality across a project. In order to be an effective tool, though, developers need to approach reviewing code with the right attitude. A code review is not a forum to show how smart you are, or an opportunity to push your private agenda for the code base. The last thing you want to do is to tick off your team members when you have an opportunity to help make the code base improve. Here are some guidelines to follow when you are reviewing code to ensure you and your team get the most out of reviewing code.
Push Team Standards, not your Opinions
A code review is not a forum for you to implement your preferred naming conventions. It’s also not the place to start pushing for sweeping changes in conventions to the code base. Make sure to differentiate very clearly between what you would like to see, because it’s your opinion, and what your team has agreed to do when you’re commenting on code.
Example, say your team is developing a micro service using Node and Express, and you commonly define your routes like this:
import * as myController from ‘../Controllers/myController’;router
.route('/my-special-endpoint')
.get(myController.handleGetFromEndpoint);
If you want to start introducing a new pattern, that’s great, but you wouldn’t start that conversation (or even continue it if it’s an ongoing discussion) by Requesting Changes on the PR and saying:
Route handlers should use Classes instead of exporting objects referencing Functions. Since we’re implementing a new controller here, this is a great place for us to start transitioning these to be the proper structure.
A code review is a terrible place to block a team members’ work for the sake of implementing a new standard you want to push. This goes for team members, tech leads, and CTOs alike. Before you block a team member’s work, ensure everyone is on the same page about what conventions and standards are in place and the direction the code base is going.
A more reasonable comment on the above example would be:
No need to hold up this code based on this, but I’d love to see us start to move towards using classes for these route handlers. I think it would give us more flexibility and better testability. I’m seeing this as an industry trend that I think we could benefit from. I’ll bring this up in the next standards discussion to get a feel from the group.
Now, you’ve exposed your team member to a way you think the team could improve, but also made it obvious that the code is fine as is and you will bring it up for further discussion in another forum.
Don’t Keep Commenting on the Same Thing
If you find a team member has violated a standard in two dozen places, please… for the sake of all reasonable developers everyone, don’t add two dozen comments on each instance. Any developing who sees two dozen comments on their code will become disheartened, deflated, and probably a bit skeptical. Instead, comment on it once or twice, and ask the developer to fix it everywhere.
Note: I’ll caveat this by saying I have worked with difficult team members who won’t change a thing without being explicitly asked. In this case, you may need to be a bit more explicit, but if you find yourself resorting to this, it’s probably time for some direct conversations with the team member and team lead.
As a contrived example, let’s say your new team member doesn’t know that your team uses camel case variable names instead of snake case:
first_variable = ‘A message for the user.’;
second_variable = ‘Another message for the user.’;Please don’t comment on every instance of this and DEFINITELY don’t comment on both lines. Instead, make a general comment on the first instance you see and prompt the team member to change it everywhere, preferably with a link to your team’s documented standards showing this example.
A quick note, our team uses camelCase for variable declarations, so make sure to change all declarations to follow this pattern. If you haven’t seen our project coding guidelines, you can find them here: {link_to_documented_standards}.
Keep Comments to a Minimum
In the same vein, if you are moving through a large PR, try to keep your comments to a handful of helpful items, addressing the most important changes you’d like to see for the code. In general 5–10 comments for a large code submission is more than enough.
If you find yourself making more comments than that, then it’s a sign that you should have a direct conversation about the direction of the code, or perhaps you’re repeating yourself. Having too many comments in a single PR will make it hard to identify what’s important and what’s not.
Don’t Comment Before the Author is Ready
There are few things that will tick off a human being more than critiquing their work before they’re finished. In Github, if a PR is marked as Draft, then by definition it’s not ready for you to review. Checkout the documentation:
Github is helping you here… listen to it. Wait for the developer to mark it as Ready for Review before you start hating on their In Progress work.
Final Thought: Accept the Review When It’s Good, not Perfect
One of the best guidelines I have ever seen on code reviews was to approve code when it looks good, not perfect. To paraphrase Voltaire:
“Perfect is the enemy of good”
Remember… writing code is hard, writing perfect code is impossible. Don’t hold up functionality because you think code can be tweaked to be slightly better. Focus on the important things:
- Does it follow the project standards?
- Is it tested properly?
- Most importantly: Does it work?
Keep a healthy perspective and make sure that the goal of your comments are to improve, educate, and collaborate. Clear communication will go a long way in ensuring that everyone gets the most out of your code reviews. Be the person people _want_ to have review their code, instead of the person people try to sneak code by.
