Remix.run Logo
Remix clone Hacker News
new | show | ask | jobsGithub
▲ shagie 16 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).

▲realusername 15 hours ago | parent | next [-]

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.

▲james_marks 15 hours ago | parent | next [-]

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.

▲wolletd 15 hours ago | parent | next [-]

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?

▲realusername 11 hours ago | parent | prev [-]

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

▲frde_me 12 hours ago | parent | prev [-]

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

▲uoaei 15 hours ago | parent | prev | next [-]

I'd say I see one anecdote, nothing to draw conclusions from.

▲fcarraldo 14 hours ago | parent | prev | next [-]

Why isn’t `obj` immutable?

▲shagie 12 hours ago | parent [-]

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.

▲tayo42 15 hours ago | parent | prev [-]

Unit tests catch that kind of stuff

▲shagie 12 hours ago | parent | next [-]

The code works perfectly - there is no issue that a unit test could catch... unless you are spying on internally created objects to a method and verifying that certain functions are called some number of times for given data.

▲tayo42 11 hours ago | parent [-]

Sure and you can do that

▲shagie 9 hours ago | parent [-]

Trying to write the easiest code that I could test... I don't think I can without writing an excessively brittle test that would break at the slightest implementation change.

So you've got this Java:

    public List<Integer> someCall() {
        return IntStream.range(1,10).boxed().toList();
    }

    public List<Integer> filterEvens(List<Integer> ints) {
        return ints.stream()
                .filter(i -> i % 2 == 0)
                .toList();
    }

    int aMethod() {
        List<Integer> data = someCall();
        return filterEvens(data.stream().filter(i -> i % 2 == 0).toList()).size();
    }
And I can mock the class and return a spied'ed List. But now I've got to have that spied List return a spied stream that checks to see if .filter(i -> i % 2 == 0) was called. But then someone comes and writes it later as .filter(i -> i % 2 != 1) and the test breaks. Or someone adds another call to sort them first, and the test breaks.

To that end, I'd be very curious to see the test code that verifies that when aMethod() is called that the List returned by SomeCall is not filtered twice.

What's more, it's not a useful test - "not filtered twice" isn't something that is observable. It's an implementation detail that could change with a refactoring.

Writing a test that verifies that filterEvens returns a list that only contains even numbers? That's a useful test.

Writing a test that verifies that aMethod returns back the size of the even numbers that someCall produced? That's a useful test.

Writing a test that tries to enforce a particular implementation between the {} of aMethod? That's not useful and incredibly brittle (assuming that it can be written).

▲nl 7 hours ago | parent | next [-]

You are correct and the objection is just completely invalid. There's no way anyone would or should write tests like this at the client level.

I think they are just arguing for the sake of arguing.

▲tayo42 6 hours ago | parent | prev [-]

You mention the tools you can use to make it happen.

I think we're at the point where you need concrete examples to talk about whether it's worth it or not. If you have functions that can't be called twice, then you have no other option to test details in the implementation like that.

Yeah there's a tradeoff between torturing your code to make everything about it testable and enforce certain behavior or keeping it simpler.

I have worked in multiple code bases where every function call had asserts on how many times it was called and what the args were.

▲anthonypasq96 14 hours ago | parent | prev [-]

youre verifying std lib function call counts in unit tests? lmao.

▲tayo42 14 hours ago | parent | next [-]

You can do that with mocks if it's important that something is only called once, or likely there's some unintended side effect of calling it twice and tests woukd catch the bug

▲anthonypasq96 13 hours ago | parent [-]

i know you could do it, im asking why on earth you would feel its vital to verify stream.filter() was called twice in a function

▲ 12 hours ago | parent [-]
[deleted]
▲noitpmeder 14 hours ago | parent | prev [-]

You're not verifying the observable behavior of your application? lmao

▲shagie 12 hours ago | parent | next [-]

How would you suggest tests around:

    void func() {
        printEvens(someCall().stream().filter(n -> n % 2 == 0).toList());
    }

    void printEvens(List<Integer> nums) {
        nums.stream().filter(n -> n % 2 == 0).forEach(n -> System.out.println(n));
    }
The first filter is redundant in this example. Duplicate code checkers are checking for exactly matching lines.

I am unaware of any linter or static analyzer that would flag this.

What's more, unit tests to test the code for printEvens (there exists one) pass because they're working properly... and the unit test that calls the calling function passes because it is working properly too.

Alternatively, write the failing test for this code.

▲tayo42 11 hours ago | parent [-]

Idk how exactly to do it in cpp becasue I'm not familiar with the tooling

You could write a test that makes sure the output of someCall is passed directly to printeven without being modified.

The example as you wrote is hard to test in general. It's probably not something you would write if your serious about testing.

▲shagie 10 hours ago | parent [-]

In C++, the code would look like:

    #include <vector>
    #include <iostream>
    #include <algorithm>

    std::vector<int> someCall()
    {
        return {1, 2, 3, 4, 5, 6, 7, 8, 9, 10};
    }

    void printEvens(const std::vector<int>& nums)
    {
        std::ranges::for_each(nums, [](int n)
        {
            if (n % 2 == 0)
            {
                std::cout << n << '\n';
            }
        });
    }

    int main()
    {
        std::vector<int> data = someCall();
        std::vector<int> tmp;

        std::ranges::copy_if(data,
                             std::back_inserter(tmp),
                             [](int n) { return n % 2 == 0; }
        );
    
        printEvens(tmp);
        return 0;
    }
---

Nothing in there is wrong. There is no test that would fail short of going through the hassle of creating a new type that does some sort of introspection of its call stack to verify which function its being called in.

Likewise, identify if a linter or other static analysis tool could catch this issue.

Yes, this is a contrived example and it likely isn't idiomatic C++ (C++ isn't my 'native' language). The actual code in Java was more complex and had a lot more going on in other parts of the files. However, it should serve to show that there isn't a test for printEvens or someCall that would fail because it was filtered twice. Additionally, it should show that a linter or other static analysis wouldn't catch the problem (I would be rather impressed with one that did).

From ChatGPT a code review of the code: https://chatgpt.com/share/69780ce6-03e0-8011-a488-e9f3f8173f...

▲quietbritishjim 12 hours ago | parent | prev | next [-]

A redundant filter() isn't observable (except in execution time).

You could pick it up if you were to explicitly track whether it's being called redundantly but it'd be very hard and by the time you'd thought of doing that you'd certainly have already manually checked the code for it.

▲anthonypasq96 13 hours ago | parent | prev [-]

what happened to not testing implementation details?