Review : Parasoft DotTest Static Code Analysis

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.
    • 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.

No comments:

Post a Comment