Hacker News Reader

Comment by marktangotango | Hacker News Reader

I once worked at a place where about 5 of 15 developers sat on crucible all day fellating each other on code reviews. Anytime the rest of the developers would have code reviewed they'd be met with a long list of required changes that where "standards" the 5 had agreed on, and never communicated out side of their own comments in the tool. And the "standards" changed often, and the changes where never communicated either. Besides being a toxic culture, it was a real drag on development velocity to say the least.

Code review culture is so important, and so often completely disfunctional. I've quit jobs because of toxic code review cultures like above, and one of the main things keeping me at my current job is sane code review culture we have.

View on HN

It's a good way to game performance reviews.

What makes you call your current review culture “sane”? I’m not sure I have seen that yet in my career.

YMMV but i work at a place where

* code review is expected responsibility, so everyone participates in every part of it regularly, so they are also incentivized to keep the process sane

* we have an auto linter and we recommend saving on fix specifically so no one argues about useless style nits

* CR back and forth is measured in minutes or hours so you are not waiting days to resolve someone’s drive by comment

* CR feedback always has a specific action item that is easy to address

* reviewees submit smaller CRs which are quick and easy to review for reviewers

> * CR feedback always has a specific action item that is easy to address

Then CRs are pretty much pointless. The feedback I want as a senior developer is the complex stuff and that is half of the time not easy to address. The trivial stuff I usually, but not always, spot myself when checking the code before sending it for a review.

Reviewers are responsible for not just pointing out issues, but also providing (at a minimum) some form of direction, or (more ideally) one or more explicit suggestions as to how to resolve those issues.

This is an essential component of a productive code review culture.

Yeah, a good review must explain why, and should ideally explain how it should be instead if it needs explaining.

* This code should be changed looks bad - not a good comment

* This code should be chabged because ten nested ternaries gets hard to read - better

* This code is hard to read because there are ten nested ternaries. Can we replace it with a helper method that returns one value using if blocks? - best, in terms of actionability

You ideally want that earlier and more high level than a code review.

If you are doing system/algorithm design in the code review, it’s not meant for that.

The action item can also be “can we create a issue to track and discuss this further”

This right here! It makes for a sane, friendly culture, and junior team members also get to learn during the code review process.

Prioritize code reviews over development work. Have SLAs. If you are assigned a CR, get to a good stopping place, pause your work, do the CR, and resume. Close CRs that have become stale due to submitter abandonment.

+1, every PR should ideally be reviewed within a day, and certainly within a week.

Reply on news.ycombinator.com