<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Mon, Dec 8, 2014 at 5:03 PM, Brant Knudson <span dir="ltr"><<a href="mailto:blk@acm.org" target="_blank">blk@acm.org</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div dir="ltr"><div><br>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.<br></div></div></blockquote><div><br></div><div>This behavior is expected. </div><div><br></div><div>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:</div><div><br></div><div><div> def outer():</div><div>    var = "outer"</div><div>    def inner():</div><div>        print var</div><div>    inner()</div></div><div><br></div><div><br></div><div>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).</div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div dir="ltr"><div><br>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. </div></div></blockquote><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div dir="ltr"><div><br>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.<br></div></div></blockquote><div><br></div><div>Why do you think the max complexity check is harmful? because it prevents large amounts of nested functions?</div><div><br></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div dir="ltr"><div><br>[1] <a href="https://review.openstack.org/#/c/139835/" target="_blank">https://review.openstack.org/#/c/139835/</a><br>[2] <a href="https://review.openstack.org/#/c/139836/" target="_blank">https://review.openstack.org/#/c/139836/</a><br>[3] <a href="https://review.openstack.org/#/c/140188/" target="_blank">https://review.openstack.org/#/c/140188/</a><span><font color="#888888"><br><br></font></span></div><span><font color="#888888">- Brant<br><br></font></span></div>
<br>_______________________________________________<br>
OpenStack-dev mailing list<br>
<a href="mailto:OpenStack-dev@lists.openstack.org" target="_blank">OpenStack-dev@lists.openstack.org</a><br>
<a href="http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev" target="_blank">http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev</a><br>
<br></blockquote></div><br></div></div>