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.

> 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.

> I've worked at places where staff engineers seem to have been rated on number of comments left on PRs

> Oh, and you had to address every one of them before getting approval.

This just sounds horrible.

> rated on the number of comments

It sounds idiotic.

It is unrelated to the question whether short-term (one PR) only reviews are preferable over reviews that recognize the reality that it is not the last PR ever and therefore more long-term view may be useful.

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.

MRs are bad place to align things at scale.

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.

Long-term project health rests on bigger things like API design, database choices, or service layout.

Hopefully these are in-scope when it comes to accepting or rejecting PRs and are not just considered incidental to whatever the stated purpose of the PR is. What you listed appears to be more fundamental system design concerns so hopefully they are candidates for further review.

It's usually out of scope by the time you're writing a CL, which is fine because there's design review. The issue is about priorities. Nitpicky code review can be very expensive, and I'm not convinced it brings noticeable benefit.

I've seen team-quarters wasted due to bad large-scale decisions. Sometimes the truth doesn't come out until the the real thing is in production, and all these extra burdens make it harder to change course. I think people are starting to get this because the amount of code nitpicking or caring about small things in general has gone way down.

> it starts showing cracks on a long enough timeline in sufficiently large or complex projects

Show me a project which this doesn't happen to on a long timeline and I'll show you a project with no users and perfect programmers.

Entropy is unavoidable. It will eventually degrade despite your best efforts and in the meantime you aren't benefiting from a working feature. We should try to delay software entropy as much as practical, but not at the expense of shipping.

Basically there is a balance to strike. I don't have a formula for it, but I do know that merging often ends up being more valuable than merging later just to ensure clean code. Working features are more valuable than a mess. A mess sucks, so we should try to avoid it, but the world runs on messes, not clean code.

Read the Redis code base please.

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.

> [...] 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.

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.

I read it as it’s up to the person who opened the PR whether to apply the nits, or whether to ignore them and use the approval to merge. This is how our codebase works: you’re free to ignore minor suggestions, but if you do edit the code, reviews are dismissed and need to be provided again.

Basically, an unaddressed nit doesn’t block merging. But any code changes will require fresh reviews.

If the change is very minor, it’s very quick for someone who has already approved to check the last commit diff and reapprove.

I think it's legitimate to force a re-review for any code change, given that programming is one of those charming areas where a single character can completely change the meaning of something.

That said, if you can read the commits individually then final review for something where minor changes were added should be utterly trivial.

IME this is always driven by lazy compliance people pushing the most straight forward way to satisfy some audit. Plenty of places subject to strict audit rules manage to make small post review edits work.

Yes, it is pretty common to require re-review if it is changed again. But approval with comments shifts control back to the submitter. They then have choices:

- Modify it and accept that this will trigger re-review

- Merge as-is... maybe they don't even agree with the nit

- Merge as-is and followup with a new MR under the same JIRA. This sometimes makes sense if it gets them into an important preprod environment, but they are really still working on the issue.

- Merge as-is and follow up with a JIRA on the backlog. Maybe they realized that the "nit" is really a bigger issue and should be addressed separately.

Regardless, there's value in not blocking the merge for non-functional issues.

I got a merge blocked last week with the comment 'I think this work is more related to "TFS X: prepare for Y" than it is "TFS Y".' (I eventually realized they wanted me to change the commit message). I hate this fucking team.

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.

Code "quality" is almost always an opinion, and not one that is based on anything objective. That doesn't mean that the idea of it doesn't have a purpose, but it's almost always used as a bat to knock someone over the head with. It's not like code can be distinguished as fresh or moldy and rotten like fruit. If that were so, then most programmers would hardly ever bicker about code quality; it would be self-evident, just as any moron can separate the good fruit from the bad.

If a unit of code fixes or improves the user experience without slowing it down and is memory efficient, then it becomes incredibly hard to objectively define said code as being bad. Perhaps it ruins the developer experience, but then again, many programmers object to things they simply don't like on a philosophical or theological basis. Programmers, depending on the language community, may consider anything that isn't "object oriented" to be poor quality, but good luck finding objective proof that object orientation is always better.

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.

> A PR is waaaaay too late to bring up

No. That’s literally the point of a review.

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.

>The reviewer could have sat down with the person before a single line of code was written.

This is a bigger waste of time. If every change requires asking someone in person, that's an infinite source of distraction and kills productivity for that person.

MOST code reviews don't end up in comments asking for big changes, and when they do the one-time cost on the author is much better to pay than the constant tax on your subject matter experts of what you're proposing.

How is spending 5 minutes x TEAM_SIZE to go over what you're going to do more time than 1-2 days solving a problem, then another hour or two for the reviewer to write out why it's wrong, then another day or two rewriting everything from scratch?

None of those timelines are realistic:

* Most CLs don't take multiple days to write

* Reasonably-sized CLs take 5-10 minutes to review - if I have things to say. Much less if the code looks good and I don't have any requests.

* I cannot think of any CL in the last 5 years where I made a comment that caused someone to "rewrite everything from scratch". Most feedback can be done in a few clicks in an IDE which supports refactor or a few minutes of copy/paste.

1. That's the point. It's not up to standard

2. They could but they usually don't. It's fine to sit down and make sure they understand some esoteric structure within a system I owj I'm not going to sit down with someone else to make sure they use for each loops unless it's a very new junior (i wouldn't block that in most code reviews anyway unless the code was legitimately painful to read, but every company is different).

3. IME there may be multiple reviewers but one primary reviewer. Half the time the non-primary ones either focus on "is this code going to break your work flow?" or are simply there for bookkeeping and isn't the approval you're looking for (i.e. A lead on a review that already talked about the plans but doesn't have the knowledge to review that specific module). Anymore than 1 primary reviewer probably needed a smaller PR if possible.

Anyone who has ever come along in a code review and told me my code “isn’t up to standard” will inevitably start a very long game of attrition with me. Here’s the thing: there’s no such thing as “standard” software architecture (until someone writes a new kind of software architecture called “standard”).

I’ve seen and used everything from MVC to DDD to TDD to MVVC to whatever React is to reactive to event-oriented to actors to whatever you can imagine. There simply isn’t a “standard” way to do anything in this industry. If you hired me, you hired me for these different perspectives on problem solving, and that goes for all of us.

A code review simply isn’t the place to enforce your unique perspectives on someone else.

Sometimes, there is simply a better way to do things. These might even be new ideas in the code base. If it looks like it is, hopefully the author will bring it up with the team beforehand. If the reviewer changes their mind in the code review… well, that’s not the authors problem. The reviewer should rewrite it and the author review it.

Actually, that’s exactly what I suggest if someone ever tries that with me (usually when I’m the new guy). Please submit your own PR showing me how it’s done. I’ll move on to another ticket.

Usually, about half way through their implementation, they’ll see why I did it the way I did it and approve my PR. It only takes one time before they start asking “why” instead of assuming they know everything; every line of code exists for a reason, after all.

In one case, one specific dude (bless his heart) kept blocking PRs after we discussed everything before hand. I had to get HR involved, and other engineers. Dude just didn’t like being wrong and would throw tantrums when he was. He eventually got fired after people realized they could do what I was doing to stop the nits he said was a blocker. Nobody cared if a variable should be renamed from “SameFactory” to “EqualFactory”.

Ah, but code quality! It’s so subjective. High code quality, to me, is easy to maintain code. Is the intention clear, do the comments reflect reality, is it easy to read, but more importantly, easy to change? Will changing a line in the module break 15 other sibling modules? God I hope not. Dependencies should be obvious. And, no, I’m not writing an interface if there is exactly one implementation. That’s ridiculous.

For some people, they want layers. More layers of abstraction than a layered cake on a wedding day. To them, that’s good code quality! Some people think high code quality is beautiful code. The kind you frame and put over your fire place to admire with a glass of wine.

There’s no objectively “high quality” code in existence.

>Anyone who has ever come along in a code review and told me my code “isn’t up to standard” will inevitably start a very long game of attrition with me.

standard is relative, and while I'm talking generically as someone on the internet with no context of your experience, a code reviewer shouldn't. Someone literally saying "the code is not up to standards" needs to clarify.

For my generic take, take "standard" as "whatever that team has laid out in its code architecture". Sometimes there may be times to question the standards if it compromises other important factors, but personally I don't think the code review is the time to fight over whitespace and bracket placement (those should be solved via a linter anyway). Make the correction and submit, those later discussions can happen offline.

>A code review simply isn’t the place to enforce your unique perspectives on someone else.

Likewise, the goal is to align everybody, not to argue over semantics or enforce your own POV. That's why code style sheets are a thing; it's a mediator that can serve as its own battlground should team/company style need to be changed. Again, every place is different, but

1. style should be automated as much as possible. If there's some stupid whitespace rule, it should be a one button click on my IDE from some provided linter to resolve (even if I will proceed to re-lint it to my style afterwards on my local machine).

2. style sheets can have suggestions as well as laws. Be reasonable on what is what

>For some people, they want layers.

sure, lawful evils exist, both maliciously and inadvertently. Sounds like you ran into both kinds. Office politics are inescapable even at the best companies.

That sounds like another discussion to adjust to style sheet, not throw it out upright. If you see a bunch of examples of a law being broken, adjust it to a suggestion unless is a strong argument made otherwise. (most) companies aren't a congress where amending such things takes months of proposal, and we can very much adjust the document to the people if there's no resistance.

Now, do you actually care enough to spearhead that change? I imagine many don't, and there in lies the problem. That's why I'm not on management track; I don't have the care nor attention to worry about documenting such things unless someone throws it at me. I'll leave that to those who do care.

To be clear, I'm not talking about code style (easily objectively quantifiable) or bad code (also easily objectively quantifiable), but architecture/design. This can be things like which folder an interface belongs in, where to put a method in a parent/child relationship that touches both, when to use value objects vs. scalars, when to break functionality into another service/library, which methods are allowed to call which methods, which classes must be injected vs. instantiated, etc.

Yeah, gotta love it when someone uses terms like "standard" and "quality" but definitions of those are written nowhere. Quality is almost never objective either; some cultures like "overripe" bananas and some consider the best bananas to have no spots, and those picking bananas need a different basis for what makes a good quality banana to the buyer. Unless programmers diligently document their standards (which they rarely do adequately), they shouldn't be surprised if the other programmers is dumbfounded by being told their code isn't up to standard.

I'd like to agree. But there is one part of that distinction that is real: there is such a thing as low quality code.

I think there is a such thing as “bad code” but not low quality code. Bad code can be detected by automated tooling and be improved through simple refactoring.

I'd say it this way: bad code doesn't robustly handle all the use cases, mis-interprets inputs sometimes, or is a mess to read and debug.

Yes, it matters what the code looks like. Knew a guy, named everything in his code a letter of the alphabet. a,b,c and when he got to z started za, zb etc.

That was 'bad code'. Or, it was 'Low Quality Code'. Nobody wants junk like that. Even if it passes tests, is robust etc.

> Nobody wants junk like that.

I can think of a few tools that do that to your PHP or JS code, on purpose. They even cost quite a bit of money (obfuscation). Sounds like you might have gotten a good deal there ... /s

The alternative is a big design upfront?

Generally, it goes something like this imaginary Slack convo:

Me: I think we are getting events from SQS out of order and that’s why we are seeing some weird synchronization issues. What do you think about sending events to the regular queue and a FIFO queue at the same time and comparing them?

Team: How would that work?

Me: Since we are using a single threaded consumer here, I think we can simply override the transport class and instead of pulling from one queue per transport instance, we set up a feature flag allowing us to pull from two, compare the events and if they are different, alerting us in the logs.

Team: Sounds good!

You don’t need a full design spec, just agreement from the team on the approach. There won’t be any surprises in the review.

> There won’t be any surprises in the review.

There can always be surprises. Sometimes when you see something implemented you can get a « click » as to why another solution would be better. It happens to me regularly both on the receiving and giving end of this and it’s rarely a big deal. Most of the time it happened to me, I could reuse the functional tests I had written and parts of the code anyway.

Reworking on something so that it’s better is never a waste of time, and it’s always better to do it before it reaches Prod.

Making a suggestion on how to do something better is quite a bit different than saying “this isn’t up to standard. Do not pass go. Do not collect $200”

We all see these suggestions. That’s ok, and wanted. But to say “ah, no I think it should be the other way.” [reject]

That’s what I was talking about here.

Kind of off topic but I'm curious what it is about a foreach that you think would do a better job than a syntactical for-loop. In my experience, for-loops are almost always the better choice, unless the implementation of "foreach" is done in a way that is functional and recursive. Foreach in the usual sense (like the method in JavaScript) provides too little control to the callee and is only really of benefit if one really needs to conditionally pass in different callback functions.

Besides, that, I mostly agree with your stance on code reviews. The way that most programmers do code review is so very "waterfall" and they don't even realize it.

> what it is about a foreach that you think would do a better job than a syntactical for-loop

It depends on the language. For example, an optimizing compiler can optimize the shit out of a for-each loop, but can only go so far in a for-loop. For example, in a for-loop, you can "skip around" the indices, and even go backward after going forward, while in a for-each loop, it will always go one-by-one, and the compiler can rely on that fact. Even the CPU can probably optimize it better since the data access will be more predictable.

Not only that, but a code reader can rely on this too. If you only use a raw for loop when you're skipping around, then the reviewer / future reader can slow down to understand what's happening here. If you use raw for loops for the for-each case, now the reader has to slow down on all your for loops. It's similar to the reasoning for "const everywhere": if you don't mark variables const, I need to figure out as a reader whether the variable is modified later, and that slows me down and increases the mental load of reading a function.

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.

> Be extremely picky for PRs from new hires, so that they do things the way that your company does them.

I think the word you are looking for is "hazing":

> haze (v): force (a new or potential recruit to the military or a university fraternity) to perform strenuous, humiliating, or dangerous tasks.

I had this at my current job. I almost quit because it was downright humiliating to get called an "idiot" in so many nice words. I don't do well with hazing... so I brought out quotes from text books, papers, and famous authors to point out how wrong they were.

No, the word is called "alignment"

>an arrangement of groups or forces in relation to one another

If you're being harassed in PRs the peer review system has failed. It's a place to make sure everyone is on the same page, not judge someone for how they code.

They are not suggesting hazing. I don't doubt that you experienced hazing. Being vigilant with new hires to assure assignment on code quality and design is not hazing though.

If someone has legitimate concerns about the design decisions made, then they should voice them. However if they are refusing to adhere to guidelines, simply because they dislike the approach then that's being overly problematic.

It’s literally the definition of hazing. But instead of being asked to jump in a pool, naked, while snowing, you are asked to build things a new hire has no business building. Then nit-picked for not knowing things. Literally set up for failure.

A better solution is to actually sit with them while they build a feature, show them around the code, and answer questions. You know, treat them like a team member instead of making them prove their mettle.

There is no hazing because there is nothing personal here. You are not being judged. The code you wrote is, but feedback in a code review doesn't say anything about your competence (though how you respond to that feedback says a lot).

If your team is insulting you or saying that you're a bad engineer based on code review comments, that's a bad team. That doesn't mean that ensuring new team members learn the team's style and patterns is hazing.

I'm sure that is what the frat boys would say when they tell everyone to jump in the pool, in a blizzard... it's not personal.

If you don't suck it up, you're not "one of us" and you need to go. Or maybe they told you to do the wrong thing and see if you point it out. Who knows!?

I definitely didn’t mean it like hazing.

I don’t see what’s so wrong about saying “you used inheritance for this relationship, but we have a pattern of keeping classes like this separate, since this system tends to change frequently. Please organize this like xyz module instead.”

Just a random example of something that a new person might do who is unfamiliar with xyz module and the complications there.

I agree that it’s good to mentor someone new, but honestly I think they still make most decisions themselves, and sometimes those decisions don’t match established patterns that they don’t know about. Ideally you notice sooner than a PR but I also think that people get busy and it’s ok to not have time to monitor everything a new person does. So sometimes it comes down to the code review to notice.

It’s not derogatory or hurtful, literally at all, it’s just pointing on that they did something in a way that goes against established patterns, and it’s teaching them what those patterns are.

My response would be a simple "Why does code changing frequently prevent inheritance from being used?" if I got a comment like that. Granted, I don't like inheritance, so I have nearly 1000 arguments on reasons not to use it that has nothing to do with code changing frequently, so ... this is probably a bad example for me, personally.

But seriously, I'd ask why, and why again. I'm very much against cargo culting, and I will refuse to make changes in my PR if it is cargo culting. You're welcome to open a PR to my PR with changes if you feel strongly about it though.

Maybe this makes me hard to work with, but so far, I feel like it has led to better code and a higher velocity, everywhere I've worked.

Team conventions are not cargo culting.

Change is bad unless it's great. Being able to look around a codebase and know how things work because similar coding styles and patterns are consistently followed is a huge productivity boost (this is also what commenters complaining about the idea of readability elsewhere in this thread are missing). Something must be 10x better to compensate for diverging from those patterns.

What you write is not YOUR code. It is your TEAM'S code, and the good of the team is far more important than what you personally like.

Heh, if that's your reasoning on why something should be the way it is, then that is what it is. Somewhat reasonable, but don't be surprised if any reasonable person quits that day. The argument is not grounded at all in computer science or anything else objectionable. It doesn't allow the team to grow and change what is in front of them every day and forces them to live with old mistakes forever. Doesn't sound like a good place to work. You don't get to a 10x solution overnight, in a single PR, you get there in increments.

I also disagree with it being "the team's code":

What you write is your code (and copyright law almost universally backs this up), what gets merged is a maintenance burden for all time. It very much matters what you like and don't like, and it very much matters that it is maintainable (whatever that means).

The team ... doesn't matter when it comes to code conventions ... they'll all be gone and moved on to other parts of the code/company/industry outside of five years. Most code lives long beyond today's team.

I've worked with some amazing engineers, including multiple whose blog posts regularly get posted here. None of them have egos or consider code review feedback personal attacks.

It's not like you're being told to never use for loops. Code review feedback such as the following is all objective, beneficial to the reader, and not worth pushing back against:

* This logic should live in [other component] * Our RPCs are named as GetFoo, not DetermineFoo * That function name does not make it obvious what this code does, please change it * This needs a test * This is untestable and needs to be refactored to support X

If anyone on my team ever described themselves as in "a war of attrition" with another teammate I'd fire them.

> If anyone on my team ever described themselves as in "a war of attrition" with another teammate I'd fire them.

I wasn't sure if I was going to reply to this part or not. But you took a comment out of context here.

I'm incredibly touchy about code reviews. I used to have the popular opinion here where "the code belongs to the team!" and all that. And then I was stuck on a team with a very toxic guy who weaponized this stuff. His code was "perfect" and he'd approve your PR to turn around and rewrite it just to show how much better it could be (and gloat about it -- not realizing if he hadn't deleted half the tests, they'd be failing). He'd constantly point out all the things that were "wrong" and with the justification of "code quality" without actually being able to point out why it was better. After working with them for two years, the entire team's philosophy turned into the one I've shared here ... because it can't be weaponized. It can't be used to tyrannically rule the code. It can't be used to bully people around "for the sake of quality!"

His "code quality tyranny" did more damage to the code than any actually bad code could have done.

So yeah, I will go into a war of attrition over certain kinds of comments that are spoken authoritatively yet are entirely opinionated ... because I've seen what happens when it gets out of hand. I have no desire to go through that ever again. Ever. If that means someone like you fires me for it, it's better than going through that shit again. Sorry, not sorry.

You experience one extreme ans then decided to take the opposite extreme stance. I recommend being more pragmatic.

Eventually, sure.

I'm also starting to think this is how the Paradox of Tolerance happens. I'm intolerant of intolerance these days. I'm much more tolerant of people having their way of doing things than I was just 10 years ago. However, as soon as one person comes along and says "It MUST be this way because QUALITY!" I go into intolerance mode and don't tolerate it.

Now, if you can give me a reason grounded in computer science, or whatever paradigm we're using (DDD, hexagonal arch, actor-model, etc), then I'm perfectly happy to agree or spend a few minutes over a coffee discussing merits. But if the reason is 'quality' ... it's a "dual to the death."

I see. I want to try and understand this because I am also trying to get better at code reviews and not come across as a dogmatic person.

I have spent almost 15 years in mostly AWS and I want to keep myself in check and make sure people don't take my suggestions as the vague "quality" as you so mention just because of my seniority.

Here is the most recent PR I did for a relatively young person in my org. Part of the PR was doing a type of snake_case to CamelCase conversion from some user inputs to some enums classes. It was done manually with String manipulation and playing with indexes etc. It also came with a bunch of unit tests to make sure the conversion was working correctly.

I put in a comment along the lines of "hey, this conversion seems like it can be done with this Apache Commons Text library; given that we are already using it in the same project in some other place, we can remove this manual code and call the library function; we can even remove the tests assuming the well-tested library is doing the right thing".

Was this comment warranted? If you think this is a reasonable comment, what computer science principle would you say it is based on?

There are three parts to every code review:

1. Code style: such as formatting and when to use certain things (non-negotiable and you really should automate that).

2. Working code: does the PR have a description/ticket and does the code do what it promises to do? Can we refactor anything to make it better?

3. Conventions: does the PR have tests when necessary, are there negative and positive tests? Does it pass those tests? Is there anything against the conventions? If so, bring it up with the dev and find out why, outside of the PR. Maybe they didn't know the convention or there is some legitimate reason for it. Don't be an ass and call them out for it on the PR in front of the whole team, give them the benefit of the doubt and if someone else comes along and calls them out, you can present a united front vs. forcing them to defend themselves all alone.

In this particular case:

Suggesting a library should have been done before the code was even written. At this point, I wouldn't even suggest using it without some homework for the reviewer and look at the implementation in the library.

If the library implementation covers cases not found in the code I was reviewing, I would suggest looking at that library for some inspiration and/or switching to it. I would emphasize switching to it if it is far and away from the library code; as time could be better spent elsewhere. If the new implementation looks better than the library one, I might even suggest refactoring the entire codebase to use the new implementation instead of the library and/or spending some time to open a PR with the original library and improve it.

The work has already been done. Try to capitalize on it, instead of dismissing it.

Seriously though, look at the library implementation before moving forward. I've seen some very popular libraries and frameworks be very poorly implemented in some areas but because "it's popular" people seem to think it is better. That's not always the case.

>The work has already been done. Try to capitalize on it, instead of dismissing it.

Except it hasn't at all. This is the attitude of people who send pull requests to open-source projects and complain that they aren't accepted.

The work is integrating that code with the rest of the system, ensuring it's compatible with future plans, ensuring that it's scalable and secure, and ensuring that the whole team can understand and maintain it.

Successful projects don't just take any code that compiles and passes the tests, they are built in a thoughtful manner with high standards.

> This is the attitude of people who send pull requests to open-source projects and complain that they aren't accepted.

To be honest, I’m not sure how you got there from here…

> The work is integrating that code with the rest of the system, ensuring it's compatible with future plans, ensuring that it's scalable and secure, and ensuring that the whole team can understand and maintain it.

I actually suggested deprecating the library. I am not going to presume how hard that is. It might be changing 2 lines of code or 1000. No idea. That’s why we have brains though, we are able to figure these kinds of things out. As far as ensuring it’s compatible with future plans… do you honestly think a “random” maintainer of a library has your best interest at heart? That’s rather unlikely unless the maintainer happens to work for the company in question. In this particular case, it looks like someone volunteered to be that maintainer (probably due to naivety, but that’s beside the point) and they actually do have your future interests at heart.

So to say that an employee doesn’t do work aligning with their company’s goals means you have bigger (political) problems than code review problems, hence why I said they should never have started working on it in the first place. For them to work on something like this for someone to review and professionally say “wtf” is a red flag, far and beyond code reviews; which is interestingly another reason why it is waaay too late to be having these conversations in a PR. It’s a smell that something in the team is fundamentally broken.

>To be honest, I’m not sure how you got there from here…

"I've done all this work, get put of my way and submit" is the attitude in both cases.

Congratulations, you wrote some code. It's not done until it meets team/project standards, and getting offended any time anyone has feedback makes you someone I'd never want to work with.

The default answer to code review feedback is always "sure" unless you have a strong argument otherwise. It takes way less time to implement feedback than to pretend you're Socrates and question everything.

> "I've done all this work, get put of my way and submit" is the attitude in both cases.

That’s quite a stretch. Nobody should be “in your way” to begin with. In a company, you are on a team and the point of a team is to work together; not get in each other’s way. That’s a sign of a dysfunctional team, which is how you end up getting team members working on the wrong thing or implementing things wrongly in the first place. Code not up to “standards” is not a reason to reject code, it’s a sign that something else is wrong on the team and that is the thing to be addressed, not the code. You aren’t going to resolve it in a code review.

I don’t know how else to say that at this point.

> The default answer to code review feedback is always "sure" unless you have a strong argument otherwise.

I’ve definitely pointed out why a deleted line needs to be kept. I would not be surprised if that someone came back to tell me why it should be deleted. As a reviewer, I lack a lot of context even if I originally wrote the code. I’d never, ever, expect someone to blindly say “sure” on a code review. I don’t write perfect code and neither do my team members, so I don’t expect a perfect reviewer.

A team works together to make the code perfect, nobody is writing mystery code and doing weird stuff without discussing it with the team first. And sure, people experiment and we review it, but with an architectural lens. Then when we are happy with the architecture, we actually implement it, together.

> Suggesting a library should have been done before the code was even written.

Unless you agree on every detail before implementing anything, I don't see how this is practical. At least not in the companies I have worked in.

> The work has already been done. Try to capitalize on it, instead of dismissing it.

Not quite though. The developer may have written down some code but the work is not done. It needs to be shipped to production, monitored and supported by the oncall etc. If there is a way to not write some code and use a library, I will suggest that.

> Don't be an ass and call them out for it on the PR in front of the whole team, give them the benefit of the doubt and if someone else comes along and calls them out, you can present a united front vs. forcing them to defend themselves all alone.

If I see a PR with some key tests missing, I don't see why asking if we can add some more tests would be seen as calling them out on it. The PR is a place to record such things - may be it does not need such tests or may be it is intentionally not handled in the code etc. Why would such a discussion be seen as someone being an ass?

Why do I have to have that perfectly normal discussion in secret away from the rest of the team? We can ask questions and still be professional. Code reviews are not just for making sure code is good, but also for education - it is a good way for the other to know what is going on and also learn.

My read on this is that may be this is what happened to you - someone was being personal and attacked team members personally in the guise of a review and now you take a stance that either discuss everything before implementation in person or have separate meetings in private to suggest changes or ask questions.

I am thankful I did not have such a colleague/mentor when I started and hopefully I am not inflicting such an attitude to the new folks that are coming in now.

> I've worked with some amazing engineers, including multiple whose blog posts regularly get posted here. None of them have egos or consider code review feedback personal attacks.

Same. Nor do I consider code review comments personal attacks, but anything that must be changed for "reasons," must be questioned. Not because it's personal, but because I legitimately want to know and I won't give up until I get a good answer.

> Code review feedback such as the following is all objective, beneficial to the reader, and not worth pushing back against

I'll agree that none of this is probably worth pushing back against, but it isn't objective. Very little is objective, in our industry.

> This logic should live in [other component]

Why? DDD will say one thing, MVC will say something different, though usually they are compatible or can be made to be compatible. You can also subscribe to hexagonal architectures and that might say something different...

There's nothing objective about where code should live, except on a hard drive or some other storage medium. I would argue that code probably shouldn't live on paper. I imagine most of us would agree with that.

> Our RPCs are named as GetFoo, not DetermineFoo

This is a nit. I'd honestly probably ignore it.

> That function name does not make it obvious what this code does, please change it

I'd probably ask for a suggestion because it probably looks obvious to me after staring at the code for so long. That being said, it might be a valid suggestion, especially if the code was refactored, but wasn't renamed.

> This needs a test

I hope nobody ever says this on my code reviews. However, I don't write tests for "obviously correct" code (code where the test implements the logic to test the logic): such as a function like:

    function returnTrue(): true { return true; }

If I see code that changes "obviously correct" code, then a test is warranted.

> This is untestable and needs to be refactored to support X

Do you know there is a such thing as legitimately untestable code (or at least, it shouldn't be tested in traditional unit tests)? Usually at the edges of two systems. For example, an API integration can't be tested fully, only the known contract from the other system. Then you start running into Postel's Law ... things get weird. Only if you have some kinds of guarantees with the other system (not usually), would I recommend traditional tests.

>> Our RPCs are named as GetFoo, not DetermineFoo

>This is a nit. I'd honestly probably ignore it.

Congratulations, until the end of time your team now has to remember multiple naming conventions, and when they want to do a code search for the RPC that retrieves foo, if they search for "GetFoo" because all the other RPCs are "GetBar", "GetBaz", and "GetTaco", they'll find no results and be confused.

"The team" is not a static thing, it is an evolving team-of-Theseus. Consistent naming and coding styles make it easier for the team, until the end of time, to read and search code. These "nits" are productivity multipliers.

As other reviewers have called out, I am a fan of "LGTM with comments", but any teammate who starts ignoring comments loses the privilege of receiving LGTM with comments from me and gets to wait until I actually review a version of the code that is acceptable to the team.

> until the end of time your team now has to remember multiple naming conventions

Do they not have PR's where you are from?

If someone is that annoyed by it, just open a PR. Just because I don't find it annoying, doesn't mean someone else won't. And vice-versa.

The code is mutable. It doesn't have to be perfect on the first iteration.

I will say this though: if you want me to do your nits, you also have to do mine. It's a two-way street when it comes to nits. If you don't do my nits, then I won't do yours. It's pretty simple. If you start withholding approvals because I stop doing your nits, then yay ... politics, I guess.

The code is mutable, but the cost to fix it during the original code review is WAY lower.

Once you land RPC DescribeFoo, I have to:

1) Define a second parallel RPC GetFoo

2) Wait for it to roll out to prod

3) Update all callers to call GetFoo, probably controlled with an experiment for safe rollover

4) Wait for all of those to roll out

5) Clean up all the callers

6) Clean up the DescribeFoo RPC

.....or you could just fix it in code review with half an hour of work.

I thought it was DetermineFoo?

Anyway. To me, DetermineFoo doesn't sound like a getter and seems like it might even possibly mutate some states. So, I suspect creating a GetFoo would be orthogonal to deprecating DetermineFoo. This means DetermineFoo might only Get a Foo by side-effect. That's why I would probably ignore it; simply because DetermineFoo might be a better name, in my opinion.

New behaviors occur all the time in software, but it doesn't mean we need to be rigid with naming things.

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.

So, when is the right time?

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.

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”.

Your comment is why I highly recommend using conventional comment 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?"

> 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.

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.

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.)

Everyone thinks their task is the most important one. If the "lock OS until review done" thing were a thing, there's a good chance that "lock OS until random bullshit someone else wants you to do" would keep you from sending out the code for review in the first place.

And even though it's "just a quick thing", if every time you're trying to get something done some "quick thing" gets in the way, it also gets incredibly frustrating. [1][2] Finding a good balance for this is hard.

What helps in my experience is building a reputation for small, easy to review changes. I know that I've left reviews from one coworker sitting for a long time because I either already took a look and knew that it was, or hadn't taken a look and dreaded it to be, a huge block of code that would require significant comments.

[1] https://www.reddit.com/r/ProgrammerHumor/comments/2rmir6/why... [2] https://www.reddit.com/r/ProgrammerHumor/comments/pafo1v/und...

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.

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.

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.

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.

> I have Github connected to Slack; if someone needs my review I get a message and can have it done within 5 minutes.

Do you not do anything else? If I'm working on something else, I'm not getting to any new task within 5m (at least, not without a net loss of productivity to reload context)

If you're writing code and you get a review request and ignore it, you are literally preventing value from being shipped while you work on making some abstract idea a reality.

Usually, the code review is the very last step before value becomes available to the business. There is absolutely no reason to delay a code review in this case, none, whatsoever.

Even if it isn't the last step, you're delaying the chance for a developer to continue working. More often than not, when a PR is submitted, there is a period where the submitter is twiddling their proverbial thumbs to prevent context switching from a reviewer.

So, delaying a review prevents value from being available and reduces overall developer productivity for the entire team.

Or, keep working on making an imaginary idea into a real one while someone else's already built idea sits there wasting money.

> Usually, the code review is the very last step before value becomes available to the business. There is absolutely no reason to delay a code review in this case, none, whatsoever.

Even if I was just working on another feature, that's questionable, because it only works if earlier-stage work might not ship at all; otherwise a delay at the beginning is the same as delay at the end, and overhead from context switching means you should prioritize whatever you're already doing.

But where it really gets absurd to claim that code review is always the highest possible priority is that I never said what I was working on at the time. Maybe I'm working on incident response - "Oh, sorry $BIGGEST_CUSTOMER, I know your prod instance is down and you're losing thousands of dollars per minute, but I just have to pop off for a bit because one of my peers needs me to code review a way to make forms load 0.2 seconds faster!". Maybe I'm working on a different feature, but it's one that will give the company more value (I have been in conversations to the tune of "we need this feature so we can close with this lucrative customer"). Maybe the other thing I'm doing is someone else's code review!

Now to be fair, I think there is real value in lowering the turnaround time on reviews. I could even see an argument for having someone who does code reviews to the exclusion of all else (which kinda sounds like the role you're playing in your company), provided that's known and factored in when handing them work. The only reason I think your position is unreasonable is that you've thrown out any notion of triage or nuance and pinned reviews to the very highest priority above all else.

I hear you, and I agree, somewhat. Hell, my wife calling me is probably more important to me than a code review.

That being said, I do make doing code reviews _the_most_important_thing_, except that I make it utterly transparent when I check for code to review (first thing in the morning and after lunch). I also make it clear that I'm only spending 30 minutes (max) on the review. If I can't review it in less than 30 minutes, I'm leaving a comment that the PR is too big and needs to be broken up and then not reviewing it (maybe someone else on the team is willing to review it).

That's my policy and the team agrees. Other team members have their policies as well (some won't review until they are also waiting for a review, some simply don't want to do reviews at all and only begrudgingly review and some are very similar to mine).

I think it is ultimately a conversation the team must have to have predictable releases and deployments. If you are doing nothing but code reviews for a couple of days before releasing ... you're going to have some fun integration issues and need quite a bit of manual testing.

>There is absolutely no reason to delay a code review in this case, none, whatsoever.

"I'm in a meeting with business owners". There, now either value is delayed or egos from the ones gaining value is bruised. Guess which one usually prevails?

We don't have to be this dramatic here.

PR review is an async process, you can't expect your teammates to drop what they are working on in order to review a PR. This is something you can bring up in your stand-up or message channel if the PR has been sitting without a review then kindly ask "hey X, and Y can you give this a review today". Or if it actually is urgent then message someone directly and say it's urgent, and offer to hop on a call to go through the review together so the feedback can be immediate.

Actual priority items are always owned by the whole team, not an individual.

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!

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.

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

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...

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

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.

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.

> 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.

Style wars are the kind of thing that should just be automated away with a source control hook that reformats all submissions.

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.

100%, extra points for fitted penname.

> 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

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.

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.

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.

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.

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.

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.

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.