[openstack-dev] [keystone][all] Max Complexity Check Considered Harmful
asalkeld at mirantis.com
Tue Dec 9 23:16:40 UTC 2014
On Wed, Dec 10, 2014 at 8:43 AM, Joe Gordon <joe.gordon0 at gmail.com> wrote:
> On Mon, Dec 8, 2014 at 5:03 PM, Brant Knudson <blk at acm.org> wrote:
>> Not too long ago projects added a maximum complexity check to tox.ini,
>> for example keystone has "max-complexity=24". Seemed like a good idea at
>> the time, but in a recent attempt to lower the maximum complexity check in
>> keystone, I found that the maximum complexity check can actually lead
>> to less understandable code. This is because the check includes an embedded
>> function's "complexity" in the function that it's in.
> This behavior is expected.
> Nested functions cannot be unit tested on there own. Part of the issue is
> that nested functions can access variables scoped to the outer function, so
> the following function is valid:
> def outer():
> var = "outer"
> def inner():
> print var
> Because nested functions cannot easily be unit tested, and can be harder
> to reason about since they can access variables that are part of the outer
> function, I don't think they are easier to understand (there are still
> cases where a nested function makes sense though).
I think the improvement in ease of unit testing is a huge plus from my
point of view (when splitting the function to the same level).
This seems in the balance to be far more helpful than harmful.
>> The way I would have lowered the complexity of the function in keystone
>> is to extract the complex part into a new function. This can make the
>> existing function much easier to understand for all the reasons that one
>> defines a function for code. Since this new function is obviously only
>> called from the function it's currently in, it makes sense to keep the new
>> function inside the existing function. It's simpler to think about an
>> embedded function because then you know it's only called from one place.
>> The problem is, because of the existing complexity check behavior, this
>> doesn't lower the "complexity" according to the complexity check, so you
>> wind up putting the function as a new top-level, and now a reader is has to
>> assume that the function could be called from anywhere and has to be much
>> more cautious about changes to the function.
>> Since the complexity check can lead to code that's harder to understand,
>> it must be considered harmful and should be removed, at least until the
>> incorrect behavior is corrected.
> Why do you think the max complexity check is harmful? because it prevents
> large amounts of nested functions?
>>  https://review.openstack.org/#/c/139835/
>>  https://review.openstack.org/#/c/139836/
>>  https://review.openstack.org/#/c/140188/
>> - Brant
>> OpenStack-dev mailing list
>> OpenStack-dev at lists.openstack.org
> OpenStack-dev mailing list
> OpenStack-dev at lists.openstack.org
-------------- next part --------------
An HTML attachment was scrubbed...
More information about the OpenStack-dev