Missing XML comment for publicly visible type or member ‘considered harmful’

One of the nice things about .net is that you can automatically generate an .xml file for the xmldoc comments.

One of the worst things however is that by default, this leads to compiler warnings (and, in case “warnings as errors is enabled” – as it should be – leads to a failed compilation).

1>FooWrapper.cs(5,18,5,28): warning CS1591: Missing XML comment for publicly visible type or member 'FooWrapper'
1>FooWrapper.cs(7,21,7,24): warning CS1591: Missing XML comment for publicly visible type or member 'FooWrapper.Foo'
1>FooWrapper.cs(9,16,9,26): warning CS1591: Missing XML comment for publicly visible type or member 'FooWrapper.FooWrapper()'
1>FooWrapper.cs(14,16,14,26): warning CS1591: Missing XML comment for publicly visible type or member 'FooWrapper.FooWrapper(bool)'
1>FooWrapper.cs(19,32,19,39): warning CS1591: Missing XML comment for publicly visible type or member 'FooWrapper.Dispose(bool)'
1>FooWrapper.cs(23,21,23,28): warning CS1591: Missing XML comment for publicly visible type or member 'FooWrapper.Dispose()'

This often leads to the desire to add comments to everything, possibly even using automated tools, which results in a class like this:

/// <summary>
/// A Class to wrap a Foo value.
/// </summary>
public class FooWrapper: IDisposable
{
    /// <summary>
    /// The wrapped Foo value
    /// </summary>
    public bool Foo { get; }

    /// <summary>
    /// Initializes a new instance of the <see cref="FooWrapper"/> class.
    /// </summary>
    public FooWrapper()
    {
    }

    /// <summary>
    /// Initializes a new instance of the <see cref="FooWrapper"/> class,
    /// with the given value for foo.
    /// </summary>
    public FooWrapper(bool foo)
    {
        Foo = foo;
    }

    /// <summary>
    /// Releases unmanaged and - optionally - managed resources.
    /// </summary>
    /// <param name="disposing">
    ///     <c>true</c> to release both managed and unmanaged resources;
    ///     <c>false</c> to release only unmanaged resources.
    /// </param>
    protected virtual void Dispose(bool disposing)
    {
    }

    /// <summary>
    /// Performs application-defined tasks associated with freeing,
    /// releasing, or resetting unmanaged resources.
    /// </summary>
    public void Dispose()
    {
        Dispose(true);
        GC.SuppressFinalize(this);
    }
}

What’s wrong with this class? The signal-to-noise ratio is atrocious, and I consider this downright harmful to understanding what the class does, and of course the comments get outdated even quicker the more there are. Let’s break it down into the useful and useless:

FooWrapper: A Class to wrap a Foo value.

Potentially useful. This tells me what the class is meant for, but sane naming of the class already does that. It could be more useful to explain why Foo needs to be wrapped and when I should use this instead of just passing around the Foo value directly, and when to subclass it.

Foo: The wrapped Foo value

Useless. I know it’s a wrapped Foo value because it’s a property named Foo in a class named FooWrapper. What could make this useful is by explaining what this Foo value represents, and what I would use it for.

FooWrapper: Initializes a new instance of the <see cref=”FooWrapper”/> class.

Useless. I know that it initializes a new instance of the FooWrapper class, because it’s a constructor of the FooWrapper class. That’s what constructors do, they initialize new instances of the class they are part of. There is no other information conveyed here – no information about potential side-effects, about valid input arguments, about potential Exceptions, nothing.

The overload that tells me that the bool foo argument will initialize Foo to the given foo is also useless, because – well, duh, what else is it’s going to do?

Dispose: Releases resources

Useless. IDisposable is a fundamental language feature, so both the reason for this method and the Dispose pattern are well known. What isn’t known is if there’s anything noteworthy – does it dispose any values that were passed into the constructor? (Important e.g., when passing Streams around – whose job is it to close/dispose the stream in the end?). Are there negative side effects if NOT disposing in time?

Useful comments

Now, this class is arguably a very simplistic example. But that makes it also a very good example, because many applications and libraries contain tons of these simple classes. And many times, it feels that they are commented like this out of Malicious Compliance in order to shut the compiler warnings up or fulfill some “All Code must be documented” rule.

The real solution is to suppress the 1591 warning and only add comments to code that do something non-obvious or critical to pay attention to. In the case of the above example class, the best I can come up with is below.

/// <summary>
/// This class wraps a Foo value, captured
/// from when the operation was started.
///
/// Operations that need to capture additional values
/// should derive from this to add their own additional
/// values.
/// </summary>
public class FooWrapper : IDisposable
{
    /// <summary>
    /// The Foo that was wrapped at the beginning of the operation.
    /// Changes to the Foo value in the holder class do not change this value.
    /// </summary>
    public bool Foo { get; }

    public FooWrapper()
    {

    }

    public FooWrapper(bool foo)
    {
        Foo = foo;
    }

    /// <summary>
    /// This class implements IDisposable to allow
    /// derived classes to capture values that need to be
    /// disposed when the operation is finished.
    /// </summary>
    protected virtual void Dispose(bool disposing)
    {
    }

    public void Dispose()
    {
        Dispose(true);
        GC.SuppressFinalize(this);
    }
}

Now, the comments convey useful information: We learn the intent of the class – that’s something not obvious from the code. Though arguably, this class should now be called InitialOperationState or something like that. It also explains why/when to create subclasses for it. The comment on the property now explains something about the purpose, rather than just reiterating the code in prose. And finally, the Dispose(bool) method explains why it’s there. The constructors and Dispose() methods do not need any comments – they don’t do anything worth commenting.

And because I suppressed 1591, the compiler is happy as well.