• Kissaki@programming.dev
    link
    fedilink
    English
    arrow-up
    8
    ·
    1 day ago

    So my main point is to stop asking whether and how code review in its current form can be saved, but to have an open conversation about what we are trying to achieve here and the costs and benefits of alternative models.

    What would these other models be? The article assumes LLM-generated code leads to the loss of current review processes, which they call ‘modern reviews’.

    I feel like my team does and will continue to do reviews for the same reasons and with the same gains as before. LLM may generate code, but I expect my colleagues to take ownership of whatever they produce, to understand it, take responsibility, and describe it. (Which is an issue/gap for some, but that was an issue even before generating code.)

  • Ephera@lemmy.ml
    link
    fedilink
    arrow-up
    5
    ·
    1 day ago

    I feel like a big problem is that LLMs only produce previously learned logical patterns, they don’t do logical reasoning of their own. And code review was always bad at catching logic bugs as well.

    As a result, even with us diligently doing code reviews at $DAYJOB, we have more logic bugs than I have ever seen in any codebase before.

    I’ll spend three weeks discussing an existing feature with other devs and fixing up logic bugs before we realize that the entire feature is conceptually illogical and needs to be removed/replaced.
    This has happened twice for me in the past three months, which should tell you a lot about how much time is wasted here.

    TL;DR: I feel like we need something different than code review to catch these logic bugs. Maybe more of a presentation of the feature implementation to the other devs, and then a discussion of that.

  • OnLawn@programming.dev
    link
    fedilink
    arrow-up
    2
    ·
    1 day ago

    Nice article, thanks. I like how the writer came up with an economic metric for understanding code review in terms of dollars in proportion to the volume of code.

    That is the right way to discuss this with upper management.

    The other barrier is the illusion of objectivity. In the other comments, and in many programmers experience with AI, there are code short cuts and issues that are objective in that no one would disagree with their value and importance, but they are subjective in that they cannot be described so cleanly that an LLM or human can’t misunderstand them. That it can be described so perfectly as to be findable 100% with no false positives or missed positives.

    And then on top of that are the purely subjective goals like ‘simplicity’. Give programmers the goal of simplicity and they wind up simply pushing the complexity onto someone else’s desk. LLMs (subagents, etc…) are even worse at that.

    And it goes all the way up and down, the more you elevate or granulate to architectural decisions or algorithmic decisions, different illusions appear.

    Code Review has never really been 100% successful, nor has Quality Control/assurance, Testing, or any other measure. At best it just forces people to look at code with another perspective to hopefully distill myth from reality. And that is itself valuable, if not for being able to show due diligence and effort.

    Which brings us back to the article’s equation of value. Given production, liability, are calculated in business terms from the real dollars spent or projected to be spent holds up, its a good start, no?

  • Derg@programming.dev
    link
    fedilink
    arrow-up
    11
    ·
    2 days ago

    Don’t forget the value of actually knowing your codebase if you wrote it… or the value lost if you just have an LLM do the whole thing.

  • farmgineer@nord.pub
    link
    fedilink
    English
    arrow-up
    5
    ·
    2 days ago

    I see all kinds of shit code come out of peoples’ coding agents. Thankfully, we are under no pressure to remove human reviews.

  • vrek@programming.dev
    link
    fedilink
    English
    arrow-up
    2
    ·
    2 days ago

    In my experience code review is often about the logic rather than the code itself. For example do you really need to copy the whole user object just to read their username? Can’t you just pass the username in as a string instead? Static validation and test suites should already of eliminated any code errors. If you show up to a code review and the code doesn’t compile, you’re going to have a bad time. Stuff like calculating the same distance between two objects multiple times should be called out in code review, maybe extract that into a function. Reaching out to a database to get the same value repeatedly should be called out, maybe store it as a local variable.

    One time I was reviewing some code which interfaces with an external third party piece of hardware which the api required the serial number for licensing. The programmer hard coded the serial number into the code. If we swapped it due to hardware malfunction we would need to update the software. If we got a second one we would need to maintain two code bases. This should be in a configuration file or something. Ai will not catch that. Static testing won’t catch this. Test suites running on the piece of hardware we had, wouldn’t catch this. Only a human reviewer would.

    • MonkderVierte@lemmy.zip
      link
      fedilink
      arrow-up
      1
      ·
      1 day ago

      Ai will not catch that. Static testing won’t catch this. Test suites running on the piece of hardware we had, wouldn’t catch this. Only a human reviewer would.

      This explains a lot of lsecurity issues in the last few years.

      Also, can’t compilers catch that and complain?

      • vrek@programming.dev
        link
        fedilink
        English
        arrow-up
        1
        ·
        1 day ago

        Well basically it’s just a ‘magic number’. If you ban all of those many programs will either break or at minimum suffer sever performance degradation.

        • MonkderVierte@lemmy.zip
          link
          fedilink
          arrow-up
          1
          ·
          13 hours ago

          Not ban, warn about fixed string assigned to variable. Ok, guess that would be hard to distinguish on compiler level.