Hey, I want your opinion on code reviews, what is the best way to use them in a professional environment? Pick one of the following and give me your thoughts (from the most forgiving to the most strict):
- no code reviews, they are useless
- optional code reviews
- mandatory reviews on code that is already merged, optional fixes
- mandatory reviews on code before merging (like a pull request), with a time-frame for optional fixes (i.e. whether to fix what has been pointed out is up to the author), merge will occur anyway.
- mandatory reviews on code before merging (PR) with mandatory fixes.
Of course in open source development with public contributions, you’ll often see (5), but I’m not convinced it could work in professional dev.
Edit: I’m talking about a team of 5 mid to senior devs (no junior or interns) working on a 2-3 year project without many security concerns, but feel free to give me your general opinion.
As with a lot of things in life, it depends.
I use 1-5 depending on the repo, who made the change, what the change is about, and how involved I am in the project.
Though the “time-frame” idea of #4 is usually replaced by conversations if it’s a coworker, as it’s more effective.
Q: about #3, do you mean on code that is already merged / committed to the default branch?
Added a few details in the post. Of course it depends, but let’s say you’re the team lead and you have to fix a general rule (otherwise no one is going to do them) which one you’re more likely to go for? e.g. if you choose (2), it’s up to every single member.
yes already merged, updated the post.
The way I see it, for any code review there are going to be different levels of recommendation regarding the comments. When I review, I try to make it clear what’s optional (/ nitpick) and what I’d really like to see fixed before I can approve it.
So even making some assumptions, I can’t choose between 4 and 5 because optional and “less optional” changes are often in a same PR.
The only one I haven’t done much of is #3. That one looks better if one has questions about code that was already reviewed, merged, and it’s likely in production.