22

I was just running style cop against some of my code and got a few:

SA1600: The field must have a documentation header.

Now don't get me wrong I like style cop, it's great when you work on a project with more then one person but this rule seems a bit excessive to me. Why would you want to add:

    /// <summary>
    /// blah blah blah
    /// </summary>

to the top of every variable. I'm pretty sure that I remember someone saying(Martin Fowler, Kent Beck..can't really remember ATM) that comment should say "why" not "what" and I really can't see how you can explain why on a variable.

I also find code that has comments on every variable harder to read because all you see is fluff.

My thoughts are if you have to explain what every variable is then you are really failing in terms of naming.

Does anyone else find commenting variables a bit of a code smell or is it just me.

Mogsdad
  • 44,709
  • 21
  • 151
  • 275
Nathan W
  • 54,475
  • 27
  • 99
  • 146
  • Bob Martin says *"Comments are **always** failures"* (emp. added) -CC, Ch 4, p. 54. So, given that this StyleCop rule demands comments in all these places all the time (assuming you work all your warnings, and you should because they are there to help you write good code), this rule is invalid as per Uncle Bob. If a comment, which is a necessary evil on the rare occassion, becomes necessary, you as the developer will know, so you will add it. In that case, the rules about xml format are a good thing, so I kept all rules except 1600-02, 1611, 1615, 1618 to alleviate some problems he mentions. – toddmo Mar 25 '15 at 21:16

7 Answers7

19

This is quite an old post but came across it while searching for a solution to this issue myself, so though I would offer a solution.

If you open your Settings.StyleCop file in the rules editor, select the Documentation Rules node, then in the Detailed settings section on the right de-select the Include fields option. Now you will no longer be required to document fields.

Nixus
  • 368
  • 3
  • 6
  • 2
    Good call, but it should be noted that this only works for fields and does not solve the problem of properties. – JasCav Apr 02 '13 at 16:59
  • Yeah, why not just click through all the items you disagree with until StyleCop is compatible with the code you write. That'd be the easiest path of all. – Quark Soup Feb 24 '15 at 18:47
12

I wouldn't say that commenting a variable is always a code smell (and it doesn't sound like that's what you're saying, either). I do agree that commenting every single variable, every single time is at the very least excessive, and possibly indicative of poor naming. In fact, some would argue that any comment is a code smell, and that descriptive names and short routines are more readable and prevent the situation where code has been changed, but the comments haven't been updated (which has certainly bitten me in a few legacy code bases). I don't think I'd take it quite that far, but if you can write code that is self-explanatory without extra explanation, that does seem preferable.

So yeah, basically what you said.

John Hyland
  • 6,855
  • 28
  • 32
  • Robert C.Martin Addresses these issue in his book clean code. He has an interesting take on things. – mike james Feb 11 '14 at 15:13
  • What you think is self commenting is very different that what I, the guy that comes in and reads your code six months from now, will consider self-commenting. And if the comment doesn't match the code, then fix the comment. – Quark Soup Feb 24 '15 at 18:45
6

XML Comments are slightly different than other comments.

If you set things up correctly, you can have them appear in tool tips in Visual Studio AND use them to create MSDN style documentation with Sand Castle. I think they should tell you what the thingy is doing when you don't have access to the source code.

The point is that these comments can appear without the source code that they are commenting. They should be helpful to another devloper who can't see your code and could care less how you are doing things.

I don't know about the "Cop" tool you are using, but it would be nice to have a way of signaling the tool that intend to leave a param blank. So:

    /// <param name="fubar"></param>  // Haven't gotten around to it
    /// <param name="portNumber" />   // I intend this parameter to have no help

I have worked on projects where we have to fill everything in and we get things like:

    /// <param name="portNumber">The Port Number</param> // What else could it be?

If you don't want to use the features above, go ahead turn off the Style Cop rule.

jrcs3
  • 2,790
  • 1
  • 19
  • 35
  • 2
    Yeah I use XML alot on public methods but commenting private variables with XML comments to me just seems like unneeded clutter. – Nathan W Jul 08 '09 at 00:49
2

Totally agree and the first thing I turned off in StyleCop. If you need to explain it, you have named it in a fashion which needs explanation and have failed in making the code self documenting.

Troy Hunt
  • 20,345
  • 13
  • 96
  • 151
2

For those that still come across this issue, this post how-to-change-a-stylecop-rule actually has the perfect answer.

Community
  • 1
  • 1
Franky
  • 651
  • 3
  • 11
1

You should try GhosDoc is an easy way to have documentation automated for every code member on your application. Just install it, right click on the member and select document this!

Do.This
  • 19
  • 1
  • 2
    By definition, GhosDoc can only add 'documentation' which can already be derived from the code (i.e., from the type of variable and the name). It therefore generates _useless_ documentation (almost by definition); it doesn't tell you anything you can't already determine from the code. It's only adding redundant noise. – andrewf Oct 09 '14 at 09:57
0

I think the correct answer here is "it depends." You can certainly explain the "why" of a variable being marked static/const, or business logic/restrictions on the variable's contents. Having said that, I agree that seeing every variable comment impedes readability and can indicate blindly following a standard or improper naming.

Andrew Coleson
  • 10,100
  • 8
  • 32
  • 30