Which are the "must follow" FxCop rules for any C# developer?

Question

I'm planning to start using FxCop in one of our ongoing project. But, when i tried it with selecting all available rules, it looks like I have to make lots of changes in my code. Being a "team member" I can't right away start making these changes, like naming convention change etc. anyway i would like to start using FxCop with a minimal rule set and would gradually increase the rule set as we goes on. Can you suggest me some must have FxCop rules which i should start following. Or do you suggest any better approach?

Note: Most of my code is in C#.

Answer 1

The design and security rules are a good place to start.

Answer 2

In my opinion, do the following:

For any new project, follow all FxCop rules. You may want to disable some of them, since not everything will make sense for your project. For an existing project, follow the rules from these categories as a minimum set:

• Globalization
• Interoperability
• Security
• Performance
• Portability

Since these are typically only few rule violations in an existing project, compared to the other categories, but may improve the quality of your application. When these rules are clear, try to fix the following categories:

• Design
• Usage

Since these will make it easier for you to spot bugs that have to do with the violations, but you will have a large amount of violations in existing code.

Always sort the violations by level/fix category and start with the critical ones. Skip the warnings for now.

In case you didn't know, there's also StyleCop available from Microsoft, checking your code on the source level. Be sure to enable MSBuild integration during installation.

Answer 3

On our most important code:

• Treat warnings as errors (level 4)
• FxCop must pass 100% (no ignores generally allowed)
• Gendarme used as a guideline (sometimes it conflicts with FxCop)

Believe it or not, FxCop teaches you a hell of a lot on how to write better code... great tool! So for us, all rules are equally important.

Answer 4

We're a web shop so we drop the following rules:

• Anything with Interop (we don't support COM integration unless a client pays for it!)
• Key signing (web apps shouldn't need high security prilages)

Occationally we'll drop the rule about using higher frameworks in dependancies as some of our CMS's are still .NET 2.0, but that doesn't mean the DAL/ Business Layers can't be .NET 3.5 as long as you're not trying to return an IQueryable (or anything .NET 3, 3.5).

Answer 5

I fully agree with Sklivvz. But for existing projects, you may clean up FxCop violations category by category.

From time to time, Gendarme accepts new rules that are quite useful. So you may use Gendarme besides.

Answer 6

In our process, we enabled all the rules and then we have to justify any suppressions as part of our review process. Often, it's just not possible to fix the error in time-efficient manner with regards to deadlines or its an error raised in error (this sometimes occurs - especially if your architecture handles plug-ins via reflection).

We also wrote a custom rule for globalization to replace an existing one because we didn't want to globalize the strings passed to exceptions.

In general, I'd say it's best to try and adhere to all rules. In my current home project, I have four build configurations - one set that specify the CODE_ANALYSIS define and one set that don't. That way, I can see all the messages I have suppressed just by building a non-CODE_ANALYSIS configuration. This means that suppressed messages can be periodically reviewed and potentially addressed or removed as required.

What I'd like to do in the long-run is have a build step that analyzes the SuppressMessage attributes against the actual errors and highlights those suppressions that are no longer required, but that's not currently possible with my setup.

Answer 7

An alternative to FxCop would be to use NDepend and its Code Query language (CQL). CQL is dedicated to write code quality rules that can be verified live in Visual Studio, or that can be verified during build process and reported in a HTML report. Here are a few samples of CQL rules (designed to be highly customizable):

Code refactored recently should be 100% covered by test:

WARN IF Count > 0 IN SELECT METHODS WHERE CodeWasChanged AND PercentageCoverage < 100

Complex methods should be commented:

WARN IF Count > 0 IN SELECT METHODS WHERE CyclomaticComplexity > 15 AND PercentageComment < 10

I don’t want that my User Interface layer to depend directly on the DataBase layer:

WARN IF Count > 0 IN SELECT NAMESPACES WHERE IsDirectlyUsing "DataLayer" AND NameIs "UILayer"

Static fields should not be named m_XXX (Custom naming conventions):

WARN IF Count > 0 IN SELECT FIELDS WHERE NameLike "^m_" AND IsStatic

Methods out of MyAssembly and MyAssembly2 should not have more than 30 lines of code:

WARN IF Count > 0 IN SELECT METHODS OUT OF ASSEMBLIES "MyAssembly1", "MyAssembly2" WHERE NbLinesOfCode > 30

Public methods should not be removed to avoid API breaking changes:

WARN IF Count > 0 IN SELECT METHODS WHERE IsPublic AND IsInOlderBuild AND WasRemoved

Types tagged with the attribute MyNamespace.FullCoveredAttribute must be thoroughly covered by tests:

WARN IF Count > 0 IN SELECT TYPES WHERE HasAttribute "MyNamespace.FullCoveredAttribute" AND PercentageCoverage < 100

Answer 8

Turn on one rule at a time. Fix or exclude any warnings it reports, then start on the next one.

Answer 9

Some of the rules avoid us bugs or leaks:

• Do not catch general exception types (May be the best rule for us. According to the case, it can be easy or difficult to enforce)
• Test for NaN correctly (easy to enforce)
• Disposable fields should be disposed (quite easy to enforce)
• Dispose should call base dispose (quite easy to enforce)
• Disposable types should declare finalizer (quite easy to enforce)

Some help us have a better design, but be careful, they may lead you to big refactoring when central API is impacted. We like

• Collection properties should be readonly (difficult to enforce in our case)
• Do not expose generic list
• member should not expose certain conrete types
• Review usuned parameters (Improves easily your API)

Someone on our project tried the performance rules with no improvement. (Actually, these rules are about micro-optimizing, which gives no result if no bottleneck identification shows microoptimizing is needed). I would suggest not starting with these ones.