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
    0·
    6 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
      0·
      6 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: given 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
        0·
        6 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.

        • mattreb@feddit.itOP
          0·
          6 months ago

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

          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.

          So basically you’re saying that all the comments are welcome, and should be argued, but you have to appoint a person that have the final say on what to merge right?

          I’m not sure what problem you think is being moved to the reviewer.

          The problem of deciding what should be merged or blocked, I’ve mainly made this post to understand that, so saying “#5 or you’re crazy” doesn’t answer much. But I’ve probably worded that badly.

          • MagicShel@lemmy.zipEnglish
            1·
            6 months ago

            If a story gets created, the code will be merged… when it’s right. If you’re talking OSS, then I am out of my element, but I’ll wager there’s no universal answer because each code owner sets their own standards.

            you have to appoint a person that have the final say on what to merge right?

            If there is work that needs to be done, and you are asked to do it, the code will be merged when it’s right. I don’t decide what to merge, I decide when something is ready to merge.

            The problem of deciding what should be merged or blocked

            If you want to merge something and I read it over and reject the PR because you forgot about concurrency, that doesn’t mean you don’t get to merge, it means that it’s not finished baking. And assuming you give a shit about the code your response should be “oh shit, lemme fix that and resubmit” OR “actually this code will never have concurrent access, and here’s why.”

            You’re making this process sound adversarial when it isn’t. It’s a group effort. Everyone wins or loses together.