10 ms·

> Developers need to make progress; overly difficult reviews can discourage future improvements. This, to me, is the most important aspect of code review: get

> Developers need to make progress; overly difficult reviews can discourage future improvements. This, to me, is the most important aspect of code review: get out of the way. If you are putting in nit picks, asking for changes that aren't related to a bug or bad architecture choice, etc, then get out of the way. If you are holding up the approval because you want someone to rewrite the code the way you would have written it, get out of the way. If you're saying anything that isn't directly related to a necessary change to the code, get out of the way. When I review, if there's no bugs, and nothing that's going to affect performance, and it passes tests and works, I approve it. If somebody wants mentorship I'll add my thoughts in a comment along with my approval. I don't have this kind of authority, but I would also recommend just not requiring a PR for certain changes. Create a guideline so that changes which a person couldn't improve by reviewing or that have no impact on the working product are just merged. More merging, less useless blockage.

onlyrealcuzzo 3y ago
Code review at most companies is an ego thing, for reviewers to feel self-important. It's a way for senior people to pieces of crap to everyone underneath them because that's what they put up with for most of their career - and now they feel like it's their turn to be a piece of crap back and perpetuate the cycle. So many industries run on this exact same cycle. At Google, the company does a good job convincing you that you're in the top x% and lucky to be there. People generally don't really feel the need to crap on people in code review to make themselves feel self-important.
- sanderjd 3y ago
  I never know whether I've just been fortunate in my career or what, but I've just never experienced anything approaching the kind of unkindness that I frequently see people say is common. I dunno, maybe it's just me, but I'm skeptical. I've always worked with professional colleagues who are trying to figure out how to do their best, and have never once seen people behaving in this childish way.
  - Ensorceled 3y ago
    For every unkind reviewer I've encountered in my career, I've encountered at least one person who overreacts to review comments. I also agree that both have been pretty rare for me.
  - wubrr 3y ago
    It really depends on the company/team. Certain companies (Amazon), use PR feedback stats as promotion metrics. They don't even consider the feedback quality, literally the stat 'average number of comments per PR' was one of the metrics used at my last team. The only 2 people promoted on this team during my tenure there got promoted based mostly on their loudness in PRs and team meetings and constant pushing for various (mostly counterproductive) 'process improvements' - these people didn't actually ship a single feature/project on the team. Very toxic place to work to say the least.
daveguy 3y ago
I think PRs are necessary. You can make a very minor change that introduces a bug or discrepancy in documentation. But I 100% agree with your "get out of the way" philosophy. That notice should come with the PR notification for the reviewer!
sanderjd 3y ago
I appreciate suggestions that are not "directly related to a necessary change to the code", and I believe my coworkers appreciate mine. It is very useful to say "did you consider doing it xyz way / using xyz library?" while also mashing the "approve" button. If they say "yep and I decided not to because of xyz" or "yep I like that idea but I'm not gonna change it at this point" then no harm done. But pretty often in my experience they say, "ah, nice, thanks, I didn't think of that / I wasn't aware of that, I like that more; updated now", and then everybody is even happier. Another useful thing, in my opinion, is to use your "fresh eyes" to make suggestions on what could use to be clarified in code or documentation. "This wasn't immediately obvious to me, could you add a comment on why it works this way?". The author is often too close to the code to see that it isn't obvious, because it is to them, because they've already spent hours thinking about it. But this should almost always what be optional suggestions. (But, rarely, code is so unclear that it really shouldn't go in without being clarified or documented.) But in general I do think it's important to consistently be thinking to oneself "my goal is to be useful to my team and organization. are my comments achieving that? would it be more useful to approve or to not approve this right now?". The temptation to be a fastidious editor can be strong - if most of us didn't have this impulse we'd probably be doing different jobs... - but I find this "usefulness" framing to help me a lot in resisting it. And very strong disagree on not requiring reviews. It is much better to tirelessly foster a low-friction review culture.
- _ea1k 3y ago
  > [...] while also mashing the "approve" button +1 - It is easy to forget that it is possible to approve with comments. Not all review feedback should block a merge.
  - nox100 3y ago
    It is a merge block in some code bases. You need to know the code the person said they were going to commit is the code they actually commit. Especially when there is financial incentive and state actors that want code inserted. In my project we used to be allowed to approve with nits but recently they changed it that the code needs a re-review for almost any edits. The system has some criteria for which it will allow minor edits but it's strict enough that I've rarely seen it pass an edit. I assume there must have been some incident that triggered this.
wubrr 3y ago
> If you are putting in nit picks, asking for changes that aren't related to a bug or bad architecture choice, etc, then get out of the way. If you are holding up the approval because you want someone to rewrite the code the way you would have written it, get out of the way. If you're saying anything that isn't directly related to a necessary change to the code, get out of the way. But how will I prove my 'mentorship' abilities and seniority at FAANG without pretending my subjective nitpicks and lack of understanding are critical PR blockers? /s
__turbobrew__ 3y ago
Hear hear! Nit picking can be soul crushing. Having to argue with someone over something which is trivial and does not matter is a sure fire way to kill my productivity.
iwontberude 3y ago
I am as unopinionated as I want others to be. All they need to do is commit a patch to my branch and fix it. It won't hurt my feelings and I don't need to consent. Let's get past the egos and wasted time asking for permission to change things.
fatherzine 3y ago
100%, extra points for fitted penname.
BytesAndGears 3y ago
I like your thought process here, but let me propose an alternative. I’m going to assume that we’re talking about Senior->Senior level reviews here. Junior engineers should obviously always get more guidance. But for Seniors, the alternative is: Be extremely picky for PRs from new hires, so that they do things the way that your company does them. Make sure they put files in the right places, and adhere to the “flow” of the rest of the codebase. Also the sorts of style that aren’t picked up by a linter — like architecture choices — should be heavily scrutinized to make sure that they fit with your company’s paradigms. After a while, they’re fully assimilated and you have a coherent style so that everyone is on the same page. Future code reviews go quickly because everyone makes similar architecture decisions and you’re never surprised. This also makes code easier to read if it all follows the same subset of patterns. I’m not actually sure which way is better, but I think that both options have benefits.
- marcosdumay 3y ago
  I've received plenty of negative feedback on the internet for this (and up to now, discarded all), but I really don't think acceptance review is the correct time for guidance. Yes, juniors need guidance. That means you must read their code, and talk to them about it. They also need real-world feedback. They do really need you not managing all of the interaction they have with the real world. Now, what you can not manage is up for a complex risk analysis, but they really need you to get out of the way at some point.
  - johnnyanmac 3y ago
    So, when is the right time?
    - marcosdumay 3y ago
      At some time you plan for it. Preferably a calm time, not one they are waiting a decision from you. E.g. before they start implementing a feature is better than after they finish it.
Eji1700 3y ago
> If you are holding up the approval because you want someone to rewrite the code the way you would have written it, get out of the way. While I'm being pedantic here and get your point, I do think it's important that code is following proper styles. I get that there's a bunch of nitpicky nonsense that goes into code review, but making sure it follows the same style so in 6 months when someone else has to dig through it you don't have to teach a totally different pattern/method or figure out what arcane bullshit they did is important. And again, I'm sorta assuming this was implied but wanted to throw it out there since it's one of the most annoying things i've ever had to deal with.
- ryandrake 3y ago
  Style wars are the kind of thing that should just be automated away with a source control hook that reformats all submissions.
  - wubrr 3y ago
    This. Any subjective style/pattern agreed to by the team should be checked and/or fixed automatically. If you don't have lint rules set up to enforce these things - go write them instead of commenting about subjective style preferences in PRs.
hn_throwaway_99 3y ago
Your comment is why I highly recommend using conventional comment labels, https://conventionalcomments.org/#labels https://conventionalcomments.org/#labels, when doing code reviews. We've internalized them where I work and I think they're really valuable. When it's abundantly clear whether something is a nitpick, suggestion, or a blocking issue, because it literally has a prefix to that effect, it takes a lot of the potential stress and confrontation out of the code review process, on both sides. Note you also don't need to memorize the labels, but starting with one forces the reviewer to ask themselves the question "Should I really block the merge for this?"
jrockway 3y ago
To me the biggest time sink in PR reviews is getting the reviewer to open the code review. If I ever take over the world, I'd make a new OS that locks all activity whenever you get a code review and doesn't let you switch applications until you click "approve" or "request changes". I find it really crazy how long people take to do reviews. I have Github connected to Slack; if someone needs my review I get a message and can have it done within 5 minutes. (I think I write the most comments on code reviews of anyone in my organization, and I also have the lowest review latency. If I can do it, anyone can do it.)
- iwontberude 3y ago
  If you link peoples ability to use their computer by how quickly they can click "Approve" on a PR, it will definitely lead to a break down in review quality and beat you at your own personal records of latency and MTTR.
  - jrockway 3y ago
    I'm sure there would be negative effects, but at least things would move forward quickly. This is the worst system except for all the others.
    - yjftsjthsd-h 3y ago
      Move quickly yes, move forward not necessarily. You've described a system where everybody hits approve or finds something to complain about as fast as they possibly can without worrying about little questions like whether it should have been approved or whether the changes they request are legitimate.
    - johnnyanmac 3y ago
      Sounds like you work best on a small team if that's your focus. Medoum-large ones already have a proven product (or proven experience). You don't focus on blazing gast iterations at that stage unless you are in early R&D or something evergreen. And well, if you're primary motivation is Financial: okay, you're not being paid to move quickly. Or if you hope to move up the ladder quickly you gotta deal with the office politics. Which includes delays in communication. React to those lapses as you feel is appropriate to your goals and personality.
kstrauser 3y ago
Note that PRs are mandatory if a company goes through SOC-2 or any other compliance framework. The underlying question they answer is “could a malicious or incompetent employee send bad code to production without another engineer viewing it first?” The specific response auditors expect is “no, because another human reviews all change requests before they’re approved”.
cedws 3y ago
Well said. Ego should stay out of PRs. Your job as a reviewer is to help the code get merged, not hinder it.
ruszki 3y ago
So you don’t care about readability, extensibility and technical debt then, right? This way of thinking can cause a lot of problems long term.
cgearhart 3y ago
This kind of greedy optimization approach is fine if all you’re just trying to maximize velocity, but it starts showing cracks on a long enough timeline in sufficiently large or complex projects. Speed without alignment on direction leads to ugly, tangled messes. As with most quality issues, the key is to try and surface quality problems as early as possible. Finding a bug in prod is worse than finding it in PR is worse than finding it with tests is worse than finding it at your desk is worse than finding it in the design. Engineering teams should choose an operating point in their processes that balances quality and velocity to match the business needs of their project. Most often I’ve seen slow PRs caused by stuffing too much of the responsibility for software quality into that single step. In my team we prefix nitpicky comments explicitly with “nit:” and it’s up to the author to decide what to do with it. We minimize trivial nits by enforcing a style checker. We also discuss in retro if the PRs are too big to review effectively or if PRs are turning up comments on solution architecture, because at that point it’s too late—the author likely should’ve separately discussed the architecture before it got to a PR.
- wubrr 3y ago
  Generally agree, but at some companies/teams the ratio of 'nitpicky subjective feedback' to 'you have a bug here feedback' is like 95:5 while at others it's 5:95. The latter function much better.
- poolopolopolo 3y ago
  MRs are bad place to align things at scale.
  - creshal 3y ago
    They should be a last resort for that sort of alignment, but it's still important to pull that last resort if it's necessary, so you don't end up like Cloudflare and let your own standards slip so badly that you end up with a 100% preventable multi-day outage.
- zdragnar 3y ago
  > In my team we prefix nitpicky comments explicitly with “nit:” and it’s up to the author to decide what to do with it This is different from what the OP is talking about. I've worked at places where staff engineers seem to have been rated on number of comments left on PRs... they were typically somewhere between nit picky and useless, with the occasional person directly contradicting feedback they'd given in a previous PR. Oh, and you had to address every one of them before getting approval.
notyourwork 3y ago
> If somebody wants mentorship I'll add my thoughts in a comment along with my approval. This is always the approach I take. Especially for junior engineers. Stay out of the way, let them merge code with as little friction and discouragement as possible. Anything that I would have implemented or approached differently I try to elaborate on. Even go so far as to write some (most or all) of the code in a comment or in a remote branch to share. I will still ship the code but let the submitter digest my thoughts.
Kelkonosemmel 3y ago
I communicate on purpose that code review is never the 99% mark It's the time to get feedback and improve. The problem is: if people are stuck and don't learn the code review phase will always stay as a long thing. The whole team is responsible for security, maintainability, etc. I don't care about some code monkey able to write some code or fixing something. I care about a team, a product. Your attitude would not work in my team, which is fine
lokar 3y ago
IME the biggest blocking feedback to Jr engineers about how to better structure things (the most expensive feedback) is around making it easy and clean to test the code. Fairly often a modest (but straightforward) refactor is needed to get good tests.
dawnerd 3y ago
Yeah no, part of code review is ensuring code quality is maintained. If you start letting badly written code fly because it passes tests, that’ll bite you. If something isn’t blocking I’ll make a note that it should be fixed if theres budget knowing sometimes you gotta let things go. But you can’t just ignore code quality and call it nitpicking.
- withinboredom 3y ago
  Badly written, or badly designed? Bad code, to me, would be an unrolled for-loop, or a for-loop where a foreach loop would do better (though now, we're getting in the realm of nit-picking). A PR is waaaaay too late to bring up architectural/design improvements (unless it is a WIP PR opened expressly for discussing the approach). I very rarely see bad code in PR's unless it is from a Junior programmer, and even then, it's usually because they don't know better. Even then, I choose to trust them to do the right thing. In most environments, you can even review the code AFTER it has been merged. So, if they don't address the issue, I can still review the code and let them know that I expect to see a follow-up PR addressing it.
  - thiht 3y ago
    > A PR is waaaaay too late to bring up No. That’s literally the point of a review.
    - withinboredom 3y ago
      No, it's not and there is a ton of literature (even entire books) written on why it isn't the time to bring this up during a review. For starters, it results in a waste of time for literally everyone involved: 1. The person writing has to rewrite it (probably). 2. The reviewer could have sat down with the person before a single line of code was written. 3. Anyone else reviewing just wasted their time because it will be rewritten. If you see code badly designed, walk over to, or call the developer and say "hey, can we discuss the design of the code." The saying "measure twice, cut once" doesn't just apply to wood. Design before you ever write any code.
NicoJuicy 3y ago
I care about readability of code, when reviewing a PR. Not doing it every time, will come back to haunt me later. Most of the times, the reply on my comment for a more obvious variable name is ( not: ) is: yeah, I thought about a better name but couldn't find any or something is found in between. Ofc, sometimes the change is so ugly I'll feel bad because they should rewrite it. I'm still struggling with it. Some people just can't code too. These will come back to haunt us. And will have bugs too...
sonicanatidae 3y ago
The other side of this coin is without QA, there is no app. Sure, technically, it can be thrown out the door, and often is, in just this form, but it's a shit-tastic mess without QA time and the users are getting real tired of half-assed garbage that cost twice what the prior package cost.
ravenstine 3y ago
The fact that your comment is currently at the top of the thread tells me that a lot of people go through this, yet this is interesting to me because past HN discussions have suggested said experience to be an exception. In my own experience, blocking (or simply not approving) PRs over nits, out-of-scope improvements, and mere personal preferences is the norm. What's worse is when PRs get blocked because a person doesn't like the pattern the author is using even though it's already one of many existing patterns and there's no explicit guideline anywhere. I find this happens a lot when lead and staff engineers make decisions on what's "considered harmful" but don't actually broadcast that decision in an effective way. What I try to do with my code reviews is prefix my comments with "Non-blocking suggestion:" or "Minor question:" so that the other person is more likely to know that a particular piece of feedback doesn't come with the expectation of needing to be addressed. Otherwise, if the code works, has tests, and isn't objectively incorrect, I try to give an approval. If you don't like something about it, there should be room in the process to make improvements. The idea that "mistakes won't actually get fixed later" is a cop out. If your team can't fix mistakes, then you should turn in your "engineer" titles. Also, when programmers do things that others on their team disagree with, it almost always comes down to a breakdown in communication. Junior developers are a bit of a different story, but senior developers not knowing how to write code that the rest of the team approves of means that people aren't communicating, and it probably means that your system is so complex that it's unreasonably difficult to determine the proper approach in the first place.
colonwqbang 3y ago
Missing or misleading documentation. Confusing names. Lacking test coverage for new functionality. Unnecessarily complex code that can readily be simplified. Code that looks important but in fact does nothing useful. Unrelated changes mixed in with a patch that's ostensibly about something else. These are all things I think are valid to point out in a code review. I don't think I would like to work in a company that rejects the notion.

onlyrealcuzzo 3y ago

Code review at most companies is an ego thing, for reviewers to feel self-important. It's a way for senior people to pieces of crap to everyone underneath them because that's what they put up with for most of their career - and now they feel like it's their turn to be a piece of crap back and perpetuate the cycle. So many industries run on this exact same cycle. At Google, the company does a good job convincing you that you're in the top x% and lucky to be there. People generally don't really feel the need to crap on people in code review to make themselves feel self-important.

sanderjd 3y ago
I never know whether I've just been fortunate in my career or what, but I've just never experienced anything approaching the kind of unkindness that I frequently see people say is common. I dunno, maybe it's just me, but I'm skeptical. I've always worked with professional colleagues who are trying to figure out how to do their best, and have never once seen people behaving in this childish way.
- Ensorceled 3y ago
  For every unkind reviewer I've encountered in my career, I've encountered at least one person who overreacts to review comments. I also agree that both have been pretty rare for me.
- wubrr 3y ago
  It really depends on the company/team. Certain companies (Amazon), use PR feedback stats as promotion metrics. They don't even consider the feedback quality, literally the stat 'average number of comments per PR' was one of the metrics used at my last team. The only 2 people promoted on this team during my tenure there got promoted based mostly on their loudness in PRs and team meetings and constant pushing for various (mostly counterproductive) 'process improvements' - these people didn't actually ship a single feature/project on the team. Very toxic place to work to say the least.

daveguy 3y ago

I think PRs are necessary. You can make a very minor change that introduces a bug or discrepancy in documentation. But I 100% agree with your "get out of the way" philosophy. That notice should come with the PR notification for the reviewer!

sanderjd 3y ago

I appreciate suggestions that are not "directly related to a necessary change to the code", and I believe my coworkers appreciate mine. It is very useful to say "did you consider doing it xyz way / using xyz library?" while also mashing the "approve" button. If they say "yep and I decided not to because of xyz" or "yep I like that idea but I'm not gonna change it at this point" then no harm done. But pretty often in my experience they say, "ah, nice, thanks, I didn't think of that / I wasn't aware of that, I like that more; updated now", and then everybody is even happier. Another useful thing, in my opinion, is to use your "fresh eyes" to make suggestions on what could use to be clarified in code or documentation. "This wasn't immediately obvious to me, could you add a comment on why it works this way?". The author is often too close to the code to see that it isn't obvious, because it is to them, because they've already spent hours thinking about it. But this should almost always what be optional suggestions. (But, rarely, code is so unclear that it really shouldn't go in without being clarified or documented.) But in general I do think it's important to consistently be thinking to oneself "my goal is to be useful to my team and organization. are my comments achieving that? would it be more useful to approve or to not approve this right now?". The temptation to be a fastidious editor can be strong - if most of us didn't have this impulse we'd probably be doing different jobs... - but I find this "usefulness" framing to help me a lot in resisting it. And very strong disagree on not requiring reviews. It is much better to tirelessly foster a low-friction review culture.

_ea1k 3y ago
> [...] while also mashing the "approve" button +1 - It is easy to forget that it is possible to approve with comments. Not all review feedback should block a merge.
- nox100 3y ago
  It is a merge block in some code bases. You need to know the code the person said they were going to commit is the code they actually commit. Especially when there is financial incentive and state actors that want code inserted. In my project we used to be allowed to approve with nits but recently they changed it that the code needs a re-review for almost any edits. The system has some criteria for which it will allow minor edits but it's strict enough that I've rarely seen it pass an edit. I assume there must have been some incident that triggered this.

wubrr 3y ago

> If you are putting in nit picks, asking for changes that aren't related to a bug or bad architecture choice, etc, then get out of the way. If you are holding up the approval because you want someone to rewrite the code the way you would have written it, get out of the way. If you're saying anything that isn't directly related to a necessary change to the code, get out of the way. But how will I prove my 'mentorship' abilities and seniority at FAANG without pretending my subjective nitpicks and lack of understanding are critical PR blockers? /s

__turbobrew__ 3y ago

Hear hear! Nit picking can be soul crushing. Having to argue with someone over something which is trivial and does not matter is a sure fire way to kill my productivity.

iwontberude 3y ago

I am as unopinionated as I want others to be. All they need to do is commit a patch to my branch and fix it. It won't hurt my feelings and I don't need to consent. Let's get past the egos and wasted time asking for permission to change things.

fatherzine 3y ago

100%, extra points for fitted penname.

BytesAndGears 3y ago

I like your thought process here, but let me propose an alternative. I’m going to assume that we’re talking about Senior->Senior level reviews here. Junior engineers should obviously always get more guidance. But for Seniors, the alternative is: Be extremely picky for PRs from new hires, so that they do things the way that your company does them. Make sure they put files in the right places, and adhere to the “flow” of the rest of the codebase. Also the sorts of style that aren’t picked up by a linter — like architecture choices — should be heavily scrutinized to make sure that they fit with your company’s paradigms. After a while, they’re fully assimilated and you have a coherent style so that everyone is on the same page. Future code reviews go quickly because everyone makes similar architecture decisions and you’re never surprised. This also makes code easier to read if it all follows the same subset of patterns. I’m not actually sure which way is better, but I think that both options have benefits.

marcosdumay 3y ago
I've received plenty of negative feedback on the internet for this (and up to now, discarded all), but I really don't think acceptance review is the correct time for guidance. Yes, juniors need guidance. That means you must read their code, and talk to them about it. They also need real-world feedback. They do really need you not managing all of the interaction they have with the real world. Now, what you can not manage is up for a complex risk analysis, but they really need you to get out of the way at some point.
- johnnyanmac 3y ago
  So, when is the right time?
  - marcosdumay 3y ago
    At some time you plan for it. Preferably a calm time, not one they are waiting a decision from you. E.g. before they start implementing a feature is better than after they finish it.

Eji1700 3y ago

> If you are holding up the approval because you want someone to rewrite the code the way you would have written it, get out of the way. While I'm being pedantic here and get your point, I do think it's important that code is following proper styles. I get that there's a bunch of nitpicky nonsense that goes into code review, but making sure it follows the same style so in 6 months when someone else has to dig through it you don't have to teach a totally different pattern/method or figure out what arcane bullshit they did is important. And again, I'm sorta assuming this was implied but wanted to throw it out there since it's one of the most annoying things i've ever had to deal with.

ryandrake 3y ago
Style wars are the kind of thing that should just be automated away with a source control hook that reformats all submissions.
- wubrr 3y ago
  This. Any subjective style/pattern agreed to by the team should be checked and/or fixed automatically. If you don't have lint rules set up to enforce these things - go write them instead of commenting about subjective style preferences in PRs.

hn_throwaway_99 3y ago

Your comment is why I highly recommend using conventional comment labels, https://conventionalcomments.org/#labels https://conventionalcomments.org/#labels, when doing code reviews. We've internalized them where I work and I think they're really valuable. When it's abundantly clear whether something is a nitpick, suggestion, or a blocking issue, because it literally has a prefix to that effect, it takes a lot of the potential stress and confrontation out of the code review process, on both sides. Note you also don't need to memorize the labels, but starting with one forces the reviewer to ask themselves the question "Should I really block the merge for this?"

jrockway 3y ago

To me the biggest time sink in PR reviews is getting the reviewer to open the code review. If I ever take over the world, I'd make a new OS that locks all activity whenever you get a code review and doesn't let you switch applications until you click "approve" or "request changes". I find it really crazy how long people take to do reviews. I have Github connected to Slack; if someone needs my review I get a message and can have it done within 5 minutes. (I think I write the most comments on code reviews of anyone in my organization, and I also have the lowest review latency. If I can do it, anyone can do it.)

iwontberude 3y ago
If you link peoples ability to use their computer by how quickly they can click "Approve" on a PR, it will definitely lead to a break down in review quality and beat you at your own personal records of latency and MTTR.
- jrockway 3y ago
  I'm sure there would be negative effects, but at least things would move forward quickly. This is the worst system except for all the others.
  - yjftsjthsd-h 3y ago
    Move quickly yes, move forward not necessarily. You've described a system where everybody hits approve or finds something to complain about as fast as they possibly can without worrying about little questions like whether it should have been approved or whether the changes they request are legitimate.
  - johnnyanmac 3y ago
    Sounds like you work best on a small team if that's your focus. Medoum-large ones already have a proven product (or proven experience). You don't focus on blazing gast iterations at that stage unless you are in early R&D or something evergreen. And well, if you're primary motivation is Financial: okay, you're not being paid to move quickly. Or if you hope to move up the ladder quickly you gotta deal with the office politics. Which includes delays in communication. React to those lapses as you feel is appropriate to your goals and personality.

kstrauser 3y ago

Note that PRs are mandatory if a company goes through SOC-2 or any other compliance framework. The underlying question they answer is “could a malicious or incompetent employee send bad code to production without another engineer viewing it first?” The specific response auditors expect is “no, because another human reviews all change requests before they’re approved”.

cedws 3y ago

Well said. Ego should stay out of PRs. Your job as a reviewer is to help the code get merged, not hinder it.

ruszki 3y ago

So you don’t care about readability, extensibility and technical debt then, right? This way of thinking can cause a lot of problems long term.

cgearhart 3y ago

This kind of greedy optimization approach is fine if all you’re just trying to maximize velocity, but it starts showing cracks on a long enough timeline in sufficiently large or complex projects. Speed without alignment on direction leads to ugly, tangled messes. As with most quality issues, the key is to try and surface quality problems as early as possible. Finding a bug in prod is worse than finding it in PR is worse than finding it with tests is worse than finding it at your desk is worse than finding it in the design. Engineering teams should choose an operating point in their processes that balances quality and velocity to match the business needs of their project. Most often I’ve seen slow PRs caused by stuffing too much of the responsibility for software quality into that single step. In my team we prefix nitpicky comments explicitly with “nit:” and it’s up to the author to decide what to do with it. We minimize trivial nits by enforcing a style checker. We also discuss in retro if the PRs are too big to review effectively or if PRs are turning up comments on solution architecture, because at that point it’s too late—the author likely should’ve separately discussed the architecture before it got to a PR.

wubrr 3y ago
Generally agree, but at some companies/teams the ratio of 'nitpicky subjective feedback' to 'you have a bug here feedback' is like 95:5 while at others it's 5:95. The latter function much better.
poolopolopolo 3y ago
MRs are bad place to align things at scale.
- creshal 3y ago
  They should be a last resort for that sort of alignment, but it's still important to pull that last resort if it's necessary, so you don't end up like Cloudflare and let your own standards slip so badly that you end up with a 100% preventable multi-day outage.
zdragnar 3y ago
> In my team we prefix nitpicky comments explicitly with “nit:” and it’s up to the author to decide what to do with it This is different from what the OP is talking about. I've worked at places where staff engineers seem to have been rated on number of comments left on PRs... they were typically somewhere between nit picky and useless, with the occasional person directly contradicting feedback they'd given in a previous PR. Oh, and you had to address every one of them before getting approval.

notyourwork 3y ago

> If somebody wants mentorship I'll add my thoughts in a comment along with my approval. This is always the approach I take. Especially for junior engineers. Stay out of the way, let them merge code with as little friction and discouragement as possible. Anything that I would have implemented or approached differently I try to elaborate on. Even go so far as to write some (most or all) of the code in a comment or in a remote branch to share. I will still ship the code but let the submitter digest my thoughts.

Kelkonosemmel 3y ago

I communicate on purpose that code review is never the 99% mark It's the time to get feedback and improve. The problem is: if people are stuck and don't learn the code review phase will always stay as a long thing. The whole team is responsible for security, maintainability, etc. I don't care about some code monkey able to write some code or fixing something. I care about a team, a product. Your attitude would not work in my team, which is fine

lokar 3y ago

IME the biggest blocking feedback to Jr engineers about how to better structure things (the most expensive feedback) is around making it easy and clean to test the code. Fairly often a modest (but straightforward) refactor is needed to get good tests.

dawnerd 3y ago

Yeah no, part of code review is ensuring code quality is maintained. If you start letting badly written code fly because it passes tests, that’ll bite you. If something isn’t blocking I’ll make a note that it should be fixed if theres budget knowing sometimes you gotta let things go. But you can’t just ignore code quality and call it nitpicking.

withinboredom 3y ago
Badly written, or badly designed? Bad code, to me, would be an unrolled for-loop, or a for-loop where a foreach loop would do better (though now, we're getting in the realm of nit-picking). A PR is waaaaay too late to bring up architectural/design improvements (unless it is a WIP PR opened expressly for discussing the approach). I very rarely see bad code in PR's unless it is from a Junior programmer, and even then, it's usually because they don't know better. Even then, I choose to trust them to do the right thing. In most environments, you can even review the code AFTER it has been merged. So, if they don't address the issue, I can still review the code and let them know that I expect to see a follow-up PR addressing it.
- thiht 3y ago
  > A PR is waaaaay too late to bring up No. That’s literally the point of a review.
  - withinboredom 3y ago
    No, it's not and there is a ton of literature (even entire books) written on why it isn't the time to bring this up during a review. For starters, it results in a waste of time for literally everyone involved: 1. The person writing has to rewrite it (probably). 2. The reviewer could have sat down with the person before a single line of code was written. 3. Anyone else reviewing just wasted their time because it will be rewritten. If you see code badly designed, walk over to, or call the developer and say "hey, can we discuss the design of the code." The saying "measure twice, cut once" doesn't just apply to wood. Design before you ever write any code.

NicoJuicy 3y ago

I care about readability of code, when reviewing a PR. Not doing it every time, will come back to haunt me later. Most of the times, the reply on my comment for a more obvious variable name is ( not: ) is: yeah, I thought about a better name but couldn't find any or something is found in between. Ofc, sometimes the change is so ugly I'll feel bad because they should rewrite it. I'm still struggling with it. Some people just can't code too. These will come back to haunt us. And will have bugs too...

sonicanatidae 3y ago

The other side of this coin is without QA, there is no app. Sure, technically, it can be thrown out the door, and often is, in just this form, but it's a shit-tastic mess without QA time and the users are getting real tired of half-assed garbage that cost twice what the prior package cost.

ravenstine 3y ago

The fact that your comment is currently at the top of the thread tells me that a lot of people go through this, yet this is interesting to me because past HN discussions have suggested said experience to be an exception. In my own experience, blocking (or simply not approving) PRs over nits, out-of-scope improvements, and mere personal preferences is the norm. What's worse is when PRs get blocked because a person doesn't like the pattern the author is using even though it's already one of many existing patterns and there's no explicit guideline anywhere. I find this happens a lot when lead and staff engineers make decisions on what's "considered harmful" but don't actually broadcast that decision in an effective way. What I try to do with my code reviews is prefix my comments with "Non-blocking suggestion:" or "Minor question:" so that the other person is more likely to know that a particular piece of feedback doesn't come with the expectation of needing to be addressed. Otherwise, if the code works, has tests, and isn't objectively incorrect, I try to give an approval. If you don't like something about it, there should be room in the process to make improvements. The idea that "mistakes won't actually get fixed later" is a cop out. If your team can't fix mistakes, then you should turn in your "engineer" titles. Also, when programmers do things that others on their team disagree with, it almost always comes down to a breakdown in communication. Junior developers are a bit of a different story, but senior developers not knowing how to write code that the rest of the team approves of means that people aren't communicating, and it probably means that your system is so complex that it's unreasonably difficult to determine the proper approach in the first place.

colonwqbang 3y ago

Missing or misleading documentation. Confusing names. Lacking test coverage for new functionality. Unnecessarily complex code that can readily be simplified. Code that looks important but in fact does nothing useful. Unrelated changes mixed in with a patch that's ostensibly about something else. These are all things I think are valid to point out in a code review. I don't think I would like to work in a company that rejects the notion.