Re: Thoughts to keep in mind for Code Review

2014-06-27 Thread Wayne Witzel
I also found this useful. This is specific to Go and more about the details of the code itself than the abstract of the review as a whole. https://code.google.com/p/go-wiki/wiki/CodeReviewComments On Wed, Jun 25, 2014 at 9:31 AM, Gustavo Niemeyer gust...@niemeyer.net wrote: Thanks,

Re: Thoughts to keep in mind for Code Review

2014-06-25 Thread Jeroen Vermeulen
On 2014-06-25 09:43, roger peppe wrote: About pre-review annotations, I agree with Ian that the code should be documented well enough that someone coming to it from scratch can understand it, but I also wonder if there is a room for review-specific comments, talking about reasons for the

Re: Thoughts to keep in mind for Code Review

2014-06-25 Thread Matthew Williams
I agree that the code needs to be self-explanatory enough to not require annotations, but annotations can be useful - especially for larger changes. Suggesting the order for code to be reviewed is certainly useful if you're reviewing code in a part of the system you aren't familiar with On Wed,

Re: Thoughts to keep in mind for Code Review

2014-06-25 Thread 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.

Re: Thoughts to keep in mind for Code Review

2014-06-25 Thread Gustavo Niemeyer
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

Re: Thoughts to keep in mind for Code Review

2014-06-25 Thread Gustavo Niemeyer
Thanks, John. Several nice ideas there. I especially like the data backing the first few points.. it provides evidence to something we intuitively understand. I also wrote some points about this same topic, but from a slightly different perspective, last year:

Thoughts to keep in mind for Code Review

2014-06-24 Thread John Meinel
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

Re: Thoughts to keep in mind for Code Review

2014-06-24 Thread Jesse Meek
+1 on annotations. Would a tag be useful to disambiguate from comments intended to stay in the PR? On 25/06/14 16: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

Re: Thoughts to keep in mind for Code Review

2014-06-24 Thread Ian Booth
-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: