On Wed, Dec 10, 2014 at 8:43 AM, Joe Gordon <[email protected]> wrote:
> > > On Mon, Dec 8, 2014 at 5:03 PM, Brant Knudson <[email protected]> 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 >> [email protected] >> http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev >> >> > > _______________________________________________ > OpenStack-dev mailing list > [email protected] > http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev > >
_______________________________________________ OpenStack-dev mailing list [email protected] http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
