Inaccurate Warning (I Think)

[Mike-E-angelo]

First off, cool project here. I just installed and it turns out most of my code is compliant. 😄

However, I do seem to have an issue here:

It would seem that the condition count is 3 here, when it's actually 2. Please let me know if and why I have this wrong. Thank you for any clarification!

[MO2k4]

Hi,

I think your assumption is wrong, but let me clarify this.

The condition count should be 3, because there are 3 conditions:

• parameters.Length == 0
• x.IsOptional
• x.Has<ParamArrayAttribute>()

Each condition should be counted, because the lambda is part of your condition. If there is lambs statement each condition is counted to the total amount.

[Mike-E-angelo]

I see what you are saying. However, I would place the lambdas in a new context altogether, as that is what a lambda is, after all. :)

On its face, there does appear to be 3 conditions, but perhaps consider that there are really two:

• parameters.Length == 0
• A call to an extension method with a single lambda argument, which, in turn, once resolved can also have an arbitrary set of its own conditions (in its own context).

To me, it would seem that the count would reset with each new/defined context.

[MO2k4]

Sorry i missed your feedback.

I think it should be considered as one context, because only the whole statement (bool-Expression and lamba-Expression) makes it complete.

So what you say is that you would expect each lambda should be counted as their own context, so it should be counted as 1?

So if you think of something like this:

var result = parameters.All(x => x.IsOptional && x.A) || parameters.All(x => x.IsOptional && x.IsB) || parameters.All(x => x.IsOptional && x.IsC)

Whats the counter here? In my opinion it should be 6, because the complexity is highly increased. Each Lamba-Expression should be checked for itself. If you think of the testable context for this statement, you need a minimum of 6 Testcases to get it all tested.

This suggestion is for the whole statement, not for parts of it.

[Mike-E-angelo]

I do see your side of this, however, consider the natural conclusion:

You could replace either the lambda (x => x.IsOptional && x.A) or the entire outer expression (e.g. parameters.All(x => x.IsOptional && x.A)) with a method call to alleviate the warnings. This would reduce to three expressions total (e.g. var result = IsA() || IsB() || IsC()) but now you have polluted your class definition with three extra methods, which I do not feel is "cleaner" code.

Just my opinion, however. 😁

[MO2k4]

Yeah that is correct, but if you think of a more complex lamba statement, then i think its better to isolate (make a seperat method) for better testing. Another point could be that if you put your lamba expression in a seperat function there could a better way in naming this function. In my opinion this results in more readable code. It's correct that you've got more methods, but i think that small methods are not the problem, if they are properly named.

[Mike-E-angelo]

We're not talking about a more complex lambda statement but what you have offered as an example, hence the apprehension on my part. As you see it, you are advocating to pollute classes with extra (unnecessary) methods due to six statements, when there are (again, my opinion) three with two nested statements.

A better and simpler example to illustrate the problem would be:

var result = parameters.All(x => x.IsOptional && x.A) || parameters.All(x => x.IsOptional && x.IsB)

This is currently counted as four expressions and would result in at least one method placed at the class and/or method (forgot about this new feature :)) to resolve the warning under discussion here.

I am with you in regards to having the "sense" to trigger encapsulation, but the point here is that (from my view) we have not reached this level yet with this example, but the application logic is saying so.

[Mike-E-angelo]

Doing some scouting on all the extensions I use and see that this one is already up to 2019.1. Thank you!

In regards to this issue. Since there are two differing views in regards to this issue, is there any way that we could make this a setting in the options menu? That way we each could win with a click of a checkbox. 😁

[MO2k4]

Hey Mike :-)

thats a good idea, i will take a closer look into that, but that will take a while.

What would be a meaningful description for that point?

[Mike-E-angelo]

Yeah, no worries. To be honest, I am more worried about being responsible for having created the only remaining issue that is sticking out like a sore thumb on this repo. That would bug me as a repo owner. 😆

For the description, my take would be:

Reset evaluation context for lambdas.

This would be unchecked as default behavior as it is now.

You could even put an information identifier/icon next to it that either presents a tooltip for more information and/or launches a new browser to this issue when clicked.