HN Reader

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

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.

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.

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.

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.

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

Is anyone else using conventional comments

https://conventionalcomments.org/

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.

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

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.

This is actually good article.

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.

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.

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

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?

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.

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