4

Should documentation be added for constructor parameters that are passed via dependency injection? In my current project I have decided to omit documentation to describe each of these parameters and my reasoning for this is as follows:

  • These constructors are not meant to be called by a developer (outside of unit testing, but I will only be using integration testing in this project) so why bother documenting the parameters passed to them? The parameters are passed by a DI framework, which isn't going to read the documentation when constructing these classes.

  • Many of the services being injected are singletons. By adding parameter documentation for every constructor to which they are injecting, I'm adding a bunch of duplicate documentation all over the project. Now, if the semantics of how that service is used changes later, I will have to update the comments in every constructor. It seems like a better solution for the developer to just go look at the class documentation for the service.

Is this reasoning sound? Are there any reasons you would disagree with my rationale for excluding these comments? Are there potential benefits of including these comments that I have overlooked?

To add some further context to this question, these are XML comments in C# that will eventually also be used to generate documentation for the project. I am in the process of integrating StyleCop into the project which has a rule to make sure that all function arguments are documented. So this issue arose because I am being forced to choose between documenting these parameters or suppressing the warning messages for each constructor using injection. I do not want to disable the rule globally because I do want to ensure that parameters for regular methods and constructors are documented.

Robert Harvey
  • 198,589
  • 55
  • 464
  • 673
mark.monteiro
  • 149
  • 4
  • 1
    Is the type name of those injected parameters "self-describing enough" for your taste? – Doc Brown Aug 23 '19 at 20:33
  • 4
    These parameters are really no different than any other kind of parameters. Also, when you are documenting a parameter, you are not necessarily describing the concrete service, but the role it plays within the confines of the class, for the benefit of the developer who is trying to understand how the class fits in a larger context. So if you have something to say that's relevant in that sense, then document them, if not, then don't (in particular, comments that just restate the exact same information contained in the code itself are useless clutter). – Filip Milovanović Aug 23 '19 at 22:24
  • are you asking about XML comments? Do you enforce them somehow? (they are easy to enforce on all public/protected members in VS C# project settings: Properties -> Build -> Output -> XML Documentation File). There's a good chance that your constructor or its class can be marked as internal i.e. XML comment is optional, and if comment doesn't provide any information beyond trivial then you don't have to write it. – KolA Aug 26 '19 at 12:47
  • @KolA I have added an edit to my question with additional context. I am using StyleCop to enforce rules about documenting function parameters. – mark.monteiro Aug 26 '19 at 15:17
  • @mark.monteiro looks like the only way to avoid explicit suppression is to configure StyleCop to allow constructors with internal (or protected internal) access modifier to be undocumented; and make all "injection" constructors internal. I don't know whether StyleCop or your DI container allows this. – KolA Aug 26 '19 at 15:50
  • also suppressions can be moved to separate source file to keep source code clean but it is far from ideal. The only alternative I can think of is to write custom attribute and Code Analysis rule (via Roslyn) but this is probably overkill. – KolA Aug 26 '19 at 15:52
  • Thanks @KolA, for now I have been using the `[SuppressMessage]` attribute, which doesn't bother me that much in terms of keeping the code clean. I am not going to make the constructors `internal` because I will be using them for unit tests in a separate assembly. This question is more about deciding whether I should suppress the messages at all, rather than how to suppress them – mark.monteiro Aug 26 '19 at 16:53

2 Answers2

4

If you can't refactor without changing your comments your comments are too coupled to your code.

Comments should never describe how the code works. Comments should describe why the code is does what it does. Or at least, what it was supposed to do. Show your intent not your method of achieving your intent.

As Doc Brown points out a good name makes leaving off comments perfectly acceptable. Comments should never be redundant duplications of what the code tells you.

Now all that said, if you go completely without comments your code had damn well be extremely readable with no surprises.

Read your code while giving thought to how it will look to the next programmer. Give them what they need and no more.

There is nothing special about parameters, DI or otherwise, that requires comments in any special way.

So please, spare us comments that add no new information and are needlessly coupled to the code like this:

//reads a file
public void read(File file){
    ...
}

Comments should add something new

//Parse all numbers and load them into a collection
public void read(File file){
    ...
}

Which is better but should lead to this:

//Loads into a collection
public void parseNumbers(File file){
    ...
}

Or even this:

public List<Integer> parseInts(File file){
    ...
}

And now you don't need any comments at all. You already know what it does.

But you don't have to take my word for it. I've asked about this before.

candied_orange
  • 102,279
  • 24
  • 197
  • 315
  • 1
    however XML comments are more coupled to the code as they annotate individual members and parameters (if topic starter had them in mind) – KolA Aug 26 '19 at 12:49
0

You should never comment how, only why. The code is the how-comment in it self (If its not refactors are in place).

Dependencies are 99% of the time irrelevant to the why-question.

Anders
  • 671
  • 1
  • 4
  • 11