Everyone agrees that static code analysis is good. However it isn't in widespread use. It has become easier with FXCop / Code Analysis built into VS2010. Personally I'm for "educating developers" over SCA.
Focus on Prevention over Post-facto Inspection, just like Bugs.
- Make sure that the number of warnings reduce over time.
- Make sure that the code isn't being engulfed by green suppression markups. Refine the set of rules as needed.
Cannot stress this enough, To avoid SCA pain (drowning in a stormy sea of SCA warnings) START EARLY.
I was asked to come up with a good set of SCA rules for acceptance test code. Here's the specific tool I'm looking at -
Parasoft dotTest (I'm using v9.1).
Setup is easy.. Liked the following aspects.
- You can install into a separate profile i.e. avoid messing up your default VS2010 setup.
- You can get a floating network license that can be shared across users. You need to check "Release automatically when idle" in the License options.
So I created mine by creating a new configuration.
Main Menu - Parasoft - Test Configurations... Click New
Adding categories one at a time by cross-verifying it with a medium sized codebase ~80 files / ~8K LOC. Took about 2 mins. The rules can be found under the "Static" tab on the right, when your configuration is selected.
- TIP! The search box can be used to filter the hierarchical view below.
- TIP! Right click on a specific rule to bring up its documentation. Or parameterize the rule if that is supported.
You can export the configuration then to a file that can be shared across your team.
Since I was looking for SCA on test code (which is run within a controlled env), I turned off the following categories
- COM Guidelines
- Casting guidelines
- Implementing Finalize and Dispose
- Security Inspection
- Serialization Guidelines
- Security Policy Rules
- WPF
That accounts for 96 / 444 rules. Next the rules that didn't make the cut (Rule Id in brackets, use search/filter to find a rule.)
- Keep line length within predefined parameters (80) [BRM.MLL-3]
- Includes whitespaces on the left - in short indentation causes rules to cause noise warnings. Unusable.
- Avoid class, struct, or interface names which are more than 14 characters long [BRM.LONGNAMES-4]
- Default limit too short. Cannot customize. I prefer long readable names over this warning.
- Always provide appropriate file header (copyright information, etc.) [BRM.FILEHEADER-3]
- Needs a comment to be the first line in the file - even before the using statements. Does not recognize standard XML comments.
- Does not exclude non-public classes. Noise
- Avoid unused private methods [CMUG.MU.AUPM-2]
- Noise warnings. Flagged methods that were being called.
- Avoid using get accessor with side effects [CMUG.PRU.AGAS-3]
- Flagged all properties that were internally calling String.Format. Noise.
- Use an array instead of many parameters [CMUG.MU.AIMP-5]
- Noise again. Recommends turning get(string elementId, string keyPropertyName, string valuePropertyName) to take a string array instead of well named parameters. Counter-productive.
- Make events public [CMUG.EVU.MEP-5]
- Put using statements in alphabetical order [CS.USO-4]
- VS2010 sorts usings such that System.* namespaces are grouped above the rest.
This rule doesn't seem to know about this. A workaround is to override this behavior in VS2010 but then that's something that each developer would have to change.
- NAMING
- Avoid language-specific names for members [NG.ATNC.ALSN.MEMBER-5] : flags up "_constaints" for containing "int"
- Avoid method names that conflict with keywords [NG.WC.KEYWORD.METHOD-3] : Will not let you have methods called Get, Stop, Select - which are sometimes the most intuitive names.
- Use Pascal case for class names [NG.CAPSTY.PASCAL.CLASS-5] : UIDriverBase is a valid name as per Design guidelines but flagged up by this rule. See Capitalization Rules for Acronyms.
- Use the property's type name as part of the property name [NG.PRN.APNCTN-4] : This is a "Consider" not a "Do" in the guidelines. Embedding type name in the property is risky w.r.t. future refactorings (e.g. if you change the type of the property with name MonitorIPString from String to StringBuilder. Misleading.
- Use the suffix 'EventHandler' for event handler names [NG.EVN.EHNEEH-3] : Broken. Here's a sample - Message: Change type of event 'X.Driver+AppLaunchedEventHander AppLaunched' or add 'EventHandler' suffix to type 'Driver+AppLaunchedEventHander'.
- Use flags attribute for bit field enum [NG.ETN.UFABFE-3] : Flagged simple 1 member enum with non-plural name as bit-field enums. Broken.
- Avoid too many function calls from a single function [OOM.FCSF-4]
- Limit of 10 is easily breached. Non-customizable. Includes property access within the method.
- Use 'GetType()' in the 'Equals()' method implementation [PB.EQL-3]
- Does not exclude overloads of Equals. e.g. Equals(MyType t) does not need to check type of t.
- Use upper case for method name with name length <=2 [NG.CAPSTY.UPPER.MN-5], [NG.CAPSTY.UPPER.PROP-5]
- Asks me to rename a method called At(params) to AT(params). Was not able to find any design guideline to support this.
- Avoid public default constructor when a non-default constructor is present [CMUG.CU.PUBDEFAULTCTOR-3]
- Contradicts Exceptions should implement common constructors [ERR.EICC-4]. The exception rule won.
- Follow the limit for Number Of Static Methods [METRICS.NOSM-1]
- This one flagged a false +ve. Static Class had 1 static var, 3 public and 3 private methods. Rule violated: 10 is not less than 10. Refactoring often leads to lots of small private methods.
Then there was a bunch that
- was auto-enforced by C# e.g. Name indexer 'item' or Name event accessors 'add' and 'remove'.
- Some were guidelines not rules.
- Some were plain out of date e.g. Provide the same accessibility for the property and its accessors [CLS.ACAC-3] This flags up a common pattern of properties whose setters are private. The explanation points to an outdated (.Net 1.1) version. We've have come a long way since then...
Turned off another 80-100 too. Finally ended up with 196 / 444 rules enabled.
There is also a report out and code-metrics at the end too. But to summarize, I found Parasoft dotTest to be a bit unpolished.. the infrastructure is there. Analysis is quick enough but the rules need to be improved. Not enough documentation online - except from Parasoft itself.
Verdict: Passable, but for now prefer Code Analysis that comes bundled with Visual Studio 2010.