At what point do Stylecop settings stop being useful and start becoming annoying?

Question

I work in a team where we use extensive ruleset in StyleCop and I am wondering what are the thoughts on the general point where such a tool stops being useful and starts becomes annoying. We also use GhostDoc so code is riddled with XML comments which make the code much harder to read and thus review. I have no problem with XML comments and find them very useful in places but are they really needed on every field and property?

We have the admirable goal of "each project must have 0 Warnings when built" but surely this goal needs to be against a reasonable StyleCop ruleset otherwise valuable time is wasted in "fixing" the cause of the StyleCop warnings.

What are the thoughts on this?

EDIT I'm now actually wondering what is the argument for a tool like stylecop AT ALL? Why not ditch it and let sensible coding standards and good code reviews take care of the rest? Especially in a good competent team? Surely then the task of getting 0 Warnings would actually add value as all Warnings would be relevant.

I think the only advantage of GhostDoc is it saves you a vital few seconds in writing an XML comment from scratch. I don't think you should accept the generated comment without editing it - which is counter-productive maybe.

Here's a combination of a Stylecop rule (SA1642: ConstructorSummaryDocumentationMustBeginWithStandardText) being met by GhostDoc generated xml comment - does either add any value at the end of the day?

    /// <summary>
    /// Initializes a new instance of the <see cref="SomeCustomType"/> class.
    /// </summary>
    /// <param name="someParameter">The someParameter.</param>
    public SomeCustomType(string someParameter)
    {
    }

Answer 1

This is more of a rant than a question, I think, but I agree with you that:

• over-enforced style rules are a bad thing.

Whilst there should obviously be guidelines for source code formatting, over-prescriptive unbreakable rules lead to unpleasant corner cases. Sticking strictly to the rules in all cases can generate unreadably messy or over-wrapped code.

Coding is a variety of writing, and as such Orwell's Bonus Rule—“Break any of these rules sooner than saying anything outright barbarous​”—needs to apply to coding style guides too.

I am sceptical that automated style enforcement is a good idea, in a team of competent programmers who can set and understand style guides. Automated lints are useful for catching accidental mistakes, but if applied with highly prescriptive laws for code formatting they cannot take account of Orwell's rule. With a strong ruleset, this may force you to write less-maintainable code under the guise of maintainability.

If you've got less-competent coders in your team, whose output is a jumble unless forced into style, then enforcement might be a good idea. (But then you've probably got bigger problems!)

• automated comments are a very bad thing

I hadn't seen GhostDoc before, but I'm actually a little bit shocked.

The very example on their own front page:

/// <summary>
///     Determines the size of the page buffer.
/// </summary>
/// <param name="initialPageBufferSize">
///     Initial size of the page buffer.
/// </param>
/// <returns></returns>
public int determineBufferSize(int initialPageBufferSize) {

is almost the canonical example of a Bad Comment, adding absolutely zero insight into the code it documents. This is absolutely worse than no comment-doc.

All the in-source-doc schemas that followed Javadoc are a little bit suspect at times, since they clutter the source code with material that's often aimed at end-users—a quite different audience to those reading the code. But this is the absolute pits. I can't imagine who thought this was a good idea.

Answer 2

StyleCop is a tool. It's not supposed to be perfect out of the box, and it's not supposed to meet everyone's needs.

Personally I say "Yes, it's important" - because when you're running a team of devs, StyleCop helps you ensure that your coding guidelines are being adhered to. That's exactly its purpose: to evaluate coding standards in an automated, measurable, consistent manner. If you don't want the ability to do that in your build process then you're right - it's a waste of time.

You say it yourself: the zero-warnings goal "needs to be against a reasonable StyleCop ruleset." There's no point running any tool with a configuration that doesn't match your needs. If a rule is "annoying" for you then turn it off - but for someone else it might be vitally important.

As to your "does either add value" question: yes. People underestimate the value of consistency. If all of your constructors have the same style of comment, if all the Intellisense for properties in your project have the same structure, it's one less mental hurdle (no matter how small) to deal with. And with tools that automate it, it's at almost zero effort. What's to complain about?

Answer 3

When it takes more time to code around the rule, than what you ever could get back by reduced maintainence.

As you know, fixing a bug takes a lot more time than writing it, so you can still do quite a lot extra work to make the code more robust and maintainable before you reach the threshold.

Answer 4

It's vitally important that code is well written and readable/maintainable, but we use Visual Studio and Resharper's automatic code-formatting helpers for our code, and AtomineerUtils to keep XML documentation comments in a strictly defined and tidy format.

As a result, the main StyleCop rules are irrelevant, as our code always adheres to the important rules "by default". The lesser StyleCop rules tend to be rather too strict for everyday use. (Most of these rules only make tiny, if any, improvements to the quality or readability of code, so we find the cost of adhering to them unacceptable. We allow our programmers a bit of "freedom of expression" - as long as their code is easily readable by others in the team, we don't mind minor variations in coding style). So after evaluating StyleCop I was unable to find any real world benefit.

In contrast we find FXCop very useful, because the problems that it highlights are more than just about minor readability issues - it picks up serious bugs and performance issues from time to time.

Answer 5

There are a few points of your question that attracted my attention, so I would like to add some thoughts to the previous answers.

I have no problem with XML comments and find them very useful in places but are they really needed on every field and property?

Some fields and properties are so obvious to everyone, that they don't need an explanation. For example, if a class Coordinate has the properties X, Y and Z, there is nothing to explain in the comments.

But when it comes to a tool like StyleCop, a tool cannot make a difference between an obvious property and a property which has chances to be difficult to understand when discovering the source code for the first time. So no, comments are not needed everywhere, but we either enforce comments on every field and property, or we disable the rule and let the developer decide.

Here's a combination of a Stylecop rule (SA1642: ConstructorSummaryDocumentationMustBeginWithStandardText) being met by GhostDoc generated xml comment - does either add any value at the end of the day?

Somehow. Some tools display constructors just like other methods, and you can't make any difference visually between the two (unless you keep in mind the name of the class). The XML comment, on the other hand, is so clear that it makes it very easy to understand that this is a constructor.

By the way, what else would you write here?

• Something else? Not having a standard for the constructors will make it difficult to read code and to understand that a method is a constructor in views where both are displayed in the same way.

• No comment at all? It can be a solution, since such comment can be easily generated from the name of the class. But it will make things more complicate (why auto-generating a comment on constructors and not on other methods?). Also, if you provide no description for this constructor, how can somebody know what is someParameter?

---

Finally, to answer your question, nobody can say that every StyleCop rule is always useful in every case. But remember that the nonsense here is the goal of "each project must have 0 Warnings when built", not the StyleCop itself. If you're an experienced developer and write clean code, there is no reason to not turn off some StyleCop rules, at least if you understand well what they mean, why they are here and what will be the consequence to not follow the rule.

In our company, we have a policy to use StyleCop for every project and if there are hundreds of warnings on a tiny project, well, there is a problem. At the same time, I often had situations where I disabled a few StyleCop rules on whole classes, just because it will waste time to enforce them, and it does not bring anything to anyone. On the other hand, I didn't appreciate at all when my colleague disabled FxCop/StyleCop just to be able to write names of classes, methods and properties in French (I'm in a French company which also works with English-spoken developers), so I would say that for some people, both tools must be enabled every time, with no ability to disable them.

Answer 6

It's useful to have a set style that everybody uses, and for new code StyleCop will enforce this to everyone's benefit.

But when adding StyleCop to an existing project which was written with a different style, you will get hundreds, or thousands, or tens of thousands of violations of what are, essentially, arbitrary rules such as:

• if must be followed by a space
• Function parameters must all appear on the same line or each on a separate line
• Single line comments must begin with a space
• Fields must have a blank line in between
• An opening brace must not be followed by a blank line
• field names must not begin with underscore or be prefixed
• Default parameter should not be used, use overloads instead
• do not prefix local calls with this
• Order of Using statements

It simply makes no sense to edit thousands of existing files with hundreds of thousands of lines of code just to comply with such rules.

• With any change there is a possibility that the change will introduce bugs.
• If you are not making changes to fix bugs, or introduce features, why are you doing it?

If the answer is "to shut up StyleCop" - and be honest - there is more than one way of doing that.

Your goal should be to eliminate warnings which don't represent bugs, so that you can see - and then fix - the more problematic warnings, which may do. The presence of 1500 warnings about the formatting of single-line comments is an obstacle you can do without.

Therefore, in a legacy codebase:

• Edit the ruleset to disable any arbitrary, stylistic rules with more than a few violations. These are more trouble than they are worth: Nothing bad will happen because the project uses underscores for fields, but if you accidentally cause a naming clash trying to fix it, it may.
• Where there are one or two violations, consider carefully whether it is better to use a suppression file than edit working, tested code just to shut up a stylistic warning. You don't need to put in a changeset with thousands of whitespace changes in hundreds of files. It's easy to introduce a behavioural difference when you think you are just reformatting a block, and easy to miss such a thing reviewing the changed lines.
• Only if you are satisfied that the rule violation represents a bug should you change the code. So what if parameters aren't on the same line? Reformat the code, by all means, if you need to understand it right now and the format makes that hard. But not just because StyleCop says so.
• Even adding parameter documentation is not necessarily harmless: If you don't know the code inside out you may simply be baking in your own misconceptions for the next developer.