Re: Thoughts to keep in mind for Code Review
On Wed, Jun 25, 2014 at 2:39 AM, Menno Smits wrote: > I completely agree with Ian's point about code needing to be > self-explanatory and stand on it's own. > > That said, the article mentions that the process of creating review > annotations encourages the author to review their own work in a way that > they may have not done otherwise, eliminating problems before anyone else > even looks at the code. Effectively, a phase of self review happens before > peer review driving the defect rate down and probably making the peer review > more efficient. I've found this to be an effective part of my personal workflow, usually stepping away from the code (even overnight) before doing a self review. Doing it in the review tool sometimes makes sense but in most cases I'll just run through a prettified diff locally and make XXX comments in the respective files. However, review annotations are most useful when you notice something *after* you've published the review request. They're a good way to get focused feedback in that situation. -eric -- Juju-dev mailing list Juju-dev@lists.ubuntu.com Modify settings or unsubscribe at: https://lists.ubuntu.com/mailman/listinfo/juju-dev
Re: Thoughts to keep in mind for Code Review
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 wrote: > 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: > > http://blog.labix.org/2013/02/06/ethics-for-code-reviewers > > > On Wed, Jun 25, 2014 at 1:20 AM, 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 > -- Wayne Witzel III wayne.wit...@canonical.com -- Juju-dev mailing list Juju-dev@lists.ubuntu.com Modify settings or unsubscribe at: https://lists.ubuntu.com/mailman/listinfo/juju-dev
Re: Thoughts to keep in mind for Code Review
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: http://blog.labix.org/2013/02/06/ethics-for-code-reviewers On Wed, Jun 25, 2014 at 1:20 AM, 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
Re: Thoughts to keep in mind for Code Review
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 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
Re: Thoughts to keep in mind for Code Review
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@lists.ubuntu.com > Modify settings or unsubscribe at: > https://lists.ubuntu.com/mailman/listinfo/juju-dev -- Juju-dev mailing list Juju-dev@lists.ubuntu.com Modify settings or unsubscribe at: https://lists.ubuntu.com/mailman/listinfo/juju-dev
Re: Thoughts to keep in mind for Code Review
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, Jun 25, 2014 at 10:25 AM, Jeroen Vermeulen < jeroen.vermeu...@canonical.com> wrote: > 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 changes themselves in the specific context of that review. >> > > There is, I think. But should it be quite so close to the code, where it > competes against commenting for the coder's time? > > Don't know if there's a definite answer, because either way we assume a > human process to complement the technical solution. But if a coder starts > by reviewing their own code, perhaps they should also turn these notes into > a single coherent "cover letter" and, in explaining, perhaps spot > structural flaws or anticipate questions. > > > Jeroen > > > -- > Juju-dev mailing list > Juju-dev@lists.ubuntu.com > Modify settings or unsubscribe at: https://lists.ubuntu.com/ > mailman/listinfo/juju-dev > -- Juju-dev mailing list Juju-dev@lists.ubuntu.com Modify settings or unsubscribe at: https://lists.ubuntu.com/mailman/listinfo/juju-dev
Re: Thoughts to keep in mind for Code Review
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 changes themselves in the specific context of that review. There is, I think. But should it be quite so close to the code, where it competes against commenting for the coder's time? Don't know if there's a definite answer, because either way we assume a human process to complement the technical solution. But if a coder starts by reviewing their own code, perhaps they should also turn these notes into a single coherent "cover letter" and, in explaining, perhaps spot structural flaws or anticipate questions. Jeroen -- Juju-dev mailing list Juju-dev@lists.ubuntu.com Modify settings or unsubscribe at: https://lists.ubuntu.com/mailman/listinfo/juju-dev
Re: Thoughts to keep in mind for Code Review
I completely agree with Ian's point about code needing to be self-explanatory and stand on it's own. That said, the article mentions that the process of creating review annotations encourages the author to review their own work in a way that they may have not done otherwise, eliminating problems before anyone else even looks at the code. Effectively, a phase of self review happens before peer review driving the defect rate down and probably making the peer review more efficient. On 25 June 2014 19:43, roger peppe wrote: > That's very interesting; thanks for the link. > > One part that jumps out at me: > > : ... many teams that review code don't have a good way of tracking > defects found > : during review and ensuring that bugs are actually fixed before the > review is complete > > We don't have this, and with github reviews it's now even harder (for > reviewer > or coder) to verify this by going back to try to correlate code review > remarks with > changes made. > > 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 changes themselves in the specific context of that review. > The code review does not show all the code in the system, and the changes > made will often be made with respect to other areas of the code base - I > can see > how a guide to how the changes fit within the context of the whole system, > and *why* we're making the changes themselves, rather than how the changed > code functions when the changes are made, might be a useful thing. > > cheers, > rog. > > > On 25 June 2014 05: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 > > > > -- > Juju-dev mailing list > Juju-dev@lists.ubuntu.com > Modify settings or unsubscribe at: > https://lists.ubuntu.com/mailman/listinfo/juju-dev > -- Juju-dev mailing list Juju-dev@lists.ubuntu.com Modify settings or unsubscribe at: https://lists.ubuntu.com/mailman/listinfo/juju-dev
Re: Thoughts to keep in mind for Code Review
That's very interesting; thanks for the link. One part that jumps out at me: : ... many teams that review code don't have a good way of tracking defects found : during review and ensuring that bugs are actually fixed before the review is complete We don't have this, and with github reviews it's now even harder (for reviewer or coder) to verify this by going back to try to correlate code review remarks with changes made. 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 changes themselves in the specific context of that review. The code review does not show all the code in the system, and the changes made will often be made with respect to other areas of the code base - I can see how a guide to how the changes fit within the context of the whole system, and *why* we're making the changes themselves, rather than how the changed code functions when the changes are made, might be a useful thing. cheers, rog. On 25 June 2014 05: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 > -- Juju-dev mailing list Juju-dev@lists.ubuntu.com Modify settings or unsubscribe at: https://lists.ubuntu.com/mailman/listinfo/juju-dev
Re: Thoughts to keep in mind for Code Review
-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
Re: Thoughts to keep in mind for Code Review
+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 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
Thoughts to keep in mind for Code Review
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
