On Wed, Dec 10, 2014 at 8:43 AM, Joe Gordon joe.gord...@gmail.com wrote:
On Mon, Dec 8, 2014 at 5:03 PM, Brant Knudson b...@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@lists.openstack.org
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
___
OpenStack-dev mailing list
OpenStack-dev@lists.openstack.org
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
___
OpenStack-dev mailing list
OpenStack-dev@lists.openstack.org
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
