Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

A flag to treat inner functions as separate from outer function #60

Closed
JanChec opened this issue May 17, 2018 · 3 comments
Closed

A flag to treat inner functions as separate from outer function #60

JanChec opened this issue May 17, 2018 · 3 comments

Comments

@JanChec
Copy link

JanChec commented May 17, 2018

Hey!

When I have a function with several helper functions, I want to group them in a common context for readability. If only the main function is called from outside and all those helper functions are just readability helpers (so this main function isn't huge), this gives me:

  • too many tightly related functions (in total) to not have a common context
  • too little code to create a module (especially when there are many modules already)
  • no side effects (state)

Then I create a function with inner functions:

  • first inner function: _run() that is the highest abstraction level inner function
  • then 2-4 simple inner functions
  • lastly in the body of the outer function: return _run()

Details why this is probably the best solution here: https://stackoverflow.com/questions/50253517/how-to-group-functions-without-side-effects

The problem is that currently mccabe sums up every inner function complexity with outer function complexity and outputs this as only one, huge result. Please consider:

def inner_uncalled_functions():
    def _run():
        return 0

    def _f1():
        return 0

    def _f2():
        return 0

    return 0


def inner_uncalled_branched_functions():
    def _run():
        return 0

    def _f1():
        if True:
            return 0
        else:
            return 1

    def _f2():
        if True:
            return 0
        else:
            return 1

    return 0


def inner_connected_functions():
    def _run():
        return _f1()

    def _f1():
        return _f2()

    def _f2():
        return 0

    return _run()
$ python -m mccabe mccabe_nested_test.py 
1:0: 'inner_uncalled_functions' 4
14:0: 'inner_uncalled_branched_functions' 6
33:0: 'inner_connected_functions' 4

(sorted for readability)

I'd love if there was a flag to treat inner functions as separate from outer function.

@sigmavirus24
Copy link
Member

Hi there @kern3l

Thanks for the suggestion! This is valuable input and the request here seems reasonable. I took a look over https://en.wikipedia.org/wiki/Cyclomatic_complexity since I haven't looked at it in a few years. One thing that stuck out to me was

One of McCabe's original applications was to limit the complexity of routines during program development; he recommended that programmers should count the complexity of the modules they are developing, and split them into smaller modules whenever the cyclomatic complexity of the module exceeded 10.

Based on the spirit of the calculation (i.e., to reduce the complexity of routines - a.k.a, functions) I'm starting to wonder if adding the flag would counteract the benefits of running mccabe. Specifically, nested functions might not contribute to logical branches in a function, however, they do contribute to the complexity as far as other people are considered when reading the same code. Further, if you look at mccabe as a calculation of the number of different tests you might need to write to exercise all the code inside a routine/function, then you must consider nested functions as well.

In short, I'm leaning towards not adding a flag for this use-case. It seems to run counter to what the complexity calculation is trying to measure and the usefulness of the calculation. Does that make sense to you?

@JanChec
Copy link
Author

JanChec commented May 18, 2018

Thanks for the answer! I completely agree that in the default mode the inner function should be counted as a part of the main function. Reasons:

  • the chosen granularity here is functions
  • inner functions are part of the outer functions
  • there's no way to use (or test) inner functions directly

I'm sure you know how it is when you're putting a product out there and end users have different needs and ideas how the product should behave, often missing its scope by some margin. If feedback seems reasonable, then, as a programmer, there's always this set of questions:

  • are my assumptions correct how core functionality should work?
  • is it reasonable to allow modification of core behaviour to satisfy the need?
  • ^ = how big is the group that wants the change vs how hard it would be to develop and maintain?
  • are there any suitable workarounds?

Myself, I use your program to automatically block unreadable crap from even reaching code review. Readability is my main concern and testability is highly correlated with that. Heh, at this point I convinced myself that adding noqa in such cases is better, because it shows that something is wrong here, which is: the code is probably not tested enough automatically. At some times it's fine, but not always.

I hope that someone will find this input valuable. Cheers!

@JanChec JanChec closed this as completed May 18, 2018
@sigmavirus24
Copy link
Member

Thank you for your understanding and your incredibly valuable feedback!

I absolutely agree that if more people comment here in support of this (because while I appreciate reactions, they provide 0 visibility into the popularity of an issue), we can definitely reconsider this.

Myself, I use your program to automatically block unreadable crap from even reaching code review. Readability is my main concern and testability is highly correlated with that.

I'm glad to hear mccabe is helping with that. It's always good to hear that we're helping you improve the quality of the code you're maintaing just like it helps us.

Thank you again!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants