[openstack-dev] Code review study

Russell Bryant rbryant at redhat.com
Tue Aug 20 15:24:40 UTC 2013


On 08/20/2013 11:08 AM, Daniel P. Berrange wrote:
> On Tue, Aug 20, 2013 at 04:02:12PM +0100, Mark McLoughlin wrote:
>> 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.
> 
> Agreed, lines of code is a particularly poor metric for evaluating
> commits on. 

Agreed, I would _strongly_ prefer no enforcement around LOC.  It's just
not the right metric to be looking at for a hard rule.

-- 
Russell Bryant



More information about the OpenStack-dev mailing list