← Back to context

Comment by shagie

7 hours ago

In some code that I was working on, I had

    // stuff
    obj.setSomeData(something);
    // fifteen lines of other code
    obj.setSomeData(something);
    // more stuff

The 'something' was a little bit more complex, but it was the same something with slightly different formatting.

My linter didn't catch the repeat call. When asking the AI chat for a review of the code changes it did correctly flag that there was a repeat call.

It also caught a repeat call in

    List<Objs> objs = someList.stream().filter(o -> o.field.isPresent()).toList();
    
    // ...

    var something = someFunc(objs);

    Thingy someFunc(List<Objs> param) {
        return param.stream().filter(o -> o.field.isPresent()). ...

Where one of the filter calls is unnecessary... and it caught that across a call boundary.

So, I'd say that AI code reviews are better than a linter. There's still things that it fusses about because it doesn't know the full context of the application and the tables that make certain guarantees about the data, or code conventions for the team (in particular the use of internal terms within naming conventions).

I had a similar review by AI except my equivalent of setSomeData was stateful and needed to be there in both places, the AI just didn't understand any of it.

  • When this happens to me it makes me question my design.

    If the AI doesn’t understand it, chances are it’s counter-intuitive. Of course not all LLM’s are equal, etc, etc.

    • Then again, I have a rough idea on how I could implement this check with some (language-dependent) accuracy in a linter. With LLM's I... just hope and pray?

    • I'd agree with that but in the JS world, there's a lot of questionable library designs that are outside of my control.

  • My reaction in that case is that most other readers of the codebase would probably also assume this, and so it should be either made clearer that it's stateful, or it should be refactored to not be stateful

Why isn’t `obj` immutable?

  • Because 'obj' is an object that was generated by a json schema and pulled in as a dependency. The pojo generator was not set up to create immutable objects.

Unit tests catch that kind of stuff