HN Reader

Good Points. The most important consideration from my experience having worked with more than 40 teams: "Code review is not the time for you to impose your personal taste on a colleague." There are always many acceptable ways how to solve a problem in software development, so if someone imposes his personal preference over that from a colleague this creates tension and a time sink for the team - don't make this a blocking review! This also plays a big role when developers obsess over code but lack the vision and experience to see that not everything will be a relevant factor for technical debt in the grand scheme for the business.

Great article, and I mostly agree.

I want to nitpick on

> Don’t review with a “how would I write it?” filter

_Thinking_ about "how would I write it?" when reviewing code catches a lot of stuff for me - I want to be able to come up with how I'd do it, see how the PR does it, and then check if the approach matches. If not, there's something to be learned and explained here. Although, all of that is in my head! If I come to the conclusion that the presented approach is wrong or inferior in an objective way (say, doesn't handle corner cases neatly or at all), I'll comment. Otherwise, let people do their thing!

So, I agree with the point of not using this to generate a lot of comments, but I do believe in it as a useful technique to engage the brain, instead of just loosely scrolling over the diff.

> For many engineers - including me - it feels good to leave a blocking review, for the same reasons that it feels good to gatekeep in general.

Clearly this is not true for everyone, but I don't think I've ever felt good about gatekeeping, and a number of people I respect generally don't either. Blocking is a weapon of last resort. That someone has had to use it means either that the system isn't working, or they're abusing the tool.

Someone shouldn't arrive at the final phase of their ticket and be told to start it over. That's a retrospective item, and if it's happened multiple times then this is an agenda item at the retro meeting.

I'm a little stricter.

In terms of formatting etc I rely on tooling which break the CI. That way they don't even surface. Equally if something slips through the CI needs to be updated.

A piece of code should match the general style of the code it is being integrated into. When you create an addition to a wood-framed house the addition won't be steel construction, unless you have an explicit reason (weight, speed, foundation etc). And if I'm the main maintainer of a codebase, it is something I can call out on. If we use Tokio and you use raw threads I'll ask about this choice.

In an ideal world you (the PR author) includes this in its description, because it is a reflection of your understanding of the codebase.

Reviewing code with 'will this work' is also debatable. Someone might propose a change that fixes an issue they are having. But maybe it's not something we want to solve in our codebase, or maybe it breaks other things the maintainers are thinking off.

I'd also add: check out the branch locally. Your colleagues do some awful things to their local environments, and the central CI isn't perfect. Your IDE might have more checks enabled than both, too

I've worked in teams where I'm the only one who actually checks out the branch to poke around

I see where this sort of take comes from. Reviews are not a place for subjective comments that just slow down or block progress, signaling, excessive nitpicking, etc. However, the reviewer should be thinking beyond the immediate "hey, this is progress", and also consider how this change affects the codebase and how it aligns with feature and technical direction. A few happy approvals and a great codebase becomes hard to work with, if contributors are inexperienced.

Code reviews are also an opportunity for learning, and provide real-life scenarios to inform decisions that are otherwise not grounded and mostly opinion. Of course, not all comments and discussions should block approval, but there is a balance. No one is going to die if this feature is delayed by a few days if you get a better result _and_ are investing in team growth.

Is anyone else using conventional comments

https://conventionalcomments.org/

In agreement with most of the points, this one was surprising to me:

> Many engineers seem to think it’s rude to leave a blocking review even if they see big problems, so they instead just leave comments describing the problems. Don’t do this. [...] Just leaving comments should mean “I’m happy for you to merge if someone else approves, even if you ignore my comments.”

Do people actually ignore a comment explaining a problem with the code as written just because it wasn't a blocking review ?

It's like ignoring someone telling you you're stepping into a hole because they're not grabbing you by the neck. Reviews shouldn't be that adversarial nor hand holding.

I'm also realizing, everywhere I work a comment is basically a blocking, except if it's explicitely flagged or discussed as optional. Trying to find some other person to review and ignore the comment is just a big NO.

> If tons of PRs are being blocked, it’s usually a sign that there’s too much gatekeeping going on.

I would expand this and say if too many of the blocking statuses are coming from one reviewer, too much gatekeeping may be going on, even if the overall rate is low. Either they're going too far, or nobody else is going far enough. If you're shouting into the void, discretion is the better part of valor. You can't save people who don't want to be saved. Find people who will appreciate your efforts instead of resenting them.

It's not that complicated:

- you review and if to the best of your knowledge you think something can be done better you comment about it and leave a suggestion on how to do it better

- then you approve the PR. Because your job is not to gatekeep the code

I disagree with the point „don’t leave too many comments”. I think it’s easier to read a code that is a) following repo standards, b) following community standards. If a PR requires a 100 comments, so be it. Granted, I also almost always give a benefit of the doubt to the author unless a change is really required and I only ever seen a PR with close to a 100 comments once or twice. I should also say that you shouldn’t leave n comments with the same content so usually there’s no need for more than a couple comments in total.

I think a lot of this stems from the code review tools themselves. Especially the "Don't just review the diff" mistake. Especially with GitHub defaulting to just showing a list of diffs and changed files. I'd find it much more useful if a review tool started with a class hierarchy or similar high level view to get a sense of: 1. What/how many classes changed? 2. What/how many methods have been added/removed/modified 3. What method signatures have changed 4. What changes are covered by new tests

"Don't leave too many comments" i think can really be rethought of, don't review style and syntax. Leave that to the robots. If you're relying on other engineers to flag style problems and linting, you're just wasting everybody's time. Set up linting and style checkers and be done with it.

The thing that always bothers me is when the comments involve things that have nothing to do with the PR.

Like, they are reasonable ideas, but open up a new issue. If every reviewer wants to tackle large topics in the PR Review that have nothing to do with what specifically is happening, then it explodes and gets even harder for others to review now that we are changing things that have nothing to do with the change.

I feel like a useful tool someone should build now that LLMs are so capable, is some sort of automated walk through of a pull request, where it steps a reviewer through initially the overview and they why of the changes, then step by step through each change, with commentary along the way generated by the LLM (and hopefully reviewed by the pull requester for accuracy). Then the reviewer can leave comments along the way.

I’ve always found the cognitive overhead of going through a pull request challenging, seems like the paradigm can shift now with all of the new tooling available to all of us.

There is opportunity in the meta.

If you are leaving a comment it’s because someone didn’t learn how things should be from the tools in your codebase.

Naming style problem? Linting isn’t catching it.

Architecture violation? Your import linting (checkers to prevent route handlers importing the DB and skipping the service layer, etc.) isn’t catching it.

Duplicate method? What tooling can you create in the LLM era - remembering you can ask agents to build scripts now?

> When you receive a review with a hundred comments, it’s very hard to engage with that review on anything other than a trivial level.

Wow, that seems crazy. I can only hope I never have to work with somebody who thinks it is productive to leave that many comments on a change -- I genuinely cannot imagine any change that could ever require that.

Great article, fully agree with all the points.

I'm not convinced code review should be about "consistency" or "system memory" at all. Half of the dysfunction in large codebases comes from reviewers over-optimizing for long-term purity instead of short-term progress.

Code review is the job, in a lot of ways - whether it’s code you wrote, code a coworker wrote, or code generated by an LLM - you are asked “is it okay if this code joins the codebase?)

It’s no small responsibility.

> Many software engineers now spend as much (or more) time reviewing the output of their own AI tools than their colleagues’ code.

This hits home.

> Don’t leave too many comments

I'll say this is directly proportionate to the size of the changes. If a code review is a conversation between n parties, it seems reasonable to me that more code leads to more comments.

This is actually good article.

I work with a person where I know going into a review that I have to check every single file path. They have this weird ability to consistently put things in entirely the wrong folder/namespace. For example, something related to Categories might end up under Orders somehow... it's like new files just go in whatever folder they happen to have open at the time.

I frankly don't even know they do it so consistently.

Something to keep in mind in your reviews as well I guess, lol.

If I see that majority of code is generated, I reject the PR