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):

  1. no code reviews, they are useless
  2. optional code reviews
  3. mandatory reviews on code that is already merged, optional fixes
  4. 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.
  5. 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.

  • toebert@piefed.socialEnglish
    51·
    2 days ago

    I’ll also say 5 but I have my gripes with it. Mainly with the “review from any other engineer” aspect that usually comes with it… I have met so many engineers whose review seems to just depend on who created the MR, as opposed to what’s in it. When an MR with 500+ lines changed gets reviewed in about 10s after requesting it, it’s kinda obvious that the system is broken.

    The people I’ve worked with who are good at their job and I’d probably be okay with them merging their changes without reviews would always ask for a review, even when it’s not mandatory or enforced. And their MR would already have comments by themselves around bits I might have a question around, and they’d even come with prompts of what they want input on. Whereas the people I wish wouldn’t even be allowed to approve anything would usually ask for an approval instead (even the wording seems telling). Sadly, often these 2 groups will have the same job title and HR will dictate that they should have the same permissions and say in things, which is what usually breaks the system IMO.

    And lastly, the amount of people who seem to treat reviews as currency/favours and just rubber stamp each others MRs without looking…sigh.