6 ms·

> but the signal to noise ratio is poor Nail on the head. Every time I've seen it applied, its awful at this. However this is the one thing I loathe in human r

> but the signal to noise ratio is poor Nail on the head. Every time I've seen it applied, its awful at this. However this is the one thing I loathe in human reviews as well, where people are leaving twenty comments about naming and then the actual FUNCTIONAL issue is just inside all of that mess. A good code reviewer knows how to just drop all the things that irk them and hyperfocus on what matters, if there's a functional issue with the code. I wonder if AI is ever gonna be able to conquer that one as its quite nuanced. If they do though, then I feel the industry as it is today, is kinda toast for a lot of developers, because outside of agency, this is the one thing we were sorta holding out on being not very automatable.

zenolijo 9mo ago
Naming comments can be very useful in code that gets read by a lot of people. It can make the process of understanding the code much quicker. On the other hand, if it's less important code or the renaming is not clearly an improvement it can be quite useless. But I've met some developers who has the opinion of reviews as pointless and just say "this works, just approve it already" which can be very frustrating when it's a codebase with a lot of collaboration.
- Quarrelsome 9mo ago
  > Naming comments can be very useful in code that gets read by a lot of people. It can make the process of understanding the code much quicker. yes but it can be severely diminishing returns. Like lets step back a second and ask ourselves if: var itemCount = items.Count; vs var numberOfItems = items.Count; is ever worth spending the time discussing, versus how much of a soft improvement it makes to the code base. I've literally been in a meeting room with three other senior engineers killing 30 minutes discussing this and I just think that's a complete waste of time. They're not wrong, the latter is clearer, but if you have a PR that improves the repo and you're holding it back because of something like this, then I don't think you have your priorities straight.
  - spooky_action 9mo ago
    Sorry for the dumb question, is the second version actually better than the first? Because I prefer the first. But perhaps you chose this as a particularly annoying/unuseful comment
    - ambicapter 9mo ago
      They’re both equally bad to me, I don’t see the improvement over just using item.count. I may be nitpicking a toy example though.
      - Quarrelsome 9mo ago
        I think in this case itemCount had application in a couple of conditions later in the function, so there was value in extracting the count. In my recollection I might be missing some nuance, lets say for the sake of argument it was: var relevantCount = items.Where(x => x.SomeValue > 5); vs var numberOfRelevantItems = items.Where(x => x.SomeValue > 5); so it wasn't necessarily cheap enough to want to repeat.
        Izkata 9mo ago
        Almost. I think you're reflexively doing the same thing GP was questioning (which I agree with; in the original example the new variable was just straight duplication of knowledge and is as likely to be the source of bugs as anything else (like if items were added or removed, it's now out of date)). Here though you're missing the ".Count", so var relevantItems = items.Where(x => x.SomeValue > 5); relevantItems.Count As long as it's not a property that's calculated every time it's accessed, this still seems better than pulling the count into its own variable to me.
    - Quarrelsome 9mo ago
      I personally don't give a shit either way but I've worked in dev shops with a clear preference for the second one. I can see their point because the code as natural language parses better but I don't think its strong enough to care about. Sort of place that is fussy about test naming so where I would do smth like: TestSearchCriteriaWhere they'd want Test_That_Where_Clauses_In_Search_Criteria_Work I think its a waste of typing but idk, I'm willing to let it slide because I think its a pointless hill to die on.
- daotoad 9mo ago
  Naming comments are useful when someone catches something like: 1. you are violating a previously agreed upon standard for naming things 2. inconsistent naming, eg some places you use "catalog ID" and other places you use "item ID" (using separate words and spaces here because case is irrelevant). 3. the name you chose makes it easy to conflate two or more concepts in your system 4. the name you chose calls into question whether you correctly understood the problem domain you are addressing I'm sure there are other good naming comments, but this is a reasonable representation of the kinds of things a good comment will address. However, most naming comments are just bike shedding.
  - Arainach 9mo ago
    If the person reading the code doesn't quickly understand what's going on from the name or finds the name confusing, the name is poor and should be changed. It is way too easy for the author to be caught up in their mental model and to be unaware of their implicit assumptions and context and choose a name that doesn't make sense. The bigger problem is people who feel ownership of shared codebases tied to their ego and who get angry when people suggest changes to names and other bits of interfaces instead of just making the suggested change. If you get code review feedback, the default answer is "Done" unless you have a strong reason not to. If it's not obvious whether the name suggested by the author or the reader is better, the reader's choice should be taken every time.
    - layer8 9mo ago
      > If the person reading the code doesn't quickly understand what's going on from the name or finds the name confusing, the name is poor and should be changed. I used to think that way, but in many nontrivial circumstances, every conceivable name will be a mismatch for where some person is coming from, and not be self-evident for their mental model. Even the same person, over a longer time span. There is often a gap to bridge from name to meaning, and a comment isn’t the worst way to bridge it.
      - hinkley 9mo ago
        I find the thesaurus helps a lot with this. Actually more than just naming, because often a word in the synonym list will stand out as a more accurate representation of the concept you’re trying to add to the code, in a way that reveals subtasks that will substantially increase the value of the feature. In short I use it as a form of rubber ducking. No it’s not like this word, it’s more like that one, but most of all like this one.
- SchemaLoad 9mo ago
  A lot of these comments are not pointing out actual issues, just "That's not how I would have done it" type comments.
  - brabel 9mo ago
    And the most amazing part is that we got a mini PR review in the comments to a single line of code someone posted just to show an example of useless debates :D
causalscience 9mo ago
Yeah or worse like my boss. We don't have a style guide. But he always wants style changes in every PR, and those style changes are some times contradictory across different PRs. Eventually I've told him "if your comment does not affect performance or business logic, I'm ignoring it". He finally got the message. The fact that he accepted this tells me that deep down he knew his comments were just bike shedding.
- zeroCalories 9mo ago
  You should have a style guide, or adopt one. Having uniform code is incredibly valuable as it greatly reduces the cognitive load of reading it. Same reason that Go's verbose "err != nil" works so well.
  - brabel 9mo ago
    Style guidelines should be enforced automatically. Leaving that for humans to verify is a recipe for conflict and frustration.
    - zeroCalories 9mo ago
      Ideally yes, but there's plenty of cases where that's not desirable or possible. For example, most people would agree you should use exhaustive checks when possible(matching in rust, records in typescript, etc.). But how do you enforce that? Ban other types of control flow? But even if you find a good balanced way to enforce it, you won't always want to enforce it. There's plenty of good use cases where you explicitly don't want a check to be exhaustive. At which point now you gotta make sure there's an escape mechanism to whatever crackhead check you've setup. Better to just leave a comment with a link to your style guide explaining why this is done. Many experienced developers that are new to rust or typescript simply never think of things like this, so it's worthwhile to document it.
- awesome_dude 9mo ago
  I've been in teams like this - people who are lower on the chain of power get run in circles as they change to appease one, then change to appease another then change to go back to appease the first again. Then, going through their code, they make excuses about their code not meeting the same standards they demand. As the other responder recommends, a style guide is ideal, you can even create an unofficial one and point to it when conflicting style requests are made
xmprt 9mo ago
Human comments tend to be short and sweet like "nit: rename creatorOfWidgets to widgetFactory". Whereas AI code review comments are long winded not as precise. So even if there are 20 humans comments, I can easily see which are important and which aren't.
- Quarrelsome 9mo ago
  it "nit" short for nitpick? I think prefixing PR comments with prefixes like that is very helpful for dealing with this problem.
  - simonbw 9mo ago
    Yes it is. I've really oijed those convention at places I've worked. It probably wouldn't be too hard to instruct AI's to use this format too.
  - hdjrudni 9mo ago
    Yes, but I don't know how effective it is. 99% of the time someone leaves a 'nit' the other person fixes it. So we're still dealing with most of them like regular comments. Only once or twice I've been like "nah, I like my way better" but I can only do that if they also leave an LGTM. Sometimes they do. There's one or two people that will hold your code hostage until you reply to every little nit. At that point they don't feel like nits. I always LGTM if the code is functionally correct or if the build breaks in a trivial way (that would also block them from submitting). Then they can address my nits or submit anyway and I'm cool with that.
    - wtetzner 9mo ago
      I wonder if there's a psychological benefit though. If someone states up front that they know something is just a nitpick, the author might be less likely to push back, and therefore it's less likely to end up in a bike shedding back-and-forth.
      - scott_w 9mo ago
eieio 9mo ago
at my last job code review was done directly in your editor (with tooling to show you diffs as well). What this meant was that instead of leaving nitpicky comments, people would just change things that were nitpicky but clear improvements. They'd only leave comments (which blocked release) for stuff that was interesting enough to discuss. This was typically a big shock for new hires who were used to the "comment for every nitpick" system; I think it can feel insulting when someone changes your feature. But I quickly came to love it and can't imagine doing code review any other way now. It's so much faster! I'm not sure how to tie this to AI code review tbh. Right now I don't think I'd trust a model's taste for when to change things and when to leave a comment. But maybe that'll change. I agree that if you automated away my taste for code it'd put me in a weird spot!
- Phlebsy 9mo ago
  > What this meant was that instead of leaving nitpicky comments, people would just change things that were nitpicky but clear improvements. They'd only leave comments (which blocked release) for stuff that was interesting enough to discuss. This is my dream; have only had a team with little enough ego to actually achieve it once for an unfortunately short period of time. If it's something that there's a 99% chance the other person is going to say 'oh yeah, duh' or 'sure, whatever' then it's just wasting both of your time to not just do it. That said, I've had people get upset over merging their changes for them after a LGTM approval when I also find letting it sit to be a meaningless waste of time.
- johntash 9mo ago
  What if you have two people with different ideas of how to name a certain variable and they just flip the name back and forth every release? I like this review method too though, and like that some pr review tools have a 'suggest changes' and 'apply changes' button now too
  - ljm 9mo ago
    I think it's a good idea to have a style guide of sorts that you can point to when people sweat the small stuff.
tr_user 9mo ago
Depends on what you're targeting - If it's a rough PR, you're looking for feedback on direction rather than nitpicks. - If it's in a polished state, it's good to nitpick assuming you have a style guide you're intending to adhere to. Perhaps this can be provided in the system prompt?
knes 9mo ago
At augment code we specifically build our code review tool to find noise to signal ratio problem. In benchmark our comments are 2 to 3x more likely to get fixed compared to bugbot coderabbit etc You should check it at Augmentcode.com
Cthulhu_ 9mo ago
This is why you should set guidelines for reviews (like e.g. https://go.dev/wiki/CodeReviewComments https://go.dev/wiki/CodeReviewComments), and ideally automate as much as possible. I'm guilty of this as well, leaving loads of nitpicky code style comments - but granted, this was before Prettier was a thing. In hindsight, I could've spent all that time building a code formatter myself lol.
catlifeonmars 9mo ago
Let me throw something out there: poor naming obscures and distracts from functional issues. You are right about a good reviewer, but a good author strives for clarity in addition to correctness. As an aside, naming is highly subjective. Like in writing, you tailor naming to the problem domain and the audience.
renegade-otter 9mo ago
If you are nitpicking style or conventions that do not have rules in your linting tools, then those should automatically be non-issues, IMO.
jolt42 9mo ago
Don't get me started on not CamelCasing acronyms, acronyms aren't more important than regular words! :)
sotix 9mo ago
I follow a consistent comment pattern[0] that makes blocking vs non-blocking easy to identify. [0]: https://conventionalcomments.org/ https://conventionalcomments.org/
hinkley 9mo ago
You can do both. The noise is often what hides the bug in the first place. Aim for more straightforward code and the bug will often surface. For a while when Node was switching to async from promise chains, people would bring me code or tests that were misfiring and they couldn’t tell why. Often it was because of either a bug in the promise chaining or someone tried to graft async code into the chain and it was an impedance mismatch. I would start them by asking them to make the function fully async and then come back. About half the time the code just fixed itself. Because the intention of the code was correct, but there was some subtle bookkeeping or concurrency issue that was obfuscated. About a quarter of the time the bug popped out and the dev fixed it. And about a quarter of the time there was a legitimate bug that had been sleeping. The function was fine ones own, but something it called was broken. There are a lot of situations like that out there. The code distracts from the real problem, but just fixing the “real problem” is a disservice because another real problem will happen later. Make the change easy. That’s always the middle of the solution.

zenolijo 9mo ago

Naming comments can be very useful in code that gets read by a lot of people. It can make the process of understanding the code much quicker. On the other hand, if it's less important code or the renaming is not clearly an improvement it can be quite useless. But I've met some developers who has the opinion of reviews as pointless and just say "this works, just approve it already" which can be very frustrating when it's a codebase with a lot of collaboration.

Quarrelsome 9mo ago
> Naming comments can be very useful in code that gets read by a lot of people. It can make the process of understanding the code much quicker. yes but it can be severely diminishing returns. Like lets step back a second and ask ourselves if: var itemCount = items.Count; vs var numberOfItems = items.Count; is ever worth spending the time discussing, versus how much of a soft improvement it makes to the code base. I've literally been in a meeting room with three other senior engineers killing 30 minutes discussing this and I just think that's a complete waste of time. They're not wrong, the latter is clearer, but if you have a PR that improves the repo and you're holding it back because of something like this, then I don't think you have your priorities straight.
- spooky_action 9mo ago
  Sorry for the dumb question, is the second version actually better than the first? Because I prefer the first. But perhaps you chose this as a particularly annoying/unuseful comment
  - ambicapter 9mo ago
    They’re both equally bad to me, I don’t see the improvement over just using item.count. I may be nitpicking a toy example though.
    - Quarrelsome 9mo ago
      I think in this case itemCount had application in a couple of conditions later in the function, so there was value in extracting the count. In my recollection I might be missing some nuance, lets say for the sake of argument it was: var relevantCount = items.Where(x => x.SomeValue > 5); vs var numberOfRelevantItems = items.Where(x => x.SomeValue > 5); so it wasn't necessarily cheap enough to want to repeat.
      - Izkata 9mo ago
        Almost. I think you're reflexively doing the same thing GP was questioning (which I agree with; in the original example the new variable was just straight duplication of knowledge and is as likely to be the source of bugs as anything else (like if items were added or removed, it's now out of date)). Here though you're missing the ".Count", so var relevantItems = items.Where(x => x.SomeValue > 5); relevantItems.Count As long as it's not a property that's calculated every time it's accessed, this still seems better than pulling the count into its own variable to me.
  - Quarrelsome 9mo ago
    I personally don't give a shit either way but I've worked in dev shops with a clear preference for the second one. I can see their point because the code as natural language parses better but I don't think its strong enough to care about. Sort of place that is fussy about test naming so where I would do smth like: TestSearchCriteriaWhere they'd want Test_That_Where_Clauses_In_Search_Criteria_Work I think its a waste of typing but idk, I'm willing to let it slide because I think its a pointless hill to die on.
daotoad 9mo ago
Naming comments are useful when someone catches something like: 1. you are violating a previously agreed upon standard for naming things 2. inconsistent naming, eg some places you use "catalog ID" and other places you use "item ID" (using separate words and spaces here because case is irrelevant). 3. the name you chose makes it easy to conflate two or more concepts in your system 4. the name you chose calls into question whether you correctly understood the problem domain you are addressing I'm sure there are other good naming comments, but this is a reasonable representation of the kinds of things a good comment will address. However, most naming comments are just bike shedding.
- Arainach 9mo ago
  If the person reading the code doesn't quickly understand what's going on from the name or finds the name confusing, the name is poor and should be changed. It is way too easy for the author to be caught up in their mental model and to be unaware of their implicit assumptions and context and choose a name that doesn't make sense. The bigger problem is people who feel ownership of shared codebases tied to their ego and who get angry when people suggest changes to names and other bits of interfaces instead of just making the suggested change. If you get code review feedback, the default answer is "Done" unless you have a strong reason not to. If it's not obvious whether the name suggested by the author or the reader is better, the reader's choice should be taken every time.
  - layer8 9mo ago
    > If the person reading the code doesn't quickly understand what's going on from the name or finds the name confusing, the name is poor and should be changed. I used to think that way, but in many nontrivial circumstances, every conceivable name will be a mismatch for where some person is coming from, and not be self-evident for their mental model. Even the same person, over a longer time span. There is often a gap to bridge from name to meaning, and a comment isn’t the worst way to bridge it.
    - hinkley 9mo ago
      I find the thesaurus helps a lot with this. Actually more than just naming, because often a word in the synonym list will stand out as a more accurate representation of the concept you’re trying to add to the code, in a way that reveals subtasks that will substantially increase the value of the feature. In short I use it as a form of rubber ducking. No it’s not like this word, it’s more like that one, but most of all like this one.
SchemaLoad 9mo ago
A lot of these comments are not pointing out actual issues, just "That's not how I would have done it" type comments.
- brabel 9mo ago
  And the most amazing part is that we got a mini PR review in the comments to a single line of code someone posted just to show an example of useless debates :D

causalscience 9mo ago

Yeah or worse like my boss. We don't have a style guide. But he always wants style changes in every PR, and those style changes are some times contradictory across different PRs. Eventually I've told him "if your comment does not affect performance or business logic, I'm ignoring it". He finally got the message. The fact that he accepted this tells me that deep down he knew his comments were just bike shedding.

zeroCalories 9mo ago
You should have a style guide, or adopt one. Having uniform code is incredibly valuable as it greatly reduces the cognitive load of reading it. Same reason that Go's verbose "err != nil" works so well.
- brabel 9mo ago
  Style guidelines should be enforced automatically. Leaving that for humans to verify is a recipe for conflict and frustration.
  - zeroCalories 9mo ago
    Ideally yes, but there's plenty of cases where that's not desirable or possible. For example, most people would agree you should use exhaustive checks when possible(matching in rust, records in typescript, etc.). But how do you enforce that? Ban other types of control flow? But even if you find a good balanced way to enforce it, you won't always want to enforce it. There's plenty of good use cases where you explicitly don't want a check to be exhaustive. At which point now you gotta make sure there's an escape mechanism to whatever crackhead check you've setup. Better to just leave a comment with a link to your style guide explaining why this is done. Many experienced developers that are new to rust or typescript simply never think of things like this, so it's worthwhile to document it.
awesome_dude 9mo ago
I've been in teams like this - people who are lower on the chain of power get run in circles as they change to appease one, then change to appease another then change to go back to appease the first again. Then, going through their code, they make excuses about their code not meeting the same standards they demand. As the other responder recommends, a style guide is ideal, you can even create an unofficial one and point to it when conflicting style requests are made

xmprt 9mo ago

Human comments tend to be short and sweet like "nit: rename creatorOfWidgets to widgetFactory". Whereas AI code review comments are long winded not as precise. So even if there are 20 humans comments, I can easily see which are important and which aren't.

Quarrelsome 9mo ago
it "nit" short for nitpick? I think prefixing PR comments with prefixes like that is very helpful for dealing with this problem.
- simonbw 9mo ago
  Yes it is. I've really oijed those convention at places I've worked. It probably wouldn't be too hard to instruct AI's to use this format too.
- hdjrudni 9mo ago
  Yes, but I don't know how effective it is. 99% of the time someone leaves a 'nit' the other person fixes it. So we're still dealing with most of them like regular comments. Only once or twice I've been like "nah, I like my way better" but I can only do that if they also leave an LGTM. Sometimes they do. There's one or two people that will hold your code hostage until you reply to every little nit. At that point they don't feel like nits. I always LGTM if the code is functionally correct or if the build breaks in a trivial way (that would also block them from submitting). Then they can address my nits or submit anyway and I'm cool with that.
  - wtetzner 9mo ago
    I wonder if there's a psychological benefit though. If someone states up front that they know something is just a nitpick, the author might be less likely to push back, and therefore it's less likely to end up in a bike shedding back-and-forth.
    - scott_w 9mo ago

eieio 9mo ago

at my last job code review was done directly in your editor (with tooling to show you diffs as well). What this meant was that instead of leaving nitpicky comments, people would just change things that were nitpicky but clear improvements. They'd only leave comments (which blocked release) for stuff that was interesting enough to discuss. This was typically a big shock for new hires who were used to the "comment for every nitpick" system; I think it can feel insulting when someone changes your feature. But I quickly came to love it and can't imagine doing code review any other way now. It's so much faster! I'm not sure how to tie this to AI code review tbh. Right now I don't think I'd trust a model's taste for when to change things and when to leave a comment. But maybe that'll change. I agree that if you automated away my taste for code it'd put me in a weird spot!

Phlebsy 9mo ago
> What this meant was that instead of leaving nitpicky comments, people would just change things that were nitpicky but clear improvements. They'd only leave comments (which blocked release) for stuff that was interesting enough to discuss. This is my dream; have only had a team with little enough ego to actually achieve it once for an unfortunately short period of time. If it's something that there's a 99% chance the other person is going to say 'oh yeah, duh' or 'sure, whatever' then it's just wasting both of your time to not just do it. That said, I've had people get upset over merging their changes for them after a LGTM approval when I also find letting it sit to be a meaningless waste of time.
johntash 9mo ago
What if you have two people with different ideas of how to name a certain variable and they just flip the name back and forth every release? I like this review method too though, and like that some pr review tools have a 'suggest changes' and 'apply changes' button now too
- ljm 9mo ago
  I think it's a good idea to have a style guide of sorts that you can point to when people sweat the small stuff.

tr_user 9mo ago

Depends on what you're targeting - If it's a rough PR, you're looking for feedback on direction rather than nitpicks. - If it's in a polished state, it's good to nitpick assuming you have a style guide you're intending to adhere to. Perhaps this can be provided in the system prompt?

knes 9mo ago

At augment code we specifically build our code review tool to find noise to signal ratio problem. In benchmark our comments are 2 to 3x more likely to get fixed compared to bugbot coderabbit etc You should check it at Augmentcode.com

Cthulhu_ 9mo ago

This is why you should set guidelines for reviews (like e.g. https://go.dev/wiki/CodeReviewComments https://go.dev/wiki/CodeReviewComments), and ideally automate as much as possible. I'm guilty of this as well, leaving loads of nitpicky code style comments - but granted, this was before Prettier was a thing. In hindsight, I could've spent all that time building a code formatter myself lol.

catlifeonmars 9mo ago

Let me throw something out there: poor naming obscures and distracts from functional issues. You are right about a good reviewer, but a good author strives for clarity in addition to correctness. As an aside, naming is highly subjective. Like in writing, you tailor naming to the problem domain and the audience.

renegade-otter 9mo ago

If you are nitpicking style or conventions that do not have rules in your linting tools, then those should automatically be non-issues, IMO.

jolt42 9mo ago

Don't get me started on not CamelCasing acronyms, acronyms aren't more important than regular words! :)

sotix 9mo ago

I follow a consistent comment pattern[0] that makes blocking vs non-blocking easy to identify. [0]: https://conventionalcomments.org/ https://conventionalcomments.org/

hinkley 9mo ago

You can do both. The noise is often what hides the bug in the first place. Aim for more straightforward code and the bug will often surface. For a while when Node was switching to async from promise chains, people would bring me code or tests that were misfiring and they couldn’t tell why. Often it was because of either a bug in the promise chaining or someone tried to graft async code into the chain and it was an impedance mismatch. I would start them by asking them to make the function fully async and then come back. About half the time the code just fixed itself. Because the intention of the code was correct, but there was some subtle bookkeeping or concurrency issue that was obfuscated. About a quarter of the time the bug popped out and the dev fixed it. And about a quarter of the time there was a legitimate bug that had been sleeping. The function was fine ones own, but something it called was broken. There are a lot of situations like that out there. The code distracts from the real problem, but just fixing the “real problem” is a disservice because another real problem will happen later. Make the change easy. That’s always the middle of the solution.