Thoughts to keep in mind for Code Review

[Richard Harding]

We've tried to encourage what we call 'reviewer comments' that are intended
to be to help the reviewer follow the code. There are definitely two chunks
of things that tend to get written. It's quite often to find a reviewer
comment that the reviewer then suggests is put into the code itself.

However, there's value in comments that help explain how the system works
in the around the code touches. Not everyone is an expert on every section
of the code, and seeing a review diff doesn't always give you enough
context to really understand why the change helps. It's particularly useful
in the case of drive-by fixes to help note "This is a drive by, not part of
the bug, etc".

I'd argue it's very valuable but true that often some of the comments
belong in the code.

Rick

On Wed, 25 Jun 2014, Ian Booth wrote:

> -1 on annotations. If you need to annotate to make it clearer then that should
> be done as code comments so the next poor soul who reads the code has a clue of
> what's been done
> 
> On 25/06/14 14:20, John Meinel wrote:
> > An interesting article from IBM:
> > http://www.ibm.com/developerworks/rational/library/11-proven-practices-for-peer-review/
> > 
> > There is a pretty strong bias for "we found these results and look at how
> > our tool makes it easier to follow these guidelines", but the core results
> > are actually pretty good.
> > 
> > I certainly recommend reading it and keeping some of it in mind while
> > you're both coding and reviewing. (Particularly how long should code review
> > take, and how much code should be put up for review at a time.)
> > Another trick that we haven't made much use of is to annotate the code we
> > put up for review. We have the summary description, but you can certainly
> > put some inline comments on your own proposal if you want to highlight
> > areas more clearly.
> > 
> > John
> > =:->
> > 
> > 
> > 
> 
> -- 
> Juju-dev mailing list
> Juju-dev at lists.ubuntu.com
> Modify settings or unsubscribe at: https://lists.ubuntu.com/mailman/listinfo/juju-dev