Microsoft StyleCop, Totalitarian Rules

I got turned onto a fairly new tool by a friend and was interested in checking it out. It's called Microsoft StyleCop and done in the style of FxCop but rather than analyzing assemblies it looks at code for formatting and style rules.

The original use was within Microsoft for keeping code across all teams consistent. Imagine having to deal with hundreds of developers moving around an organization like Microsoft where there are dozens of major teams (Windows, Office, Visual Studio, Games, etc.) and millions of lines of code. It helps if the code is consistent in style so people moving between teams don't have to re-learn how the "style" of that new team works. Makes sense and I can see where there's benefit, even in smaller organizations.

As an example, here's a small User Interface utility class for long(ish) running operations. It's simple but works and is easy to use:

using System;
using System.Windows.Forms;
 
namespace UserInterface.Common
{
    /// <summary>
    /// Utility class used to display a wait cursor
    /// while a long operation takes place and
    /// guarantee that it will be removed on exit.
    /// </summary>
    /// <example>
    /// using(new WaitCursor())
    /// {
    ///     // long running operation goes here...
    /// }
    /// </example>
    internal class WaitCursor : IDisposable
    {
        private readonly Cursor _cursor;
 
        public WaitCursor()
        {
            _cursor = Cursor.Current;
            Cursor.Current = Cursors.WaitCursor;
        }
 
        public void Dispose()
        {
            Cursor.Current = _cursor;
        }
    }
}

One could even argue here that the class documentation header is somewhat excessive, but this is meant to be a framework class that any application could use and maybe deserves the <example/> tag.

Maybe it's my formatting style but I like using the underscore prefix for class fields. This is for two reasons. First, I don't have to use "this." all over the place (so the compile can tell between a parameter variable, local variable, and class variable. Secondly, I can immediately recognize that "_cursor" is a class wide variable. Sometimes we have a policy of only referencing variables via Properties so for example I could tell if this was a problem if I saw a method other than a getter/setter use this variable. The debate on underscore readability can be fought some other time, but for me it works.

After running StyleCop on this single file (I wasn't about to deal with all the voilations in the entire solution) it created this list of violations:

  • SA1600: The field must have a documentation header.
  • SA1600: The constructor must have a documentation header.
  • SA1600: The method must have a documentation header.
  • SA1633: The file has no header, the header Xml is invalid, or the header is not located at the top of the file.
  • SA1309: Field names must not start with an underscore.
  • SA1200: All using directives must be placed inside of the namespace.
  • SA1200: All using directives must be placed inside of the namespace.
  • SA1101: The call to _cursor must begin with the "this." prefix to indicate that the item is a member of this class.
  • SA1101: The call to _cursor must begin with the "this." prefix to indicate that the item is a member of this class.

Hmmm, that's a lot of problems for such a little file. Now grant you, when you run FxCop against any assembly (even Microsoft ones) you get a whack of "violations". They range from actual, real, critical errors that should be fixed, to annoyances like not enough members in a namespace. Any team using FxCop generally has to sift through all the violations and decide, as a team, what makes sense to enforce and what to ignore. StyleCop has similar capabilities through it's SourceAnalysisSettingsEditor program (buried in the Program Files directory where the tool is installed or via right-click on the Project you're performing analysis on). It allows rules to be ignored but it's pretty simplistic.

I think one of the biggest issues with the tool is the fact that it goes all Chef Ramsey on your ass, even if its code created by Microsoft in the first place. For example create a new WinForms project and run source analysis on it. You'll get 20+ errors (even if you ignore the .Designer generated file). You can exclude designer files and generated files through the settings of the tool, but still its extra work and more friction to use the tool this way. It might be debated that the boilerplate code Visual Studio generates for new files (which you can modify but again, more work) should conform to the StyleCop guidelines. After all Microsoft produced both tools. However this would probably anger the universe as the "new" boilerplate code would look different from the "old".

There are other niggly bits like the tool insisting on documenting private variables so pretty much every property, method, and variable (public, private, or otherwise) will all have at least an extra 3 lines added to it should you enforce this rule. More work, more noise.

I'm somewhat torn on the formatting issues here. What it suggests doesn't completely jive with me, but that might be style. After all, the tool is designed to provide consistency of code formatting across multiple disparate sources. However unless you're a company with *no* code and start with this tool, you'll probably be ignoring certain rules (or groups of rules) or doing a lot of work to try to bring your code to match the violations you'll stumble on. It's like writing unit tests after the fact. Unit tests are good, but writing them after the code is done (and even shipped) has a somewhat diminished cost to benefit ratio.

In getting this simple class up to snuff I had to not have the urge to hit Ctrl+Alt+F in ReSharper (ReSharper's default formatting totally blows the violations) and hold my nose on a few things (like scattering the code with "this." prefixes and seemingly redundant documentation headers). Documentation is a good thing but my spidey-sense has taught me that comments mean something might be wrong with the code (not descriptive enough, should have been refactored into a well-named method, etc.). It only took a few minutes to shuffle things around, but I look at large codebases that you could point this tool at and think of weeks of code reformatting and what a cost that would be.

In any case, here's the final class with the changes to "conform" to StyleCop's way of life:

//-----------------------------------------------------------------------
// <copyright file="WaitCursor.cs" company="MyCompany">
//     Copyright MyCompany. All rights reserved.
// </copyright>
//-----------------------------------------------------------------------
namespace UserInterface.Common
{
    using System;
    using System.Windows.Forms;
 
    /// <summary>
    /// Utility class used to display a wait cursor
    /// while a long operation takes place and
    /// guarantee that it will be removed on exit.
    /// </summary>
    /// <example>
    /// using(new WaitCursor())
    /// {
    ///     // long running operation goes here...
    /// }
    /// </example>
    internal class WaitCursor : IDisposable
    {
        /// <summary>
        /// Holds the cursor so it can be set on Dispose
        /// </summary>
        private readonly Cursor cursor;
 
        /// <summary>
        /// Default constructor
        /// </summary>
        public WaitCursor()
        {
            this.cursor = Cursor.Current;
            Cursor.Current = Cursors.WaitCursor;
        }
 
        /// <summary>
        /// Resets the cursor back to it's previous state
        /// </summary>
        public void Dispose()
        {
            Cursor.Current = this.cursor;
        }
    }
}

I feel this is a lot of noise. Sure, it could be consistent if all files were like this but readability is a funny thing. You want code to be readable and to me this last version (after StyleCop) is less readable than the first. Documenting default constructors? Pretty useless in any system. What more can you say except "Create an instance of <T>". Documenting private variables? Another nitpick but why should I? In this class you could probably rename it be _previousCursorStyle or something to be more descriptive and then what is documentation going to give me. Have I got anything extra from the tool as a result? I don't think so.

If it's all about consistency something we've done is to share a ReSharper reformatting file which tells R# how to format code (when you press Ctrl+Alt+F or choose Reformat Code from the ReSharper menu). It has let us do things like not wrap interface implementations in regions (regions are evil) and decide how our code should be formatted like curly braces, spacing, etc. However it completely doesn't match StyleCop in style or form. You could probably tweak ReSharper to match StyleCop "to a certain extent" but I disagree on certain rules that are baked into the tool.

For example take "this." having to prefix a variable. To me a file full of "this" prefixes is just more noise. ReSharper agrees with me because it flags "this.PropertyName" as redundant. Maybe the debate whether it's a parameter or a field is probably a non-issue. If a method is short, one can immediately identify the local variables and distinguish them from member fields and properties with a glance. If it is long, then there is probably a bigger issue than the code style: the method simply should be refactored. For whatever reason, Microsoft thinks "this." is important and more readable. Go figure.

Rules can be excluded but it's a binary operation. Currently StyleCop doesn't have any facility on differentiating between "errors" and "warnings" or "suggestions". Maybe it should but then with all the exclusions and errors->warnings you could configure, the value of the tool quickly diminishes. Enough errors being turned into warnings and you would have to argue the value of the tool at all, versus a ReSharper template.

In any case, feel free to try the tool out yourself. If you're starting with a brand new codebase and are willing to change your style to match a tool then this might work for you. For me, even with public frameworks I'm working on, the tool seems to be more regiment and rules than being fit for purpose. I suppose if you buy into the "All your StyleCop are belong to us" mentality, it'll go far for you.

For me it's just lots of extra noise that seems to provide little value but YMMV.

8 Comments

  • Quite sincerely, I disagree with everything you just said.

    First of all you sound like exactly the same sort of thing that prevents from a single code style (my style is way better than yours, etc). Frankly, to a reasonable extend, every style is good, you just need to get used.

    If you don't want to use stylecop then don't, but don't base your conclusions on the use of resharper. Since when can you justify your code style on how easy it is to work with a propietary, non-free tool? Maybe it works for you but you certainly cannot generalize it as many people simply don't use R#

    Finally, stylecop is just a tool, tools help us by improving or speeding things we do but they don't think for us. When I design code I let the tool point out possible problems or improvements, the is my brain who is in charge of deciding whether they really are or not.

  • I prefer applying standard formatting automagically instead of validating it. ReSharper, e.g., does the magic for a whole soluion - et voilà: All source files from all developers use the same style.

  • Wouldn't reformatting with Resharper essentially show the entire file as being changed in source control? On a multi-person project it would be horrible to do that after things had been checked in ... the churn would be huge.

  • What are the other options besides StyleCop, Resharper?

    Thanks

  • I disagree with you on not using "this" with internal properties of a class. I think it's a lot more clear that way and it causes intellisense to get a hint that I want something so it displays a useful list.

    I found another program that does this called CodeIt.Right and it's a lot like FxCop but useful. It finds this kind of stuff and then offers to fix it for you so instead of just validating it will validate and then correct (but only if you tell it to do so). Really easy way to enforce coding style as well as a bunch of other stuff. Only drawback is that it isn't free (it's not expensive though) but then neither is resharper so whatever.

  • "goes all Chef Ramsey on your ass"

    LOL, Oh, man, that was worth the price of admission. *wipes a tear from his eye*

  • Check out Stylecop for Resharper. It makes resharper adhere to all of the stylecop rules.

    Great plugin.

  • Following some rules is better than no rules, also it makes sense for entire company to be on same page either it be resharper or stylecop. Remember this is not about your code better or mine its about how other people can understand your code in better way

Comments have been disabled for this content.