Agreed, but for a slightly different reason. The suggestion is to annotate the patch with the reason for the change, rather than the code itself, which might indeed lead to a different kind of comment. While this might be useful, one of the interesting outcomes of code reviewing is that it forces the final logic to go through different eyes and mindsets. The "I don't get it" is not always a bad thing in a review.. it's rather the reason why simplifications and entirely different approaches are suggested. Many times I consciously avoid reading an on-going discussion in the review before doing my own review, precisely so I can get a fresh perspective on the code before getting to know everyone else's. Then, with inline reviewing saying "Please tell me why you did this" is very cheap on both ends.
On Wed, Jun 25, 2014 at 1:42 AM, Ian Booth <ian.bo...@canonical.com> 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@lists.ubuntu.com > Modify settings or unsubscribe at: > https://lists.ubuntu.com/mailman/listinfo/juju-dev -- gustavo @ http://niemeyer.net -- Juju-dev mailing list Juju-dev@lists.ubuntu.com Modify settings or unsubscribe at: https://lists.ubuntu.com/mailman/listinfo/juju-dev
