kisstkondoros / codemetrics

VSCode extension which shows the complexity information for TypeScript class members
Other
402 stars 20 forks source link

what does "complexity" mean? #28

Closed ericblade closed 7 years ago

ericblade commented 7 years ago

... is this some standard measurement? The extension looked interesting, so I installed it, but despite having a bunch of really complex code, I pretty much only see one warning from this plugin, on a really simple section:

    function getIntentName() {
        switch (type) {
            case 'IntentRequest':
                return req.data.request.intent && req.data.request.intent.name;
            case 'LaunchRequest':
                return 'LaunchRequest';
            case 'SessionEndedRequest':
                return 'SessionEndedRequest';
            default:
                return '';
        }
    }
bduff9 commented 7 years ago

I too would like some clarification. In my case, I have some relatively short functions that are telling me are ridiculously complex. Some of it I get (having arrow functions inline vs. breaking them out) but it also tells me that an empty object ({}) is a complexity and I'm not sure how to refactor that. More information about what this does, what the benefit is, and how to fix the messages it gives would be much appreciated. Thanks!

ericblade commented 7 years ago

Absolutely would be nice. The idea is interesting, but functionally it seems very strange. I opened up another app and every function inside it had a 39 or higher score. At that point, I disabled it.

kisstkondoros commented 7 years ago

@ericblade the code snippet You posted contains 4 return statements, a boolean expression and it is essentially a function declaration, I think that combined with the switch statement and with each case statement is about ~11 in complexity. If this seems to be incorrect for You, then You can change the weight for e.g. switch / case / default statements in the configuration, I'm personally against having switch cases and return statements all around the codebase.

@bduff9 the extension does not distinguishes between an empty object and an object declaration (which has actual members defined). I think it is pretty much self explaining what the complexity means (if You click on the code lens, You'll get a list which explains what that number is actually about). I have some bad news for you, if You want to lower complexity you have to refactor the code (or change the rules in the settings 😄 ). It would be nice to trigger automatic refactoring when selecting an item from the quickpick menu, but there is no such a thing in vscode (I have had implemented a similar thing for Intellij/Java which does this and it is amazing).

@ericblade most likely it was some heavy IIFE stuff (likely jquery or prebuilt code), which is hard to detect and handle differently. I know it is not very convenient but You can just simply ignore the numbers in that case. If it was not IIFE, please post a code snippet.

bduff9 commented 7 years ago

@kisstkondoros That's not really an answer to our questions. I get the point of it is to show where refactoring is suggested/necessary (that's why I mentioned it in my question). However, why is an empty object considered a complexity? Are you suggesting I create a separate function for every single object I need? That seems like a terrible idea and will do the opposite of simplifying code.

Also, to your point, the list does not explain how to resolve. It simply says the object is a complexity, or the if statement is a complexity, etc. It doesn't explain why or what I can do (short of creating a hundred smaller functions) to resolve. Maybe the complexity score is set too low for normal users, but then why are these values the default? Regardless of which direction you wanted this plugin to take, better documentation and explanations will go a long way to helping as I, like the previous posted, uninstalled it when it provided me no additional help and in fact made my code harder to read with numerous markup lines.

I like the idea and see how it could be useful, I just wish it was either easier to use or had better docs somewhere so your users aren't forced to submit a github issue just to get some clarification.

choclaudio commented 7 years ago

Is this not simply cyclomatic complexity?

ericblade commented 7 years ago

i mean, i guess that could be refactored into:

if (type === 'IntentRequest') return name;
if (type === 'SessionEndedRequest' || type === 'LaunchRequest') return type;
return '';

... but.. that's literally what switch is for. I don't want to turn into debating code style, because that's not the point here. I think that this plugin identifies really very simple things as being of high complexity, for reasons that are not at all documented. Which leaves this to me as an interesting idea that I'd like to see become a lot smarter than it currently seems to be. Or I'm not understanding it at all, which is why I asked to begin with.

kisstkondoros commented 7 years ago

As @choclaudio pointed out the plugin provides cyclomatic complexity (kind of), and also provides a way to configure weights to single aspects.

If You don't like that e.g. switch case statements increase the complexity that much You can change that by setting

    "codemetrics.nodeconfiguration.CaseClause": 0,
    "codemetrics.nodeconfiguration.SwitchStatement": 0,
    "codemetrics.nodeconfiguration.BreakStatement": 0,
    "codemetrics.nodeconfiguration.DefaultClause": 0

or if You don't like object literals increase complexity by one always then You can turn it off like this

    "codemetrics.nodeconfiguration.ObjectLiteralExpression": 0

I have no clue how useful the current default configuration is for the community, and if these defaults are wrong then maybe I have to change them.

I can think of some kind of preset selection feature to make the initial configuration easier, but I'll close this ticket, without any adjustments to the code base.

PR's (as always) are welcome.