[bind10-dev] code review style

[JINMEI Tatuya / 神明達哉]

At Mon, 14 May 2012 09:09:58 +0800,
Kevin Tse <xiejiagui at cnnic.cn> wrote:

> 	I have encountered a problem how we do code review, and I think there
> may be some thing changed for our code review style. Because some times
> the code review made me confused.There may be an Explicit style
> documentation for it like code style documentation.

Do you have any specific suggestion?

BTW, we do have documentation about code review process:
http://bind10.isc.org/wiki/CodeReviewProcedure
although I suspect it's not read/honored much and maybe even stale in
some points.

Regarding this:

> "
> The most common mistake in code review - the mistake that everyone makes
> when they're new to it - is judging code by whether it's what the
> reviewer would have written.
> Given a problem, there are usually a dozen different ways to solve it.
> Andgiven a solution, there's a million ways to render it as code. As a
> reviewer, your job isn't to make sure that the code is what you would
> have written - because it won't be. Your job as a reviewer of a piece of
> code is to make sure that the code as written by its author is correct.
> When this rule gets broken, you end up with hard feelings and
> frustration all around - which isn't a good thing.
> "

In essence I agree with what's written here.  But I'm not sure how we
could improve/change our way of reviewing the code along with this.

I myself am probably sometimes too vocal about *my opinion* about the
code I review, beyond whether the code is *correct* (which is also a
subjective word, though).  My personal guideline is generally like
this:

- Check if the code does what it's expected to do in that task.  If
  not, point it out as a blocking issue.  Sometimes even suggest how
  to fix it.
- Also consider if the code looks *good* to me (whether it's safe,
  intuitive, concise, efficient, etc).  If I have some comments on
  this point, state these, too, generally trying to also explain why,
  and sometimes even suggest how I would have written it myself.  But
  I consider these optional points to make the implementation possibly
  better - the author may or may not agree with my view.  And, these
  are generally not a requirement to be addressed for merge.
- Same for the design, if the task involves new interfaces or some
  architecture level changes.
- In some cases the views of the author and reviewer (me) are so
  different.  I generally first try to find a point where we both can
  agree, but the basic rule when there's no such easy point is to
  respect the author's one; if I still don't agree with it, I may
  bring it to the list for other opinions, but that's beyond the scope
  of the code review.

So, I in fact sometimes include my judgement in my review feedback in
terms of what I would have written it.  But as noted above, its
purpose is the base of post-review discussion, not a request of
changing the code that way, unless, of course, the author agrees that
the suggestion really makes the code better.

This is not a written guideline or not even explicitly shared with
others, but I think other reviewers (in BIND 10) are more or less
following the same principle.

Now, how can we change or improve that?  Can we agree that the above
is what we are generally doing now in the first place?  If so, should
we change that, or should we explicitly written down it somewhere?

---
JINMEI, Tatuya
Internet Systems Consortium, Inc.