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 > [email protected] > Modify settings or unsubscribe at: > https://lists.ubuntu.com/mailman/listinfo/juju-dev -- Juju-dev mailing list [email protected] Modify settings or unsubscribe at: https://lists.ubuntu.com/mailman/listinfo/juju-dev
