Preferences

I disagree. The point of code review (and indeed, software engineering) is to get at the difference between code that works for its intended purpose (as far its author knows) and code that it's reasonable for us to be responsible for collectively and in perpetuity. That includes things like working in the way you would expect, minimizing surprise, using names that make sense, etc. which are fundamentally subjective and human factors. It's expected that a new team member bounces off some "not the way I'd have done it" type comments for a while until they're socialized into the way that the team or project does things and can reproduce it themselves the first time. Formatters, linters, and documentation can speed up this acculturation process but there is an irreducible degree of "good taste" which you just have to cultivate over time.

That said, it's also important the reviewer feels equally responsible for getting the work done and is willing to make pragmatic tradeoffs taking into account timeline pressure, how contained something is, how likely it is to actually cause trouble, how hard it is to change later. Cross-team and cross-org reviews are more likely to involve pure gatekeeping, which is death.


I appreciate and agree with your comment about taste, but that's not a reason to block a merge. It's a teachable moment, though.

The reasons to block merging code should be explicitly and clearly stated in your Coding Standards documentation.

This item has no comments currently.

Keyboard Shortcuts

Story Lists

j
Next story
k
Previous story
Shift+j
Last story
Shift+k
First story
o Enter
Go to story URL
c
Go to comments
u
Go to author

Navigation

Shift+t
Go to top stories
Shift+n
Go to new stories
Shift+b
Go to best stories
Shift+a
Go to Ask HN
Shift+s
Go to Show HN

Miscellaneous

?
Show this modal