19 ms·

That is absolutely true. Most companies have code review process that is totally full of nitpicking and trivial comments. I once suggested replacing that with s

That is absolutely true. Most companies have code review process that is totally full of nitpicking and trivial comments. I once suggested replacing that with static analysis tools to remove such comments and make feedback faster in our org, and was told that such code review is necessary for everyone. It helps people get promoted, makes people feel they prevented code issues, keeps the code review metrics looking good for the top level overlords who sit and monitor number of reviewer comments.

javajosh 3y ago
It depends on your team. If you have good juniors, they will relish the feedback. Of course it needs to be substantive. Often junior code will be far more complex and difficult to read than senior code, and code reviews are a chance to get that feedback across. That said, often teams don't give credit for code reviews, or take into account the (sometimes considerable) time required to write them. To some extent, tools are also to blame - I think all teams would do well to pick a system that integrates with the dominant IDE. Doing code reviews in the same space you write code just makes sense; requiring that you use a friggin' webapp is HORRIBLE.
- crop_rotation 3y ago
  The thing is, almost all the trivial comments can be easily automated. That would both give the junior dev quicker feedback before even raising the review, and save time for everyone.
  - javajosh 3y ago
    >Of course it needs to be substantive
  - morelisp 3y ago
    Junior code is not usually more complex because of bad naming conventions and inconsistent styling and related issues that tools can help with, although of course these don't help its readability overall. Rather it usually has control flow that's far too literal a translation of requirements leading to an exponential explosion of paths, insufficient and/or over-eager error handling (often both at the same time), poor representational choices leading to inefficiency or more special cases, and so on. None of these can be effectively automatically detected.
- ghqst 3y ago
  As an intern doing work in an unfamiliar framework, it was really valuable to get code review from seniors about how to better go about solving a problem structurally. Not something static analysis would've caught.
swader999 3y ago
If you can't change the Company, change your company.
- BSEdlMMldESB 3y ago
  if it is "your" company but you cannot change it, I put that it is not your company. but that in fact you are just its employee and not the other way around.
  - swader999 3y ago
    Good catch. I rephrased it more to what I intended. (Parent replied to my initial comment: "If you can't change your Company, change your Company.")
    - mikestew 3y ago
      If I am catching your meaning correctly, I would also lower-case the second use of “company” to drive home a different usage of the word. I nitpick only because it’s a clever turn of a word that deserves understanding.
      - swader999 3y ago
        Fixed!
        cratermoon 3y ago
        Did we just witness a nit-picky "code" review, right here in the comments? I think we did.
        depressedpanda 3y ago
        It wasn't nit picky as I wouldn't have understood what the author meant before the reviewers came in and suggested improvements.
avidiax 3y ago
> keeps the code review metrics looking good for the top level overlords who sit and monitor number of reviewer comments Wouldn't the top level overlords want better code review metrics? Padding your stats with comments that can be handled by static analysis makes such noisy metrics even more noisy.
- crop_rotation 3y ago
  Running SQL queries on the data warehouse and generating reports saying very few comments on reviews is much easier than bothering to look into the comments. I ran into a high level engineer making very high amounts of money who would just do such vague things all day and claim the org needed more comments.
arp242 3y ago
> replacing that with static analysis tools to remove such comments I dislike excessive usage of these kind of tools because not infrequently you end up making your code worse just to satisfy some dumb tool. The real solution is just accepting that not all code needs to be like you would have written it yourself, and asking yourself "does this comment address anything that's objectively wrong with the code?" A lot of times the answer to that will be "no".
- renegade-otter 3y ago
  It doesn't have to be excessive - it has to be reasonable. But I agree, outside of the strictly-enforced automation, just let it go and stop going "nitpick!" The less communication between devs during code reviews - the better. It's exhausting to be diplomatic to your coworkers asking them to change something. It's a minefield full of feelings and egos.
- a_cardboard_box 3y ago
  Human reviewers can also demand your code be made worse. Sometimes, the choice is making your code worse to satisfy a dumb tool (software), or making your code worse to satisfy a dumb tool (person).
  - arp242 3y ago
    At least when it's a person it's someone you can talk to, reason with, compromise with, reasonably disagree with. Can't do any of that with software.
    - kilotaras 3y ago
      Most tools have an escape hatch, see e.g. [0] [0] https://github.com/search?q=clang-format+off&type=code https://github.com/search?q=clang-format+off&type=code
  - pjmlp 3y ago
    Example, clean code architecture astronauts.
natded 3y ago
[dead]
GrinningFool 3y ago
In some environments, that is true. But the process of review can also help build shared knowledge and understanding of the changes and the code base.
- MrJohz 3y ago
  I'm increasingly of the opinion that this should be the primary value of code review. The goal is to make sure that multiple people working on a codebase have at least some idea of what's going on in every corner of it. Obviously that's never going to be perfect, and you won't understand code as well by reading it as by working with it, but at least having some sort of familiarity is invaluable. The other important benefit is catching stupid mistakes - formatting, missing code, > vs >=, etc - dumb mistakes that everyone makes. But ideally most of those mistakes are caught by types, tests, and linting, so the code review just makes sure that a different pair of eyes has sanity checked things. If code reviews become mainly about this sanity check, then it's often a sign that the types, tests, or linting are inadequate (or that someone is being too picky). The other problem is when reviews too often focus on the big problems - architectural discussions about how to implement a feature - in which case there's usually a need to have more architectural discussion before implementation starts.
  - detourdog 3y ago
    I agree with you and the person you are agreeing with. I have to add that using analysis takes none of that away. Using analysis creates a culture of analysis tools. This also can make the revealing of performance surprises a shared learning experience from the grayest beard to the greenest horn.
    - MrJohz 3y ago
      Completely agree, analysis tools go hand in hand with code review as a learning tool. Partly, they take more responsibility away from finding the obvious bugs, and partly, like you say, they help surface problems that people might not be looking out for. I'd love it if things like performance checking tools were more commonly used.
Waterluvian 3y ago
This is what I implemented prior to my team growing beyond 1 person. If linting passes, it’s valid style. If you think the style is problematic, that’s an issue with the linting and formatting and not the PR. In practice this hasn’t been a problem. We’ve made 2 or 3 linting PRs over the years.
- mtsr 3y ago
  It’s why opinionated tools like rustfmt are so good. They set and enforce a standard which is good enough that even perfectionists don’t feel like arguing them ad nauseam.
  - Waterluvian 3y ago
    Yeah! Any one opinion may not be the best for any one person, but everyone being consistent is the best for everyone.
- hereonout2 3y ago
  Introducing linting in the test stage (flake8 with python) solves so many problems. By completely removing any personal preferences and disagreements on style, the whole team just gets to hate on and berate an inanimate tool instead. We all just seem to equally dislike flake8 and move on with our lives.
xwdv 3y ago
[flagged]
ryukoposting 3y ago
I've worked in spaces where static analysis tools were run automatically on every new PR. Trust me, it's not as good as it sounds. Static analyzers are fully incapable of detecting the more nuanced mistakes, so a human touch will still be necessary. Nearly all of the "bugs" found by the static analyzer won't actually be bugs, but you'll have to "fix" them anyway because the reviewer (again, you'll still need one) will demand that all the warnings be cleared before approving your code. Build a culture that prefers succinct, non-nitpicky code reviews. Static analysis tools only give reviewers more crap to nitpick.
- NavinF 3y ago
  > Nearly all of the "bugs" found by the static analyzer won't actually be bugs Which static analyzer is this? Every tool I've used only finds bugs the are provable so the false positive rate is essential zero
  - kevin_thibedeau 3y ago
    Plenty of C/C++ static analysis tools have pedantic rules that flag correct code. Effective use of them means going through which rules you want to disable to minimize the unproductive make-work of satisfying the machine.
Macha 3y ago
Sometimes there's a real prisoner dilemma thing going on here too. As a senior reviewing a junior's PR there are things that could be better but sometimes it's just not important. Are their variable names a bit too wordy? Did they have uneven amounts of whitespace between methods? In an ideal world these are more a "feedback for next time if it becomes a pattern" type thing. But as a reviewer, someone is looking at your comments per PR as an indicator for how much guidance you're providing, or going "oh who let that get merged" so you make the comment anyway. And then as the reviewee, they're concerned that leaving comments unaddressed looks bad for their level of responsiveness to feedback, or they think the reviewer might give them bad feedback for pushing back, so they'll then go and make the changes. But now they need someone to approve the updated version and the latency cycle starts again.
- delusional 3y ago
  I always make it very clear in my comments if something is a blocker (this can't be merged before this is fixed) or a note (this would better fit the style of the existing code), and then i make it very clear in person that I do not take it as a personal affront if they don't fix my note comments. I find that this works pretty well. It has the additional positive effect of opening me up to criticism. If they understand that not all my comments are always vital or "right" I imagine it's easier to dispute anything else I say too.
  - ryanschneider 3y ago
    Good call, I use “minor:” as a comment prefix for this but just realized that I got into that habit awhile back and the team composition has changed since then so newer members don’t have that context. I wish there was something like semantic commits “fix/feat/chore” for review feedback, specifically a keyword for these little things that are more future advice than must fixes. I also try to be clear when I don’t expect to do a second review if they make the minor changes, that the green check mark carries forward. More. broadly I’ve been trying to favor “rough consensus and running code” in my PR reviews, even if I don’t fully agree with an approach I’ll give the writer the benefit of the doubt since they’ve most likely spent more time thinking about the problem than I have. Or at least that’s my hope!
Philip-J-Fry 3y ago
Nitpicking is definitely a real thing. Perhaps it's the feel that they need to find something wrong with the code. But there's also very real issues that one person thinks is a nitpick that really isn't. Perhaps because they can't see the issue with their own eyes, they don't understand the issue or they lack the ability to leave feelings aside and rethink what they've written. We've all been attached to code we've written. We probably think it's the most elegant code ever written. But sometimes you've just gotta admit you were wrong, it's not readable or it's flawed and it will be worse off for the codebase. I've also been called a nitpicker by someone more senior than me for pointing out some very real and potentially problematic race conditions in their code. To me, a race condition is a fundamental issue with what has been written and should be fixed. But to them it was acceptable because they hadn't seen it break naturally yet.
- nickjj 3y ago
  > Nitpicking is definitely a real thing. Perhaps it's the feel that they need to find something wrong with the code. You can use code formatting and linting tools to automate the low hanging fruit but there's still opportunities to offer feedback that could be considered nitpicking but IMO isn't. For example variable names, breaking up long functions, not enough test coverage which hints maybe the author didn't consider these cases or changing this type of code: if not something: # Unhappy case else: # Happy case To: if something: # Happy case else: # Unhappy case If you have 10 devs working on a project you're going to get 10 different ways of thinking. If you're working on a 10 year old code base with a million lines of code and you don't address things that could sometimes feel like nitpicking then you wind up with a lot of technical debt and it makes it harder to work on the project. I like when people comment on my code and find ways to improve it in any way shape or form. I don't care if they have 5, 10 or 15 years less experience. I treat it like a gift because I'm getting to learn something new or think about something from a perspective I haven't thought of.
  - cratermoon 3y ago
    > test coverage which hints maybe the author didn't consider these cases In my reviews, when I look at the tests, I put on a test engineer hat. Testers like to do things like give an empty value where one is supposed to be required by the business logic, a negative or other out-of-range value, an emoji or other wonky unicode in text. Just basic stuff to a test engineer, but lots of programmers only write tests for the expected happy path, and maybe one or two cases for common alternate paths. The most common failures I find are in handling dates and date comparisons. I've been wanting to add more fuzz testing to the tests I write, but I haven't quite gotten my head around how to do it effectively.
deleted 3y ago
[deleted]
pjmlp 3y ago
I have given up on review process. Just implement whatever clean code guidelines everyone is keen in having, with their dependency injection guidelines of an interface for every single class, and move on with the actual work instead of burning discussion cycles in pull request reviews.
RHSeeger 3y ago
Static analysis tools and peer reviews can catch different types of issues, though. Just like a statically compiled language can catch some bugs that a dynamic language would not; but not everything. I'm a big fan of peer reviews, and I tend to focus my comments on - This won't do what you appear to be expecting it to - This will prevent <some other feature we're likely to need> from being implemented; at least without a much higher cost - This will work, but is very hard to understand; it will negatively impact maintenance. Consider doing it this other way, or adding an explanation about what's going on - This code is ok, but might read/work better <this other way>. I'm not failing the review because of it, but it's worth keeping in mind for future code.
Tainnor 3y ago
Many code bases that I've worked on suffered from too much, and badly engineered, code - not from not having enough code. Yes, you can go to extremes, and some people are annoyingly nitpicky when it doesn't matter. But generally, code is often a liability, stuff that is hard to understand (or even potentially subtly wrong) will cause issues down the line, and so on. Code review helps mitigate these issues, something which I think is one of the few empirical results we actually have. Plus, it also spreads knowledge about the code base. The submission is in any case not pointing out that code review itself is bad (or even that having to adapt to a new code style is the problem), but rather the whole bureaucracy around it. Code reviews are good when the turnaround is swift.
- ReaLNero 3y ago
  > Many code bases that I've worked on suffered from too much, and badly engineered, code - not from not having enough code. Because the projects that weren't built fast enough were eventually thrown away, meaning they never require maintenance, which is the source of the sampling bias you are observing. I've seen it happen in some cases (engineer with the wrong personality leading an R&D project).
  - Tainnor 3y ago
    But not every code base is in an early startup phase. Once the company is stable there isn't really any excuse to knock out underengineered garbage just to get something out of the door.
Forricide 3y ago
I _kind of_ understand in the sense that - it can feel bad/like wasted time to do code review when you don't leave a single comment. But of course, it's very very important to ignore that feeling and do the sensible thing, which is be happy that the code review is smooth and the code can be merged. I can't imagine the existential dread of realizing that half your job is pointing out the same common errors every single time. Tools like clang-tidy, clang-format, etc are so vital for staying sane.
- lrem 3y ago
  I’ve reached the point where my reviews tend to be either instant LGTM, or “please formalise the idea behind this into a design doc and submit for review to the TL forum” (I.e. a polite “I think you don’t know what you’re doing”). The usual “how about ${BETTER_NAME}” can be left optional, there’s very rarely anything actually bad by the time people submit for review. Is this a rare culture?
  - Forricide 3y ago
    This definitely isn't my experience, but I work in C++, where having a second eye to look over things is... valuable. That said, I think culture for this kind of thing varies wildly between different companies, and between teams in those companies; many (most?) teams are comprised of small numbers of people, so even one team/technical lead having a preference for a certain style could change things, and... well, everyone's different.
PartiallyTyped 3y ago
I have been told that we need 2 people to do code review, in a team of 3 people, and we can just “ask a member from another team who works on the same product”. We were told that outsiders can act as external bar raisers who will comment on “overall flow and code quality”. Sounds like glorified and highly opinionated linters whom will just gatekeep progress because they “don’t get it”.
AdrianB1 3y ago
I am doing code reviews for a large manufacturing company with dozens of plants; that code is supposed to be deployed everywhere, so the entire company is at stake. I insist on doing code review in areas where static analysis tools are completely useless, like in SQL code: without context, any SELECT can be good enough to pass static analysis, but crash in production due to performance problems. Nobody was ever promoted in my company for doing code reviews. None of the people that were ever promoted know how to do any sort or code review. I can say that the correlation between promotion and code reviews is negative.
jpollock 3y ago
This week during code reviews, I caught: 1) O(N^2) in the hottest code we have. 2) O(N^3) from someone else in the same area. 3) Filling a cache on first access - this causes the system to page (high latency!). 4) Hunting for data themselves and creating null pointer errors. 5) Hunting for data themselves and missing all the fun edge cases. 6) Duplicating a feature that already existed. I'm guessing (4) could have been caught by static analysis?
- regularfry 3y ago
  Possibly (6), depending on the nature of the duplication.
qudat 3y ago
Agreed. We don’t need human linters: https://bower.sh/human-linting https://bower.sh/human-linting
dkarl 3y ago
I agree that formatters and static analysis tools are the best way to reduce trivial PR comments. However, I would say that the kinds of things addressed by formatters and static analysis tools, and similar issues that tools don't catch such as misspellings, are worth addressing in PRs. "Nitpicking" is in the eye of the beholder, and your company might have a particular culture problem that I haven't seen, but many of the things I've seen called "nitpicking" are legitimate readability issues. For example, a field named "customers" that holds a single customer can be a major readability hurdle for someone who is looking at code for the first time. Even if you discount readers' annoyance and discomfort, which I don't think you should, a single field named "cusotmer" can become a drag on readability if the misspelling gets amplified in the codebase or if some programmers on the project aren't native English speakers. (It can be much harder to mentally gloss over language errors that aren't in your native language.) Something I learned in school is that it's extremely hard to read something you've written as another person would read it. I had an English teacher that would split the class into pairs every time we had a paper to turn in, and in each pair, you would read the other person's paper out loud to them. The results were comical, and to most students entirely unexpected. Their whole school careers, they had thought teachers were being mean and nit-picky, until they got to see their friends struggle to read what they had written. Then they started to understand that a wrong word, a misspelling, a wrong verb tense, an ambiguous pronoun, could stop a reader in their tracks or send them down the wrong path trying to understand what the writer meant. They also learned that they could not see these issues in their own writing. When they read somebody else's writing, they would stumble over errors, but in their own writing, they had to make a special effort to see them, and even then, readers would effortlessly "discover" errors that the author had missed. For many programmers, PRs are their first and only opportunity to develop this empathy for readers, and if they reject the feedback, they're going to remain forever in the dark about how other people experience their code.

javajosh 3y ago

It depends on your team. If you have good juniors, they will relish the feedback. Of course it needs to be substantive. Often junior code will be far more complex and difficult to read than senior code, and code reviews are a chance to get that feedback across. That said, often teams don't give credit for code reviews, or take into account the (sometimes considerable) time required to write them. To some extent, tools are also to blame - I think all teams would do well to pick a system that integrates with the dominant IDE. Doing code reviews in the same space you write code just makes sense; requiring that you use a friggin' webapp is HORRIBLE.

crop_rotation 3y ago
The thing is, almost all the trivial comments can be easily automated. That would both give the junior dev quicker feedback before even raising the review, and save time for everyone.
- javajosh 3y ago
  >Of course it needs to be substantive
- morelisp 3y ago
  Junior code is not usually more complex because of bad naming conventions and inconsistent styling and related issues that tools can help with, although of course these don't help its readability overall. Rather it usually has control flow that's far too literal a translation of requirements leading to an exponential explosion of paths, insufficient and/or over-eager error handling (often both at the same time), poor representational choices leading to inefficiency or more special cases, and so on. None of these can be effectively automatically detected.
ghqst 3y ago
As an intern doing work in an unfamiliar framework, it was really valuable to get code review from seniors about how to better go about solving a problem structurally. Not something static analysis would've caught.

swader999 3y ago

If you can't change the Company, change your company.

BSEdlMMldESB 3y ago
if it is "your" company but you cannot change it, I put that it is not your company. but that in fact you are just its employee and not the other way around.
- swader999 3y ago
  Good catch. I rephrased it more to what I intended. (Parent replied to my initial comment: "If you can't change your Company, change your Company.")
  - mikestew 3y ago
    If I am catching your meaning correctly, I would also lower-case the second use of “company” to drive home a different usage of the word. I nitpick only because it’s a clever turn of a word that deserves understanding.
    - swader999 3y ago
      Fixed!
      - cratermoon 3y ago
        Did we just witness a nit-picky "code" review, right here in the comments? I think we did.
        depressedpanda 3y ago
        It wasn't nit picky as I wouldn't have understood what the author meant before the reviewers came in and suggested improvements.

avidiax 3y ago

> keeps the code review metrics looking good for the top level overlords who sit and monitor number of reviewer comments Wouldn't the top level overlords want better code review metrics? Padding your stats with comments that can be handled by static analysis makes such noisy metrics even more noisy.

crop_rotation 3y ago
Running SQL queries on the data warehouse and generating reports saying very few comments on reviews is much easier than bothering to look into the comments. I ran into a high level engineer making very high amounts of money who would just do such vague things all day and claim the org needed more comments.

arp242 3y ago

> replacing that with static analysis tools to remove such comments I dislike excessive usage of these kind of tools because not infrequently you end up making your code worse just to satisfy some dumb tool. The real solution is just accepting that not all code needs to be like you would have written it yourself, and asking yourself "does this comment address anything that's objectively wrong with the code?" A lot of times the answer to that will be "no".

renegade-otter 3y ago
It doesn't have to be excessive - it has to be reasonable. But I agree, outside of the strictly-enforced automation, just let it go and stop going "nitpick!" The less communication between devs during code reviews - the better. It's exhausting to be diplomatic to your coworkers asking them to change something. It's a minefield full of feelings and egos.
a_cardboard_box 3y ago
Human reviewers can also demand your code be made worse. Sometimes, the choice is making your code worse to satisfy a dumb tool (software), or making your code worse to satisfy a dumb tool (person).
- arp242 3y ago
  At least when it's a person it's someone you can talk to, reason with, compromise with, reasonably disagree with. Can't do any of that with software.
  - kilotaras 3y ago
    Most tools have an escape hatch, see e.g. [0] [0] https://github.com/search?q=clang-format+off&type=code https://github.com/search?q=clang-format+off&type=code
- pjmlp 3y ago
  Example, clean code architecture astronauts.

natded 3y ago

[dead]

GrinningFool 3y ago

In some environments, that is true. But the process of review can also help build shared knowledge and understanding of the changes and the code base.

MrJohz 3y ago
I'm increasingly of the opinion that this should be the primary value of code review. The goal is to make sure that multiple people working on a codebase have at least some idea of what's going on in every corner of it. Obviously that's never going to be perfect, and you won't understand code as well by reading it as by working with it, but at least having some sort of familiarity is invaluable. The other important benefit is catching stupid mistakes - formatting, missing code, > vs >=, etc - dumb mistakes that everyone makes. But ideally most of those mistakes are caught by types, tests, and linting, so the code review just makes sure that a different pair of eyes has sanity checked things. If code reviews become mainly about this sanity check, then it's often a sign that the types, tests, or linting are inadequate (or that someone is being too picky). The other problem is when reviews too often focus on the big problems - architectural discussions about how to implement a feature - in which case there's usually a need to have more architectural discussion before implementation starts.
- detourdog 3y ago
  I agree with you and the person you are agreeing with. I have to add that using analysis takes none of that away. Using analysis creates a culture of analysis tools. This also can make the revealing of performance surprises a shared learning experience from the grayest beard to the greenest horn.
  - MrJohz 3y ago
    Completely agree, analysis tools go hand in hand with code review as a learning tool. Partly, they take more responsibility away from finding the obvious bugs, and partly, like you say, they help surface problems that people might not be looking out for. I'd love it if things like performance checking tools were more commonly used.

Waterluvian 3y ago

This is what I implemented prior to my team growing beyond 1 person. If linting passes, it’s valid style. If you think the style is problematic, that’s an issue with the linting and formatting and not the PR. In practice this hasn’t been a problem. We’ve made 2 or 3 linting PRs over the years.

mtsr 3y ago
It’s why opinionated tools like rustfmt are so good. They set and enforce a standard which is good enough that even perfectionists don’t feel like arguing them ad nauseam.
- Waterluvian 3y ago
  Yeah! Any one opinion may not be the best for any one person, but everyone being consistent is the best for everyone.
hereonout2 3y ago
Introducing linting in the test stage (flake8 with python) solves so many problems. By completely removing any personal preferences and disagreements on style, the whole team just gets to hate on and berate an inanimate tool instead. We all just seem to equally dislike flake8 and move on with our lives.

xwdv 3y ago

[flagged]

ryukoposting 3y ago

I've worked in spaces where static analysis tools were run automatically on every new PR. Trust me, it's not as good as it sounds. Static analyzers are fully incapable of detecting the more nuanced mistakes, so a human touch will still be necessary. Nearly all of the "bugs" found by the static analyzer won't actually be bugs, but you'll have to "fix" them anyway because the reviewer (again, you'll still need one) will demand that all the warnings be cleared before approving your code. Build a culture that prefers succinct, non-nitpicky code reviews. Static analysis tools only give reviewers more crap to nitpick.

NavinF 3y ago
> Nearly all of the "bugs" found by the static analyzer won't actually be bugs Which static analyzer is this? Every tool I've used only finds bugs the are provable so the false positive rate is essential zero
- kevin_thibedeau 3y ago
  Plenty of C/C++ static analysis tools have pedantic rules that flag correct code. Effective use of them means going through which rules you want to disable to minimize the unproductive make-work of satisfying the machine.

Macha 3y ago

Sometimes there's a real prisoner dilemma thing going on here too. As a senior reviewing a junior's PR there are things that could be better but sometimes it's just not important. Are their variable names a bit too wordy? Did they have uneven amounts of whitespace between methods? In an ideal world these are more a "feedback for next time if it becomes a pattern" type thing. But as a reviewer, someone is looking at your comments per PR as an indicator for how much guidance you're providing, or going "oh who let that get merged" so you make the comment anyway. And then as the reviewee, they're concerned that leaving comments unaddressed looks bad for their level of responsiveness to feedback, or they think the reviewer might give them bad feedback for pushing back, so they'll then go and make the changes. But now they need someone to approve the updated version and the latency cycle starts again.

delusional 3y ago
I always make it very clear in my comments if something is a blocker (this can't be merged before this is fixed) or a note (this would better fit the style of the existing code), and then i make it very clear in person that I do not take it as a personal affront if they don't fix my note comments. I find that this works pretty well. It has the additional positive effect of opening me up to criticism. If they understand that not all my comments are always vital or "right" I imagine it's easier to dispute anything else I say too.
- ryanschneider 3y ago
  Good call, I use “minor:” as a comment prefix for this but just realized that I got into that habit awhile back and the team composition has changed since then so newer members don’t have that context. I wish there was something like semantic commits “fix/feat/chore” for review feedback, specifically a keyword for these little things that are more future advice than must fixes. I also try to be clear when I don’t expect to do a second review if they make the minor changes, that the green check mark carries forward. More. broadly I’ve been trying to favor “rough consensus and running code” in my PR reviews, even if I don’t fully agree with an approach I’ll give the writer the benefit of the doubt since they’ve most likely spent more time thinking about the problem than I have. Or at least that’s my hope!

Philip-J-Fry 3y ago

Nitpicking is definitely a real thing. Perhaps it's the feel that they need to find something wrong with the code. But there's also very real issues that one person thinks is a nitpick that really isn't. Perhaps because they can't see the issue with their own eyes, they don't understand the issue or they lack the ability to leave feelings aside and rethink what they've written. We've all been attached to code we've written. We probably think it's the most elegant code ever written. But sometimes you've just gotta admit you were wrong, it's not readable or it's flawed and it will be worse off for the codebase. I've also been called a nitpicker by someone more senior than me for pointing out some very real and potentially problematic race conditions in their code. To me, a race condition is a fundamental issue with what has been written and should be fixed. But to them it was acceptable because they hadn't seen it break naturally yet.

nickjj 3y ago
> Nitpicking is definitely a real thing. Perhaps it's the feel that they need to find something wrong with the code. You can use code formatting and linting tools to automate the low hanging fruit but there's still opportunities to offer feedback that could be considered nitpicking but IMO isn't. For example variable names, breaking up long functions, not enough test coverage which hints maybe the author didn't consider these cases or changing this type of code: if not something: # Unhappy case else: # Happy case To: if something: # Happy case else: # Unhappy case If you have 10 devs working on a project you're going to get 10 different ways of thinking. If you're working on a 10 year old code base with a million lines of code and you don't address things that could sometimes feel like nitpicking then you wind up with a lot of technical debt and it makes it harder to work on the project. I like when people comment on my code and find ways to improve it in any way shape or form. I don't care if they have 5, 10 or 15 years less experience. I treat it like a gift because I'm getting to learn something new or think about something from a perspective I haven't thought of.
- cratermoon 3y ago
  > test coverage which hints maybe the author didn't consider these cases In my reviews, when I look at the tests, I put on a test engineer hat. Testers like to do things like give an empty value where one is supposed to be required by the business logic, a negative or other out-of-range value, an emoji or other wonky unicode in text. Just basic stuff to a test engineer, but lots of programmers only write tests for the expected happy path, and maybe one or two cases for common alternate paths. The most common failures I find are in handling dates and date comparisons. I've been wanting to add more fuzz testing to the tests I write, but I haven't quite gotten my head around how to do it effectively.

deleted 3y ago

[deleted]

pjmlp 3y ago

I have given up on review process. Just implement whatever clean code guidelines everyone is keen in having, with their dependency injection guidelines of an interface for every single class, and move on with the actual work instead of burning discussion cycles in pull request reviews.

RHSeeger 3y ago

Static analysis tools and peer reviews can catch different types of issues, though. Just like a statically compiled language can catch some bugs that a dynamic language would not; but not everything. I'm a big fan of peer reviews, and I tend to focus my comments on - This won't do what you appear to be expecting it to - This will prevent <some other feature we're likely to need> from being implemented; at least without a much higher cost - This will work, but is very hard to understand; it will negatively impact maintenance. Consider doing it this other way, or adding an explanation about what's going on - This code is ok, but might read/work better <this other way>. I'm not failing the review because of it, but it's worth keeping in mind for future code.

Tainnor 3y ago

Many code bases that I've worked on suffered from too much, and badly engineered, code - not from not having enough code. Yes, you can go to extremes, and some people are annoyingly nitpicky when it doesn't matter. But generally, code is often a liability, stuff that is hard to understand (or even potentially subtly wrong) will cause issues down the line, and so on. Code review helps mitigate these issues, something which I think is one of the few empirical results we actually have. Plus, it also spreads knowledge about the code base. The submission is in any case not pointing out that code review itself is bad (or even that having to adapt to a new code style is the problem), but rather the whole bureaucracy around it. Code reviews are good when the turnaround is swift.

ReaLNero 3y ago
> Many code bases that I've worked on suffered from too much, and badly engineered, code - not from not having enough code. Because the projects that weren't built fast enough were eventually thrown away, meaning they never require maintenance, which is the source of the sampling bias you are observing. I've seen it happen in some cases (engineer with the wrong personality leading an R&D project).
- Tainnor 3y ago
  But not every code base is in an early startup phase. Once the company is stable there isn't really any excuse to knock out underengineered garbage just to get something out of the door.

Forricide 3y ago

I _kind of_ understand in the sense that - it can feel bad/like wasted time to do code review when you don't leave a single comment. But of course, it's very very important to ignore that feeling and do the sensible thing, which is be happy that the code review is smooth and the code can be merged. I can't imagine the existential dread of realizing that half your job is pointing out the same common errors every single time. Tools like clang-tidy, clang-format, etc are so vital for staying sane.

lrem 3y ago
I’ve reached the point where my reviews tend to be either instant LGTM, or “please formalise the idea behind this into a design doc and submit for review to the TL forum” (I.e. a polite “I think you don’t know what you’re doing”). The usual “how about ${BETTER_NAME}” can be left optional, there’s very rarely anything actually bad by the time people submit for review. Is this a rare culture?
- Forricide 3y ago
  This definitely isn't my experience, but I work in C++, where having a second eye to look over things is... valuable. That said, I think culture for this kind of thing varies wildly between different companies, and between teams in those companies; many (most?) teams are comprised of small numbers of people, so even one team/technical lead having a preference for a certain style could change things, and... well, everyone's different.

PartiallyTyped 3y ago

I have been told that we need 2 people to do code review, in a team of 3 people, and we can just “ask a member from another team who works on the same product”. We were told that outsiders can act as external bar raisers who will comment on “overall flow and code quality”. Sounds like glorified and highly opinionated linters whom will just gatekeep progress because they “don’t get it”.

AdrianB1 3y ago

I am doing code reviews for a large manufacturing company with dozens of plants; that code is supposed to be deployed everywhere, so the entire company is at stake. I insist on doing code review in areas where static analysis tools are completely useless, like in SQL code: without context, any SELECT can be good enough to pass static analysis, but crash in production due to performance problems. Nobody was ever promoted in my company for doing code reviews. None of the people that were ever promoted know how to do any sort or code review. I can say that the correlation between promotion and code reviews is negative.

jpollock 3y ago

This week during code reviews, I caught: 1) O(N^2) in the hottest code we have. 2) O(N^3) from someone else in the same area. 3) Filling a cache on first access - this causes the system to page (high latency!). 4) Hunting for data themselves and creating null pointer errors. 5) Hunting for data themselves and missing all the fun edge cases. 6) Duplicating a feature that already existed. I'm guessing (4) could have been caught by static analysis?

regularfry 3y ago
Possibly (6), depending on the nature of the duplication.

qudat 3y ago

Agreed. We don’t need human linters: https://bower.sh/human-linting https://bower.sh/human-linting

dkarl 3y ago

I agree that formatters and static analysis tools are the best way to reduce trivial PR comments. However, I would say that the kinds of things addressed by formatters and static analysis tools, and similar issues that tools don't catch such as misspellings, are worth addressing in PRs. "Nitpicking" is in the eye of the beholder, and your company might have a particular culture problem that I haven't seen, but many of the things I've seen called "nitpicking" are legitimate readability issues. For example, a field named "customers" that holds a single customer can be a major readability hurdle for someone who is looking at code for the first time. Even if you discount readers' annoyance and discomfort, which I don't think you should, a single field named "cusotmer" can become a drag on readability if the misspelling gets amplified in the codebase or if some programmers on the project aren't native English speakers. (It can be much harder to mentally gloss over language errors that aren't in your native language.) Something I learned in school is that it's extremely hard to read something you've written as another person would read it. I had an English teacher that would split the class into pairs every time we had a paper to turn in, and in each pair, you would read the other person's paper out loud to them. The results were comical, and to most students entirely unexpected. Their whole school careers, they had thought teachers were being mean and nit-picky, until they got to see their friends struggle to read what they had written. Then they started to understand that a wrong word, a misspelling, a wrong verb tense, an ambiguous pronoun, could stop a reader in their tracks or send them down the wrong path trying to understand what the writer meant. They also learned that they could not see these issues in their own writing. When they read somebody else's writing, they would stumble over errors, but in their own writing, they had to make a special effort to see them, and even then, readers would effortlessly "discover" errors that the author had missed. For many programmers, PRs are their first and only opportunity to develop this empathy for readers, and if they reject the feedback, they're going to remain forever in the dark about how other people experience their code.