Some slight QA fixes in 2 commits

[zijianhuang]

Please check the commit comments.

[jsakamoto]

@zijianhuang Thank you for your contributions.

I'll accept your commit 5cf6dca in these days . It is nice improvement I think.

But your commit e3a7917 may has to consider well.
Because contributors of this library had discussion about pull request #14 . At that time, we decided to change catching exception type from limited type (FormatException) to all of types (any Exceptions) to make more defensive and safety code.

How do you think about this?

[jsakamoto]

@zijianhuang Could you please append unit test cases about your commit 5cf6dca ?

If you can't, then I'll take that task.

[jsakamoto]

@zijianhuang One more thing... 😄
Could you consider usinge nameof operator instead of string literal?

ex)
from: throw new ArgumentNullException("singleAddress"); to: throw new ArgumentNullException(nameof(singleAddress));

[zijianhuang]

Yes, all similar instances now are using nameof(...). Pushed to the fork.

BTW, CA will pick this up as a well if the name in exception does not match with the one in the function prototype.

Regarding to #14, catching ArgumentException also handle ArgumentNullException which is derived from ArgumentException. In short, conforming to the original requirement.

For the sake of defensive programming and safety, library code should not catch "all of types (any Exceptions)". Catching general exception should be the job of the application and CLR. Major platforms like WinForms, WPF, WCF and MVC etc. have built-in mechanism of catching uncaught exceptions. It is the responsibility of the application developers to utilize those mechanisms built-in.

Regarding to " unit test cases about your commit 5cf6dca", generally speaking, it is rightful to write test cases to cover new exceptions thrown, because this is part of the behaviors of the codes. However, generally I am a bit lazy in this particular case for these reasons:

1. The behavior is changed from throwing NullReferenceException accidentally to more informative and defensive ArgumentException. So the behavior is not significantly changed.
2. CA will pick up if the name is not matched
3. CA will pick up if someone later will remove respective defensive lines.

The major reason of writing test cases is to get assertion. In such case, I get assertion from CA. Nevertheless, having double assertions in both test case and CA is good.

[jsakamoto]

@zijianhuang I,m sorry that I don't know about what is meaning of term "CA"?
Would you tell me about it? (Is that a role name of human? or automatic system's name? etc...)

(The term "CA" what I well known is "Certificate Authority", but it doesn't match in this context. )

[jsakamoto]

@zijianhuang I can't merge your pull request directly, because your one pull request contains two factors.

• One of it is "add some parameter checks". This is acceptable for me.
• Another one contains "change catching exception type in TryParse static method".

If I (or the other contributors) reject this change (I don't decide it yet), I can't merge "add some parameter checks" commit because it built on "change catching exception type in TryParse static method" commit.

So I create new branch "improve/add-parameter-checks" and "cherry pick" your commit about "add some parameter checks".

See also: https://github.com/jsakamoto/ipaddressrange/commits/improve/add-parameter-checks

[jsakamoto]

@zijianhuang Oh, I understand about what is "CA"!
It's C# compiler's messages, isn't it?

So your commit e3a7917 contains resolving CA2240 C# compiler warning ( https://msdn.microsoft.com/en-us/library/ms182342.aspx ), that change GetObjectData method to virtual method.

I did not know about C# compiler detect these precise problems.
When I build this library on my Visual Studio 2015 Pro, it report nothing.

What is the difference about your and mine development environment and configurations?

[jsakamoto]

@zijianhuang

What is the difference about your and mine development environment and configurations?

I found it. (Project property > "Code Analysis" category > check on "Enable Code Analysis on Build")

At last, I understand what you want to say.

[jsakamoto]

@zijianhuang I create another branch and make commit about fix CA2240.

See also: https://github.com/jsakamoto/ipaddressrange/commits/fix/CA2240

[zijianhuang]

@jsakamoto, glad that you found out CA by yourself. It is Microsoft's implementation for Static Code Analysis, based on the obsolete and legendary FxCop. I believe that you like (Peer) Code Review as a part of QA, so Code Analysis is a robot for you to do automatic code review, it is better to let CA pickup something before you doing peer code review with your naked eyes. Noted that CA does not read C# codes, but IL codes.

Sorry that I may not offer more assistance in cherry picking of multiple commits in a pull request, since generally I just do trunk based development, and use feature branches only occasionally. Hopefully it is not hard for you to merge the changes through whatever means.

[jsakamoto]

@zijianhuang

1. CA will pick up if the name is not matched
2. CA will pick up if someone later will remove respective defensive lines.

Could you tell me about how to configure "Code Analysis" rule set to report those situations?

Default rule set "Microsoft Managed Recommended Rules" report nothing in those cases, and "Microsoft All" report too many reports taht include the report which I don't want receive.

[zijianhuang]

this is the ruleset I had customized over years:

https://github.com/zijianhuang/TraceHub/blob/master/Fonlow.ruleset

based on "Microsoft Managed Recommended Rules" but more restricted, according to Best Practices in MSDN., Noted that there's no one-size-fits-all, different types applications might have different concerns. Mine could be a good starting point for you.

[jsakamoto]

@zijianhuang Thank you for your disclosing the rule set codes.

At last, I figure out what rule do I need to detect the two cases ( "1. CA will pick up if the name is not matched" and "2. CA will pick up if someone later will remove respective defensive lines." ). It is CA2208 and CA1062. Isn't it?

But, I couldn't find those two rules in Fonlow.ruleset. How do you detect that validation of parameters before using was missed?

So I create my own custom rule set based on "Microsoft Managed Recommended Rules" ( commit 085d613 ), then I can see two warnings about the two cases.

Net step, I merged that commit into improve/add-parameter-checks branch ( commit 72c8f98 ).

After it, I see another CA1062 warnings ( "no validation of parameter before use it" ).

Why don't you fix it? What is the reason?

[zijianhuang]

You know that CA rules are largely based on best practices documented in MSDN. And best practices could be context specific. For different projects, I would adjust a little bit, according to the percentage of false alarms.

For example, regarding to CA1062, it could generate a lot fault alarms in MVC or Web API projects, because you don't really need to verify all parameters of those public functions in controllers, because all parameters in GET are already verified through URL queries.

Apparently you are new to CA, and I am still studying too even though I have been using CA/FxCop for over 10 years. And Microsoft had also been adjusting the default rules over years. As mentioned, there's no one-size-fits-all. So I am glad that you are making your own custom ruleset. Just just make CA work hard for you before peer code review.

[jsakamoto]

@zijianhuang Thank you for your reply. I think, I understand what you say. I became a fun of "Code Analysis" because it's very useful as you say.

But you didn't answer for my any questions.

Especially, why don't you add parameter validations at public methods of Bits class even CA report CA1062?

[zijianhuang]

I had answered your question implicitly. Just a bit more history to clarify. I copied and pasted the fonlow ruleset from one of my ASP.NET Web API project, and in such project, CA1062 generated a lot false alarms, so I had unchecked this rule. I had spotted some instances that missed the validation, of my immediate concerns. Surely this rule may be more appropriate for your project, but it is your choice or decision. I merely inspire you who know more contexts.

Generally, if a rule result in 50% - 75% false alarms in a specific project, I would exclude the rule.

And alternative solution is, you have multiple custom rulesets in your sln, so different csproj could use different ones.

[jsakamoto]

@zijianhuang There is your arbitrary decision? It sounds like a paradox what you said.

• As you said, and I agree, "CA will pick up if someone later will remove respective defensive lines".
• But, if we disabled CA1026, then CA can't pick up that violation if someone remove respective defensive lines.
• Or, if we keep CA1026, then 3 warnings are still appear, so it causes confusing in contributors, and it may cause missing detection of someone removing respective defensive lines, until we fixed those CA1026 x 3 warnings.

I can't accept this situation.

My decision is:

• Enable CA1026 check.
• Fix all CA1026 warnings. (Especially Bits class's some method. Those are public and reusable code, are not ASP.NET controller or like it, so fixing CA1026 is a good operation, I think.)
• Skip implementation of unit test cases for null argument checking, because CA will pick up it.

I'll release a new version of IPAddressRange library after those tasks finished.

[zijianhuang]

It is not really appropriate to make this issue / pull request become a comprehensive tutorial of how to use CA efficiently in production, and I expect you to do more researches through other means, say searching MSDN articles or CodeProject articles.

Anyway, I would still give you one hint to ease your apparent frustration in the new landscape, and help you to make decisions in other contexts in the future.

Right click on a false alarm of CA warning, and in the context menu, select "Suppress" ....

[jsakamoto]

@zijianhuang Thank you for your replay.

Right click on a false alarm of CA warning, and in the context menu, select "Suppress" ....

Thanks. I understand that I can control CA warnings by source code level with some attributes.

---

BTW, I would like comment about the another topic: "TryParse static method should catch all types of exception or not?"

I'm going to consider this topic well, so I'll create new issue about this.

I'll release new version on NuGet which contains changing about improving some method arguments checkings, ahead of resolving "TryParse static method should catch all types of exception or not?" discussion.

[jsakamoto]

I merged your changes except improvement of catching exception manually, and published NuGet packages of new version.

And, I created new issue #24 just now, so I close this pull request.

@zijianhuang, I appreciate your pull request and kindness. Please continue discussions at #24 . If you want to keep open this thread, please reply to me.