‹ BackHN Continuity

Thread

There is more to code review than (automatable) detection

165 points · 117 comments · utiiiD

  1. n4r9 · · focus · HN ↗
    There&#x27;s been a lot of talk about the purpose of code review recently. It makes sense in the face of AI. Heres a link that was submitted a little while ago: <a href="https:&#x2F;&#x2F;mathstodon.xyz&#x2F;@mjd&#x2F;115096720350507897" rel="nofollow">https:&#x2F;&#x2F;mathstodon.xyz&#x2F;@mjd&#x2F;115096720350507897

    And in response I wrote a non-exhaustive checklist of things that a code review can look for:

    - Does it functionally achieve what it sets out to (as per tacker issue or PR description)?

    - Does it have extraneous code? Leftover debug prints, private API keys etc...

    - Does it have any obvious defects? Memory leaks, un-handled edge cases, security flaws, obsolete API calls, etc...

    - Could it be more understandable? Add&#x2F;remove abstractions, better variable&#x2F;method names, more&#x2F;less functional etc...

    - Is the style consistent with the codebase and&#x2F;or style guidelines?

    - Are there obvious performance improvements? Hashset instead of list, lazy evaluations, etc...

    - Is it sufficiently well tested?

    I think LLMs are okay at most of these, and worst at the first.

    1. anarazel · · focus · HN ↗
      - Do we want this? Cost&#x2F;Benefit etc

      - Is the change architecturally right?

      Particularly the latter LLMs seem still pretty useless at.

      1. birdatlaw · · focus · HN ↗
        The former feels more like a product leadership problem.

        Although I do think that LLMs have made it much easier to justify writing low-value code which can make this more common now.

        1. goda90 · · focus · HN ↗
          The &quot;do we want this&quot; question can also apply to functionality, not just cost&#x2F;benefit. I&#x27;ve seen AI volunteer &quot;features&quot; that aren&#x27;t actually useful or are actively harmful to user experience but it generates them because they do make sense in different contexts.
Open on Hacker News to reply ↗

Unofficial Hacker News client; not affiliated with Y Combinator.