3 ms·

Ask HN: Is there some unwritten code of conduct for code reviews?

Code reviews are all the rage these days: I am getting code reviews at work from people whom I don'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?

noir_lord 7y ago
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 7y ago
  At what stage of the argument is one going to be flagged as uncooperative?
  - noir_lord 7y ago
    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 7y ago
      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 7y ago
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.
jamessb 7y ago
Google's engineering practice documentation has a section on 'The Standard of Code Review': https://google.github.io/eng-practices/review/reviewer/standard.html https://google.github.io/eng-practices/review/reviewer/stand... 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 7y ago
  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.
olingern 7y ago
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?

noir_lord 7y ago

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 7y ago
At what stage of the argument is one going to be flagged as uncooperative?
- noir_lord 7y ago
  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 7y ago
    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 7y ago

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.

jamessb 7y ago

Google's engineering practice documentation has a section on 'The Standard of Code Review': https://google.github.io/eng-practices/review/reviewer/standard.html https://google.github.io/eng-practices/review/reviewer/stand... 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 7y ago
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.

olingern 7y ago

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?