On Tue, 2013-08-20 at 11:26 +0100, Mark McLoughlin wrote: > On Thu, 2013-08-15 at 14:12 +1200, Robert Collins wrote: > > This may interest data-driven types here. > > > > https://www.ibm.com/developerworks/rational/library/11-proven-practices-for-peer-review/ > > > > Note specifically the citation of 200-400 lines as the knee of the review > > effectiveness curve: that's lower than I thought - I thought 200 was > > clearly fine - but no. > > The full study is here: > > http://support.smartbear.com/resources/cc/book/code-review-cisco-case-study.pdf > > This is an important subject and I'm glad folks are studying it, but I'm > sceptical about whether the "Defect density vs LOC" is going to help us > come up with better guidelines than we have already. > > Obviously, a metric like LOC hides some serious subtleties. Not all > changes are of equal complexity. We see massive refactoring patches > (like s/assertEquals/assertEqual/) that are actually safer than gnarly, > single-line, head-scratcher bug-fixes. The only way the report addresses > that issue with the underlying data is by eliding >10k LOC patches. > > The "one logical change per commit" is a more effective guideline than > any LOC based guideline: > > https://wiki.openstack.org/wiki/GitCommitMessages#Structural_split_of_changes > > IMHO, the number of distinct logical changes in a patch has a more > predictable impact on review effectiveness than the LOC metric.
Wow, I didn't notice Joe had started to enforce that here: https://review.openstack.org/41695 and the exact example I mentioned above :) We should not enforce rules like this blindly. Mark. _______________________________________________ OpenStack-dev mailing list OpenStack-dev@lists.openstack.org http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
