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.

  • devfuuu@lemmy.worldEnglish
    36·
    3 months ago

    I don’t think I’ve ever not done #5 in all places I’ve worked. It’s insane to even think otherwise. What happens is that if anything really urgent needs to bypass the process, then the team lead or someone in high power can merge or bypass the normal procedure, but that’s rare.

    • mattreb@feddit.itOP
      210·
      3 months ago

      Ok then you’re moving the problem to the reviewer, what change request would you say is a mandatory fix? In the place you worked, were you marking the comments on the review in any way to make them mandatory / non mandatory? Or just discussing them one by one?

      Edit: give the amount of downvotes let me clarify this comment is not trying to be polemical, I’m genuinely asking the above questions.

      • MagicShel@lemmy.zipEnglish
        12·
        3 months ago

        Everything gets reviewed. If you have a constructive comment you put it in the review.

        I note when I think I have a better way of doing something but the existing way works fine, and I leave that fix up to the submitter. But sometimes I just say no this is wrong. And then whether it gets merged anyway depends on my role. I’m a tech lead now so that’s basically the end although if they want to argue their case I’ll hear them out.

        The devs I work with are all seniors and have all been working in the system longer than me, so I respect them and listen when they disagree. Generally when that happens I’m right in principle and they agree with me, but the code is a fucking mess and we can’t do A right without having to change the rest of the alphabet, and we have bigger fish to fry.

        In other positions I made my comments and whoever was in charge got to decide whether to accept the change or send it back. I try to always make at least one constructive comment even if it’s just like: I really like how you did this.

        I’m not sure what problem you think is being moved to the reviewer. It’s a team and everyone has the same end goal. I appreciate when my code is reviewed because any of us can make a mistake or forget to consider some outside factor. Code review is where assumptions are tested and discussed and hopefully everyone comes away knowing more and agreeing on a path forward.