Code reviews are a fairly common practice in engineering teams. But, they are often implemented poorly without getting the benefits.
How not to ask for an effective code review -
“Can you review my pull request so I can merge it and close my sprint task?”
How to ask for an effective code review -
“I don’t feel great about how I have done X, can I get feedback on the implementation”
Here are some things I’ve learned from working in many teams:
1. Don’t use code reviews as a blank slate for feedback
Ask for specific feedback from the reviewer. Read your own pull request changes first and leave comments for the reviewer.
Some examples are:
“I have made a few assumptions here, can I get feedback whether these are safe to make”
“I think this function needs to abstract a lot of complex logic, and I’m not happy with my implementation, would you do it differently?”
“I have never used this API before, so I’m not feeling confident about this block of code, does this feel right to you?”
“I wasn’t sure if there’s a way to log events already, so I added one. Let me know if this fits well in our architecture”
These questions also help the reviewer be genuinely helpful instead of simply nitpicking.
If you are new to team or new to the codebase, code reviews are a great opportunity with rich context to learn from your coworkers about best practices, good parts and bad parts to copy code from, even the team’s preferred code styles. You decide what you want feedback on.
2. Use code review to enforce coding style
a) Code aesthetics should be automated or ignored. Enforcing them by getting another developer to review your code is very expensive. There’s a reason it’s called code review and not code critique.
b) Code reviews are a good spot to look for opportunities to simplify the code with design patterns or the right abstractions. However, this is as a bonus. If the reviewer finds something while looking at your code, they can suggest an improvement.
It’s the place to share personal opinions about what makes code good with live examples and can help create a shared understanding as a team. But, it’s not the place to enforce “rules”.
3. Use code reviews to find bugs
This one’s rare but I’ve seen it. The job of the reviewer is not to find faults in your code. That’s what tests/testing is for.
It’s always a good idea to get fresh pair of eyes on a feature to catch edge cases, but the reviewer can’t catch all the bugs by simply reading the code and compiling it in their mind.
TL;DR:
- Ask for feedback, not permission
- Be specific, ask questions