
Ask HN: Is there some unwritten code of conduct for code reviews? - MichaelMoser123
Code reviews are all the rage these days: I am getting code reviews at work from people whom I don&#x27;t even know; Is there some accepted code of conduct for this kind of situations? 
I mean is one supposed to just do what any reviewer is suggesting, or is there an acceptable way to resolve differences of opinion?
======
jamessb
Google's engineering practice documentation has a section on 'The Standard of
Code Review': [https://google.github.io/eng-
practices/review/reviewer/stand...](https://google.github.io/eng-
practices/review/reviewer/standard.html)

It includes this important rule:

> In general, reviewers should favor approving a CL [changelist - equivalent
> to a patch] once it is in a state where it definitely improves the overall
> code health of the system being worked on, even if the CL isn’t perfect.

~~~
3minus1
I'm really glad this was posted. The rule that makes sense to me, when before
I didn't know where to draw the line between accept and nitpick. Gonna start
thinking about this in code reviews going forward.

------
noir_lord
In theory a code review should be a conversation in which you reach consensus.

The goal should be that the reviewer understands the code and what it does and
is satisfied with the other things.

In reality what often happens is a busy programmer looks at it, spots that
there is a { in the wrong place, flags that and considers his/her job done and
moves on.

If someone flags something in a way that you disagree/or don't understand the
best approach is "Can you explain why you think your approach is better than
mine and what the trade-offs are?" in which case you'll either get an answer
that is satisfactory and accept their changes or not in which case you then
have to escalate it again.

If you think of it as asynchronous pair programming rather than a chance for
someone to nitpick everything you do then it's more obvious how to respond.

~~~
MichaelMoser123
At what stage of the argument is one going to be flagged as uncooperative?

~~~
noir_lord
At whatever point one party pisses off another in some way.

You can delay that point by been open and reasonable up to a point, if you
can't then reach consensus you need to escalate it to a lead/PM.

It can get both messy and personal if you aren't careful because two
programmers might often solve a problem equally well but differently and
therefore there simply isn't a clear correct answer.

In those cases I'd tend to default to the solution of the person who wrote the
code (simply because they'll have put more thought into each part than the
person doing the review usually).

Team dynamics will largely play a part as well, you may have an incredibly
good programmer who can't take criticism at all well (seen that) or whatever
and then it becomes a management problem rather than a technical problem.

A lot of it is simply tone and empathy though, "I'm curious why you did A
instead of B here?" against "Why the hell didn't you do B?" or "B is clearly
superior!" (with no explanation of why or the trade-offs).

Asking clarifying questions is often a good way to drill down to the meat of
the problem (and in a good code review environment imo one of the strongest
benefits, it increases comprehension of the code and intent for all parties).

One of my lecturers used to say "there are no technical solutions to a people
problem".

~~~
MichaelMoser123
Thanks! So one possible answer is: try to reach the other party directly as
soon as possibles (directly so that the discussion is not logged as responses
to the code review log), this might help to reach consensus without stepping
on anyones toes, so to speak. (and before the other party feels like it wants
to escalate the issue) I think it is better to keep the discussions like this
off the record, because the other party might not feel confident to formulate
his/her position in a formal manner or might feel insecure about having this
kind of paper trail .

------
codingdave
There isn't one standard answer. The overall idea is to get another set of
eyes on any code before it hits production, to help catch places where it
could be improve, catch bugs early, help each other find better ways of doing
things, etc.

In practice, some teams do this very well and it an ongoing discussion of good
practices that makes everyone better. Other teams nitpick over trivia, asking
everyone else to code it like they would have. Some teams require an actual
approval, others just toss all the code up for a while before merging it
whether or not it got reviewed. Sometimes a lead reviews it all and say
yea/nay, other times it is a teaching tool to expose people to new areas of
the code before asking them to help out in that area.

In other words, no - there is no standard code of conduct across the industry.
But there should be some common understanding of how your team, specifically,
is doing reviews, and what expectations exist.

------
olingern
Code review, IMO, is specific to an org and how it wants the culture and
codebase to mature overtime.

Careless, quick reviews are going to foster an environment where PR authors do
not feel adequately supported where as pedantic, nit-picking reviews are going
to exhaust.

As for the semantics, it's often about being idiomatic in the language and
adhering to good design, i.e. if some requirement changes -- can this change
with it?

