Hacker News Reader

Comment by davedx | Hacker News Reader

As someone who worked on code bases with sometimes fairly big teams before the days of mandatory code reviews: I applaud this. I feel like the culture of code review has become way too serious and strict, to the point we need a “Nit:” syntax because yes, people take issue with the very biggest and very littlest parts of your contributions these days.

Everything has to be perfect. Everything has to be “clean”. It can really get quite stifling, especially when you remember a time before code review when working, high quality software was somehow still delivered.

View on HN

At my workplace all code review comments must be classified as either "blocking", which is stuff like "this code will break the app for our users" or "this is insecure", or "non-blocking", which is stuff like "this method is hard to read and should be broken up" or "I think this variable could be named better". Non-blocking stuff is (really, truly) optional to address.

IMO this system works pretty well as a balance between "we want to make sure nothing catastrophically bad hits production" and "we don't need perfect code, just good-enough code". It does require that you have a general understanding on the team that the purpose of the code is to deliver value to your users, and does not have to be perfect to be valuable.

We did also at one point try a system with more granular classification where every comment had to be prefixed with one of a predefined list of things like "nit", "security", "performance", "comment", or "kudos", which was fine but I don't think really adds all that much relative to just "blocking / non-blocking".

> "I think this variable could be named better"

I had a friend describe to me how hurt he was because the code reviewer didn't like how he named things. Apparently he built a giant metaphor for his system. And he then wrote code in that metaphor.

I don't recall the details. It involved fruit, trains, clippers, etc. He was writing a system to report on how disk space was used across their systems.

Names are important for understanding code. They are most useful if everybody uses the same conventions. It sounds like your friend was trying to push personal conventions onto others.

Did he eventually realize that his naming system was confusing and could be cleaned up?

https://conventionalcomments.org/

Yeah, it was that list of prefixes. Thanks.

This sounds good. It all depends on your engineering culture at the end of the day.

Not sure of your personal experience with this but mine was that the era before code reviews was mostly enabled by having often large teams of QA / SDETs who were burdened with finding the output of bad code and throwing bugs back at the eng team (see: the famous account of winword engineers pushing known bugs into the code to increase perceived velocity with the expectation to get a QA report back to fix them).

That era also required more documentation effort since knowledge wasn't shared at the time the code was written and best practices weren't as well enforced around making sure code is readable since there was never a "test" of someone else looking at it.

Kind of feels like these folks are trying to distribute that QA role across the company (everyone dogfoods the latest versions). I imagine this would work well so long as the team is reasonably small and the product is simple enough that everyone uses every feature of the app often enough to find small bugs and corner cases, but I still wonder if at some point enough issues will arise in prod that could have easily been caught earlier such that someone will eventually say "maybe we should just look at each other's code and catch this stuff before it's committed?"

Would love to see a followup from these folks in another year or two.

Yes and no. If you have mostly senior people who take responsibility for the code they write, then the amount of stuff getting picked up by QA will be more or less the same IME. Code reviews catch some stuff but they can also slow down your team’s velocity disproportionately to the bug/arch issues it catches. YMMV of course. Some orgs have much more sane and pragmatic code review practices than others.

There are two kinds of NITs.

The first kind should really just be linter rules. If it's not important enough to enforce across the project, it isn't worth mentioning now.

The second are strongly held opinions the other person doesn't want to make a big deal about. Maybe they are expecting pushback, or not feeling confident in their explanation of the issue. So they couch it in a nit.

You should be able to push back on comments you disagree with. Often I'll bring up suggestions that I think would improve the code but that I don't think are must haves. I think we can share a tremendous amount of knowledge in code review.

And of course, the easiest way to get fewer code review comments is to thoroughly review your own code with a critical eye. Review it like its someone else's. Find the typos and unclear variables. Anticipate the "nits" that you know are coming.

Most of the Nit level issues should probably be caught by the linter/static checker, which reduces a lot of the minor comments. Not that that seems to stop some people, of course.

I completely agree with you.

There was a time were a software developer was trusted as a responsible adult company employee.

I think that what changed is that, before, devs were mostly experienced trained professionals, and young ones were expected to be seriously trained before being "trusted".

Now, dev work has been commoditized with the influx of "web dev shops" and easy things like js and nodejs allowed a lot of mediocre script kiddies to be considered the same as professional developers.

Nitpicks by devs with incomplete information are the worst. They have little context but end up blocking a PR for weeks because they think it should be solved differently or are not convinced.

After 5 weeks of debates and discussions, the original PR is merged with little to no modifications. But everyone lost time.

Reply on news.ycombinator.com