Switching off worthless Code Analysis rules

This post is related to Microsoft’s Code Analysis tool for .Net projects aka FxCop.

Code Analysis is a great tool that helps to keep code cleaner and avoid some common mistakes. However some rules produce CA errors much more often than others. Developers get used to these violations and just blindly suppress them in a hurry to build the project.

I believe having unnecessary Code Analysis rules and their numerous suppressions is bad, because:

  1. When Code Analyzer detects some violation, developer should locate it, analyze the issue, fix it or suppress rule violation and rebuild the project. It takes the time. It’s OK if the rule actually detects some issue with code that should be fixed. However as we will see below, many default rules are pretty useless for major projects, and all above actions are just waste of time.
  2. Long lists of suppressions pollute the code and decrease it readability. Such code doesn’t look good to me:

  3. When developers suppress more and more Code Analysis errors, they start to treat the whole step of Code Analysis as un-useful, do not investigate issues properly and just suppress the errors as quickly as possible. Thus some real problems worth fixing could be suppressed. In my opinion it’s better to have reduced set of really useful rules. There will be much less Code Analysis errors that should be processed properly.
  4. Disabling unnecessary rules will decrease time spent for Code Analysis step, because each rule is processed independently. Thus we’ll decrease time of every single build for every Project developer.

Taking into account above points, I have analyzed CA suppressions for the Project I’m currently working on. Then I’ve considered carefully which of these rules provide the value for the Project and which are just worthless and should be disabled thus.

With a simple Perl script I’ve gathered statistics on what rules are suppressed more often in the Project. I’ve excluded suppressions for auto-generated code and counted suppressions for Production and Unit Test code separately. Here is the statistics I got:

Rule IDCA TitleCA CategorySuppr. CountSuppr. %% in Prod. code% in Test codeSuggestion
CA1704Identifiers should be spelled correctlyMicrosoft.Naming73914.90%76.60%23.40%Disable
CA1707Identifiers should not contain underscoresMicrosoft.Naming4789.60%31.20%68.80%Disable for UT
CA1709Identifiers should be cased correctlyMicrosoft.Naming4348.70%91.90%8.10%Disable
CA1506Avoid excessive class couplingMicrosoft.Maintainability3296.60%29.80%70.20%Disable for UT
CA2000Dispose objects before losing scopeMicrosoft.Reliability2414.90%52.70%47.30%Disable for UT
CA1819Properties should not return arraysMicrosoft.Performance2274.60%100.00%0.00%Keep
CA1020Avoid namespaces with few typesMicrosoft.Design2254.50%78.20%21.80%Disable
CA1031Do not catch general exception typesMicrosoft.Design1803.60%81.70%18.30%Keep
CA1006Do not nest generic types in member signaturesMicrosoft.Design1803.60%92.80%7.20%Disable
CA1303Do not pass literals as localized parametersMicrosoft.Globalization1543.10%60.40%39.60%Disable
CA1051Do not declare visible instance fieldsMicrosoft.Design1352.70%97.80%2.20%Keep
CA2227Collection properties should be read onlyMicrosoft.Usage1242.50%100.00%0.00%Keep
CA1056URI properties should not be stringsMicrosoft.Design1122.30%100.00%0.00%Disable
CA1726Use preferred termsMicrosoft.Naming1092.20%88.10%11.90%Disable
CA1002Do not expose generic listsMicrosoft.Design1092.20%98.20%1.80%Keep
CA1801Review unused parametersMicrosoft.Usage1082.20%96.30%3.70%Keep
CA1024Use properties where appropriateMicrosoft.Design992.00%52.50%47.50%Disable
CA2204Literals should be spelled correctlyMicrosoft.Usage961.90%89.60%10.40%Disable
CA1702Compound words should be cased correctlyMicrosoft.Naming881.80%65.90%34.10%Disable
N/AOtherN/A79816.00%N/AN/AKeep

As you see top 20 rules share almost 85% of all suppressions in the Project. Here are my considerations which of these top rules should be kept and which of them should be disabled and why.

1. CA1704: Identifiers should be spelled correctly
Suggestion: Disable

Our Project (as well as any other) uses a lot of words specific to application domain. Code Analyzer certainly doesn’t know about these words, that triggers rule violation. I believe this rule, as well as all other rules from Naming category, should be just removed from checked rule set. Most cases when this rule hits, are not a typo and rule is just suppressed. Of course there are cases, when developer actually misspells some word and CA1704 rule could detect it. However it happens rarely and doesn’t affect the code quality too much. And if misspelled identifier is used widely in the Project, there is a great chance that someone will notice the typo and will correct it.

There is a possibility to avoid suppression of CA1704 by adding dictionary of recognized words in XML file. But again, I believe this is a waste of time and the better option is just to disable this rule.

2. CA1707: Identifiers should not contain underscores
Suggestion: Keep for Production code, Disable for UT code

Almost 70% of suppressions for this rule are in Unit Test code. This is because many developers follow recommended naming approach for Unit Test methods:

[MethodUnderTest]_[ScenarioUnderTest]_[ExpectedBehavior].

Such naming for UT was proposed by Roy Osherove in his excellent book ‘The Art of Unit Testing’. This naming approach also gathered majority of votes in prominent StackOverflow question on Unit test naming best practices. Developer that follows this naming approach should suppress rule CA1707 for each test method he writes. It doesn’t make much sense to me.

My suggestion is to disable CA1707 for rule set used in UT projects.

3. CA1709: Identifiers should be cased correctly
Suggestion: Disable

This rule is suppressed mostly for acronyms. The rule requires that two-letter acronyms use all uppercase letters, and acronyms of three or more characters use Pascal casing.

This casing rule is even often violated by developers of .Net Framework library. The proper name for DbContext should be DBContext if this rule is followed.

4. CA1506: Avoid excessive class coupling
Suggestion: Keep for Production code, Disable for UT code

70% of suppressions for this rule are in Test code. It hits for Unit and Integration tests that should setup and configure numerous dependencies.

In Production code the rule often hits for Composition Root where Dependency Injection container is configured. Composition Root is coupled with big number of dependencies by its nature, so this suppression is OK.

Sometimes rule CA1506 really detects some problem with the code, when class or method is overcomplicated and is coupled with large number of dependencies. In this case the code should be refactored with Extract Class or Extract Method. Unfortunately, these are not trivial refactorings (especially Extract Class), and developers usually just suppress the rule.

In my own projects I use different rule sets for Production and Test code, having CA1506 disabled for Test rule set and enabled for Production. If only one rule set could be used, I’d prefer to have CA1506 enabled despite a large number of required suppressions for UT and Composition Roots.

5. CA2000: Dispose objects before losing scope
Suggestion: Keep for Production code, Disable for UT code

CA2000 is one of the most useful rules, despite the fact that it produces large number of false positives. The main sources for suppressions are methods that create instance of disposable object that is returned to the caller or assigned to instance field. The caller should take care for calling Dispose() on the object. However Code Analysis just treats such situation as missed Dispose() call.

Nevertheless this rule often helps to detect missed Dispose() calls and should be definitely used in rule set for Production code.

However I prefer to disable this rule for Test code. We should strive to make UT code as simple as possible. Disposing objects in test code doesn’t make much sense and should be omitted thus.

6. CA1819: Properties should not return arrays, CA2227: Collection properties should be read only
Suggestion: Keep

CA1819 and CA2227 are described together because they are often related to each other.

Say we have following Data Contract:

It triggers violation of CA1819 because the property returns array object. We could try simplest possible fix:

In this case we’ll get violation of CA2227 because the Collection property has a setter. Trying to fix further we get:

And this works just fine with any serializer/deserializer.

So my suggestion here is to keep CA1819 and CA2227 enabled, and avoid suppression by replacing arrays with collections like in the code above.

7. CA1020: Avoid namespaces with few types
Suggestion: Disable

This rule is violated if you group your source files by directories and use default namespace naming that imitate directory tree. CA1020 is just another worthless rule with unclear background. It’s often happen that such directory (i.e. namespace) contains few classes, but is extended later during development. Merging it with some parent namespace at first and extracting to separate namespace later (when there are enough classes), is just a pain and unnecessary waste of time.

8. CA1031: Do not catch general exception types
Suggestion: Keep

This rule is usually suppressed for top level handler that catches all exceptions, logs them and returns some default value in case of error.

I prefer to leave this rule enabled so that developer could think twice before catching and suppressing all possible exceptions.

9. CA1006: Do not nest generic types in member signatures
Suggestion: Disable

This rule hits more often for asynchronous methods returning some kind of collection. Suppose you have interface method with following signature:

You’ll get CA1006 violation because List<int> is nested inside a Task<>.

Since asynchronous methods are widely used in the Project, this rule brings too many false positives and thus should be disabled until Code Analyzer correctly processes such cases.

10. CA1303: Do not pass literals as localized parameters
Suggestion: Disable

This rule hits 40% of time for Test code where string literals are used widely.

In Production code it’s often suppressed for log messages and other cases when string is not exposed to the UI. Another source of suppressions are internal Project tools that aren’t localized.

I believe it’s better to disable this rule. Of course it could happen that some novice developer makes a mistake and uses string literal that should be actually kept in resources. However this is not very likely and will be detected during localization check by QA.

11. CA1051: Do not declare visible instance fields
Suggestion: Keep

This rule is suppressed unexpectedly often in our Project. In most cases it’s found in DTO, however public fields do not provide any benefit over properties.

This rule should definitely be kept enabled. Also all suppressions of CA1051 should be revised and fixed.

12. CA1056: URI properties should not be strings
Suggestion: Disable

CA1056 is quite annoying rule that is also often suppressed. Sometimes it’s just more convenient to handle URLs as raw strings without carrying on restrictions that System.Uri requires.

13. CA1726: Use preferred terms
Suggestion: Disable

One more rule from Naming category that is useless in my opinion. In our codebase almost all suppressions of this rule are for ‘Login’ term. CA1726 claims it should be ‘Logon’. It seems questionable for me as well as for other developers who just suppress this strange rule.

14. CA1002: Do not expose generic lists
Suggestion: Keep

This rule is not very useful in our project, since we do not develop public libraries and any interface or contract could be refactored at any time. However this rule develops a good habit of using proper collections for externally visible members. That’s why I prefer to keep it enabled in our rule set.

15. CA1024: Use properties where appropriate
Suggestion: Disable

This is that weird rule that requires that method

is replaced with a property. Any developer can make a correct decision when the property is a better option than a method. This rule just doesn’t bring any value but provides too much false positives for methods that are not suitable for being the properties.

16. CA2204: Literals should be spelled correctly
Suggestion: Disable

In my opinion this rule should also be disabled with the same reasoning as for rules from Naming category: it generates too much false positives and does not provide too much value.

17. CA1702: Compound words should be cased correctly
Suggestion: Disable

What would you choose for naming in your code:

  • Logoff or LogOff
  • TimeStamp or Timestamp
  • plainText or plaintext
  • SubAccount or Subaccount

That’s examples from our Project and CA1702 insists on second versions. That doesn’t make much sense to me. I’d prefer just to disable this rule.

Based on above information, I proposed following improvements in Code Analysis for our Project:

  1. Separate rule set for Test projects should be cloned from Production rule set.
  2. Following CA rules should be disabled for Production code:
    • CA1704: Identifiers should be spelled correctly
    • CA1709: Identifiers should be cased correctly
    • CA1020: Avoid namespaces with few types
    • CA1006: Do not nest generic types in member signatures
    • CA1303: Do not pass literals as localized parameters
    • CA1056: URI properties should not be strings
    • CA1726: Use preferred terms
    • CA1024: Use properties where appropriate
    • CA2204: Literals should be spelled correctly
    • CA1702: Compound words should be cased correctly
  3. Following CA rules should be disabled for UT code:
    • CA1707: Identifiers should not contain underscores
    • CA1709: Identifiers should be cased correctly
    • CA1506: Avoid excessive class coupling
    • CA2000: Dispose objects before losing scope
    • CA1020: Avoid namespaces with few types
    • CA1006: Do not nest generic types in member signatures
    • CA1303: Do not pass literals as localized parameters
    • CA1056: URI properties should not be strings
    • CA1726: Use preferred terms
    • CA1024: Use properties where appropriate
    • CA2204: Literals should be spelled correctly
    • CA1702: Compound words should be cased correctly
  4. Existing suppressions for disabled rules should be removed from the code. This step could be done in automatic way by Perl or Python script.

These proposals were discussed in community of Technical Leaders and were accepted with light adjustments.

This entry was posted in Programming and tagged , , , . Bookmark the permalink.

Leave a Reply