5 ms·
In some code that I was working on, I had // stuff obj.setSomeData(something); // fifteen lines of other code obj.setSomeData(something); /
by shagie 9mo 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).
- uoaei 9mo agoI'd say I see one anecdote, nothing to draw conclusions from.
- realusername 9mo agoI 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 9mo agoWhen 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 9mo agoThen 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 9mo agoI'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 9mo agoMy 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
- tayo42 9mo agoUnit tests catch that kind of stuff
- anthonypasq96 9mo agoyoure verifying std lib function call counts in unit tests? lmao.
- tayo42 9mo agoYou 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 9mo agoi 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
- deleted 9mo ago[deleted]
- noitpmeder 9mo agoYou're not verifying the observable behavior of your application? lmao
- anthonypasq96 9mo agowhat happened to not testing implementation details?
- quietbritishjim 9mo agoA 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.
- fcarraldo 9mo agoWhy isn’t `obj` immutable?
- shagie 9mo agoBecause '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.