Rule negation

Jul 15, 2010 at 6:25 PM
Edited Jul 15, 2010 at 6:26 PM
Sometimes my style god does not agree with your style god. In those situations I can only suppress a rule, but I can't use StyleCop to enforce my (superior) view of the world.

Take rule SA1122: UseStringEmptyForEmptyStrings for example. Our team thinks that "" should be used instead of string.Empty. I can suppress rule SA1122, but I cannot trigger a warning if string.Empty is used.

I can see envision four different solutions in order from best to worst:

  1. Add a negate toggle to some rules. If on it would negate the meaning of the rule. I could see where some rules could not be negated.
  2. Make the on/off toggle for some rules into a tri-state toggle of negative, off, positive. The default would be positive. A good amount of time would need to be spent making the interface intuitive.
  3. Add a UseEmptyStringsForStringEmpty rule. This would greatly increase the number of rules. It also introduces the problem of rule conflict.
  4. Your style god is always correct. All dissenters should just learn to live with it.

This is an issue for more than just SA1122. For example, SA1200: UsingDirectivesMustBePlacedWithinNamespace could use the same treatment. But it doesn't make sense for SA1201: ElementsMustAppearInTheCorrectOrder. What does it mean to negate SA1201? The elements must be in reverse order? The elements must be in an incorrect order?

Coordinator
Jul 15, 2010 at 6:36 PM

option 4 - "one style to rule them all, one style to bind them"

Jul 15, 2010 at 6:57 PM

using "" instead of String.Empty is actually a performance issue and not just a style issue.  code analysis (FxCop) will also complain about this as well.

But as for the negation thing, I like this idea for some rules.  We would prefer to see tabs instead of spaces in files, but that is not how the stock rule is written.  I ended up writing my own rule to enforce tabs, but it would be nice to be able to negate the existing rule.

Developer
Jul 15, 2010 at 7:48 PM

As I understand initial StyleCop intentions, it's very close to option 4.
But it could not be so useful without customization, so here plugin possibility appears.

Logically, I don't see a big difference between having several "paired" rules and converting existing ones from "on-off" toggle to "tri-state" (or some other) - it's more likely to a question of interface/usability.

So it seems the best opion for you will be creating custom rules - fortunately you can already do it (thanks to Jason once again for plugin support!).

Best regards,
Oleg Shuruev 

Jul 15, 2010 at 8:17 PM

It does take some time to get used to writing StyleCop compliant code. I know there was some rules I didn't like when I first started using the tool, but once we each get past our own egos that our own style is better the process goes a lot smoother.

Regarding your decision of the SA1122 rule... you're creating a new copy of an empty string in memory each time you use "" rather than just pointing to the single read-only string.Empty.

There is a specific reason why the using directives need to be inside the namespace for SA1200, I forget what that reason is since it's been so long since I looked, but there is a reason. Something around types defined within the file may not be found if multiple namespaces are defined in the same file. That sounds vaguely familiar.

As for SA1201, there is a specific order of members in your objects. I would suggest reading http://www.thewayithink.co.uk/stylecop/sa1201.htm to familiarize yourself with that order.

Jul 15, 2010 at 9:05 PM
I did not want this discussion to degenerate into a debate about a specific rules. Yesterday I was reading on multiple websites about how there is no significant difference in speed between using "" and String.Empty. Today I cannot find good links. Here are some starting links:
  • http://stackoverflow.com/questions/263191/in-c-should-i-use-string-empty-or-string-empty-or
  • http://www.csharper.net/blog/string_empty_vs_empty_quotes___quot__quot___vs_string_length.aspx
  • http://www.devnewsgroups.net/group/microsoft.public.dotnet.framework/topic56496.aspx
Again, the point is not if this rule is good or bad, or if I like it or not. The point is that if someone disagrees with the rule, simply suppressing it may not be what they want. Maybe they want to enforce the opposite. SA1122 was an example.

Paired rules lead to conflict. What if both UseStringEmptyForEmptyStrings and UseEmptyStringsForStringEmpty are enabled?

This has nothing to do with my ego or the rules I like or dislike. It is about the rules that my team have agreed upon. The correct style is not my style, it is not your style, it is not StyleCop default style. It is the style that a group of developers agree upon. What is important is that they write code that is consistent with their agreed upon standard.

As for SA1201, I understand that it imposes a specific order for members in objects. I actually think that it is a good rule. My point was that it is an example of a rule that does not negate well. I was trying to get across that I understood that not all rules are negateable or made sense with a tri-state toggle.

Developer
Jul 15, 2010 at 9:52 PM

This has nothing to do with my ego or the rules I like or dislike. It is about the rules that my team have agreed upon. The correct style is not my style, it is not your style, it is not StyleCop default style. It is the style that a group of developers agree upon. What is important is that they write code that is consistent with their agreed upon standard.

It seems that StyleCop's code principle:
StyleCop provides value by enforcing a common set of style rules for C# code. StyleCop will continue to ship with a single, consistent set of rules, with minimal rule configuration allowed. Developers can implement their own rules if they so choose.
doesn't sound like "tool for enforcing any style for every team".

However, as for me, personally I absolutely agree with you. I believe that there is nothing bad with more flexibility and often you really need to enforce your specific style, not the "worldwide" one.
And I consider StyleCop the best tool for doing this, using third-party or own-written rules (e.g. StyleCop+ removes all barriers in naming constraints).

Paired rules lead to conflict. What if both UseStringEmptyForEmptyStrings and UseEmptyStringsForStringEmpty are enabled?

Doesn't seem to be a problem.

First, final StyleCop running usually takes place on build server, with settings specified by some "administrator" guy.

Second, you can make your rules to give warning about configuration conflict (I did such thing in StyleCop+ - if you try to turn on extended original rules without turning off the original ones, you will immediately get a warning during analysing process).

Good luck!

Best regards,
Oleg Shuruev