StefanMaron / BusinessCentral.LinterCop

Community driven code linter for AL (MS Dynamics 365 Business Central)
https://stefanmaron.com
MIT License
77 stars 31 forks source link

How about cyclomatic complexity? #48

Closed lvanvugt closed 3 years ago

lvanvugt commented 3 years ago

@StefanMaron, as per the short discussion on you blog I am posting this question here: how about cyclomatic complexity? Or in other words: what became of this feature that was part of the AL Lint VS Code extension? Or is it in there and I do not understand how to get it triggered (btw: not expecting it as LinterCop is a cop)?

BTW: see #47, where I note thta the read.me is a bit sparse on telling what LinterCop is actually doing.

lvanvugt commented 3 years ago

BTW2: the same applies to Maintainability Index.

StefanMaron commented 3 years ago

Short answer: Yes, I am planning to integrate both again.

As you already stated, its not possible to do it the same way as it was in the AL Linter. Since I can only display messages when given conditions are met. So I see two options here:

  1. Always show the values of maintainability Index and cyclomatic complexity on every procedure and trigger as an Info message
  2. Make some kind of config file similar to the AppSource.json and set a threshold for both values and show a warning if those are reached
lvanvugt commented 3 years ago

Can 1 and 2 be combined? I.e., when the threshold is reached color the info or the function name (the latter likewise AL Test Runner does in relation to the outcome of a test run).

StefanMaron commented 3 years ago

Yes, I think I just can do both and hide the info if the warning shows up. I would also split it in two rules to each can be turned off separately

StefanMaron commented 3 years ago

I searched a bit for a reasonable way of calculating those values and this one seems to be okay: https://radon.readthedocs.io/en/latest/intro.html

For cyclomatic complexity it seems to be just a counting of decisions in a given code block. Mapping the list in the document to al statements which would each add +1 to the complexity:

For Halstead volume I am still not sure what would be the operators used to calculate it.

lvanvugt commented 3 years ago

I searched a bit for a reasonable way of calculating those values and this one seems to be okay: https://radon.readthedocs.io/en/latest/intro.html

Great

For Halstead volume I am still not sure what would be the operators used to calculate it.

I have not gone into this myself.

StefanMaron commented 3 years ago

I added the info message in v0.15. But I decided to make the info message disabled by default as it can generate a huge amount of messages. Just set it to info in a custom rule set to enable it.

Warning message will fowllow

lvanvugt commented 3 years ago

When will it be available?

StefanMaron commented 3 years ago

Info message is available now. And I will try to also implement the warning this week. I will keep you updated here

lvanvugt commented 3 years ago

Not realy. image

StefanMaron commented 3 years ago

The vs code extension just works as an autoupdater of the dll if you have the extension installed it automatically checks if a new version of the dll is available and downloads it from this github. The check is run on startup of vs code or manually with a command

lvanvugt commented 3 years ago

image

lvanvugt commented 3 years ago

How to enable the info message?

StefanMaron commented 3 years ago

First of all you need to have the code cop in you settings:

"al.codeAnalyzers": [
    "${CodeCop}",
    "${UICop}",
    "${analyzerfolder}BusinessCentral.LinterCop.dll" <== this one
],

then you should already see some warnings in your code. To enable the info message you need to activate the rule via custom rule set: https://docs.microsoft.com/en-us/dynamics365/business-central/dev-itpro/developer/devenv-rule-set-syntax-for-code-analysis-tools this is my setting: "al.ruleSetPath": "./my.ruleset.json"

and within I have my rules adjusted:

{
    "name": "Name",
    "description": "Description",
    "rules": [
        {
            "id": "LC0001",
            "action": "Hidden",
            "justification": "Justification"
        },
        {
            "id": "LC0009",
            "action": "Info",
            "justification": "Justification"
        }
    ]
}

Could be that you need to reload VS Code after setting this up

lvanvugt commented 3 years ago

All set. Now where should I be able to see the info message regarding cyclometric complexity? Don't seem to find anything yet.

StefanMaron commented 3 years ago

They should show up between other warnings or if you have error lens, right behind the function in code image image

lvanvugt commented 3 years ago

Doesn't show at all. Any way to check things in the background?

StefanMaron commented 3 years ago

Do you have "al.enableCodeAnalysis": true in settings?

lvanvugt commented 3 years ago

Holy smoke, that's what I missed. Thanx. Working nicely right now.

StefanMaron commented 3 years ago

Glad to hear it works :) I will update the readme later to make the install a bit clearer

lvanvugt commented 3 years ago

But the cyclomatic complexity is not done right IMHO.

Example 1

image Shouldn't this be 1? No decisions, so basic 1.

Example 2

image Shouldn't this be 4? Basic 1 plus 3 decisions.

StefanMaron commented 3 years ago

I did not add the plus 1. 😅 but that will be an easy one

lvanvugt commented 3 years ago

v0,15 + 1 = v1.15? Or v0.16? 😅

StefanMaron commented 3 years ago

What would be default thresholds to raise a warning instead of an info message?

lvanvugt commented 3 years ago

I reckon you do make it configurable, but you need a value to start with, right?

StefanMaron commented 3 years ago

Yes exactly I need some default values in case no config is in place. So the default value for warnings to start appear is when either cyclomatic complexity reaches 8 or greater or when maintainability index reaches 20 or lower

jwikman commented 2 years ago

For Halstead volume I am still not sure what would be the operators used to calculate it.

Hi @StefanMaron, what did you decide to use for Halstead Volume? It could be really helpful to get some more insights into how both Cyclomatic complexity and Maintainability index are calculated by this rule, when you are trying to improve your score.

Maybe it could deserve it's own .md file with documentation? (That might useful for every rule, actually 😀)

StefanMaron commented 2 years ago

I will put it on the list! Could you please rephrase it into a new issue saying to document all the rules? Then it does not get lost ;)