Open Stack

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[1][2], 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
>     inner()
>
>
> 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.

-Angus


>
>> 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?
>
>
>
>>
>> [1] https://review.openstack.org/#/c/139835/
>> [2] https://review.openstack.org/#/c/139836/
>> [3] https://review.openstack.org/#/c/140188/
>>
>> - Brant
>>
>>
>> _______________________________________________
>> OpenStack-dev mailing list
>> OpenStack-dev at lists.openstack.org
>> http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
>>
>>
>
> _______________________________________________
> OpenStack-dev mailing list
> OpenStack-dev at lists.openstack.org
> http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.openstack.org/pipermail/openstack-dev/attachments/20141210/1b83ccbf/attachment.html>