‹ BackHN Continuity

Thread

There is more to code review than (automatable) detection

165 points · 117 comments · utiiiD

  1. dimbletimbers · · focus · HN ↗
    A defense of human code review I wish I saw more often, especially in light of the concerns people have about cognitive/comprehension debt: comprehension redundancy. At the end, if taken seriously, at least two people understand how the feature works (even if that number is, on average, trending closer to between one and zero). Ideally at least one of the two also comes away with a better understanding of the wider system and how the feature fits into or stands out from that landscape.
    1. atomicnumber3 · · focus · HN ↗
      If only one person knows the code, then PR time isn't going to save you.

      I'm tech lead and I basically don't review PRs, and I tell people this, with a caveat - if you can tell me what you specifically want me to review, for what specific purpose, I'm happy to!

      So "can you review this bit for race conditions" is great, love it. This forces people to actually think about what in their code they should be suspicious of, if anything.

      "Can you review this" [link to PR] is getting a rubber stamp because humans have never been good enough at "just spotting bugs" to make this worth it and now any LLM is better than a human.

      And for the purpose of understanding - PR time is too late. I have not reviewed a PR ("for real") in a long time and yet I could tell you how every system my people have built works down to a very fine level of detail. And it's because _we talk to each other!_ We don't just chill in the same slack channel and code independently, we all value each others brains and want each others inputs because we know it will improve our product and we value what perspectives others will bring.

      Trying to learn via PR is a sad substitute for real collaboration and teamwork.

      1. 20k · · focus · HN ↗
        This is wild to read, I always review PRs and frequently find bugs or significant problems in them that get them bounced back
        1. onion2k · · focus · HN ↗
          That's a strong signal that your team isn't doing well. Significant problems should have been spotted at a software design stage, or raised in standups, or identified in a pairing session. The earlier you can find an issue the simpler it is to fix, so waiting until the last moment (e.g. PR) means you're spending far more time fixing issues than necessary.

          As for finding bugs, what happens if you miss them? Do they go out to production and potentially lose user data? Finding bugs in PR is a big problem. For a start it shows your automated tests aren't good enough, and secondly it shows the devs aren't checking their code works well enough.

          If you do them at all, PRs should be a gate for checking whether the code meets the team's quality bar, not if it even works. The team should be able to deliver working code without them.

          1. virgilp · · focus · HN ↗
            > The earlier you can find an issue the simpler it is to fix, so waiting until the last moment (e.g. PR) means you're spending far more time fixing issues than necessary.

            Wait, does that mean that E2E tests that catch bugs are a strong signal that the team isn't doing well? How about component tests that catch bugs? Wouldn't they be better caught at the unit-test level? Do you see how your argument is flawed? - nobody is "waiting until PR to catch all bugs" but that doesn't mean that PR review can't/ shouldn't catch bugs! Sometimes even significant ones, yes.

            You don't eliminate E2E tests because "you have good unit tests". You shouldn't just eliminate PR reviews because "we communicate inside the team".

            1. onion2k · · focus · HN ↗
              does that mean that E2E tests that catch bugs are a strong signal that the team isn't doing well?

              Yes. E2E tests are there to give you confidence that future changes haven't broken things. They're not there to catch bugs before the feature goes to production. Unit and integration tests should do that though.

              You shouldn't just eliminate PR reviews because "we communicate inside the team".

              You should eliminate them as soon as they're not giving you any real value, but if you don't eliminate them before that team's will stop trying to get that value in a better way because they believe the PR process catches bugs. It doesn't though, so all it really achieves is stopping the team trying to improve.

              1. virgilp · · focus · HN ↗
                > are there to give you confidence that future changes haven't broken things.

                > They're not there to catch bugs

                What's the difference between those 2 things? I genuinely can't tell. Are you saying "E2E tests are only useful if they are *always* green"? No - of course they'd ideally be always green - but it they would be guaranteed to be green, only then they would become completely useless.

                It's like saying "ideally you shouldn't write code with bugs in the first place". Sure. Ideally we shouldn't. Now back in the real world...

Open on Hacker News to reply ↗

Unofficial Hacker News client; not affiliated with Y Combinator.