From the API documentation: "Every function, constant, class, interface, class member (function, property, constant), and file must be documented, even private class members.". I think this should be changed so it is optional to place comments. Because of this requirement most comments are absolutely useless and just present because they need to be. To give some examples:

  /**
   * The renderer service.
   *
   * @var \Drupal\Core\Render\Renderer
   */
  protected $renderer;

  /**
   * The HTML response attachments processor service.
   *
   * @var \Drupal\Core\Render\AttachmentsResponseProcessorInterface
   */
  protected $htmlResponseAttachmentsProcessor;

Why does this need to have the comment "The renderer service."? From both the name of the variable and the @var it is already very clear that it's a renderer service. Code should be self explaining and adding comments for the sake of requiring comments just makes it more bulky and less readable because you are able to see less lines of actual code on your screen. In this case the previous example could be written as:

  /**
   * @var \Drupal\Core\Render\Renderer
   */
  protected $renderer;

  /**
   * @var \Drupal\Core\Render\AttachmentsResponseProcessorInterface
   */
  protected $htmlResponseAttachmentsProcessor;

This already saves 4 lines of comments while no context is lost about what these methods are. To give another example on a construct of a class:

  /**
   * Constructs a new HttpKernel decorator.
   *
   * @param \Symfony\Component\HttpKernel\HttpKernelInterface $httpKernel
   *   Original http kernel.
   * @param \Drupal\Core\Entity\EntityTypeManagerInterface $entityTypeManager
   *   Entity Type Manager.
   * @param \Drupal\Core\PageCache\ResponsePolicy\KillSwitch $killSwitch
   *   Kill switch service.
   * @param \Drupal\Core\Password\PasswordInterface
   *   Password checker service.
   */
  public function __construct(HttpKernelInterface $httpKernel, EntityTypeManagerInterface $entityTypeManager, KillSwitch $killSwitch, PasswordInterface $passwordChecker) {
    $this->httpKernel = $httpKernel;
    $this->entityTypeManager = $entityTypeManager;
    $this->killSwitch = $killSwitch;
    $this->passwordChecker = $passwordChecker;
  }

could look like:

  /**
   * @param \Symfony\Component\HttpKernel\HttpKernelInterface $httpKernel
   * @param \Drupal\Core\Entity\EntityTypeManagerInterface $entityTypeManager
   * @param \Drupal\Core\PageCache\ResponsePolicy\KillSwitch $killSwitch
   * @param \Drupal\Core\Password\PasswordInterface
   */
  public function __construct(HttpKernelInterface $httpKernel, EntityTypeManagerInterface $entityTypeManager, KillSwitch $killSwitch, PasswordInterface $passwordChecker) {
    $this->httpKernel = $httpKernel;
    $this->entityTypeManager = $entityTypeManager;
    $this->killSwitch = $killSwitch;
    $this->passwordChecker = $passwordChecker;
  }

In this case all required comments describing the function and variables are just a waste of space and make the code less readable.

TLDR; Good code doesn't need comments and is self explaining by the use of @param, @var tags etc, don't require developers to put useless comments everywhere.

Comments

slootjes created an issue. See original summary.

euphoric_mv’s picture

+1 I totally agree that descriptions in DocBlocks should be optional.

drunken monkey’s picture

-1, I like this as consistent as possible. Otherwise, we'll just have people arguing over which members are self-explanatory and which need comments. And it would be harder to spot where doc blocks were actually forgotten.

Also: “Good code doesn't need comments” is definitely wrong, but I guess you didn't mean it like that.

slootjes’s picture

@drunken_monkey #3 I do mean exactly that. When using proper names of classes, methods and variables it should be pretty much self-explaining. Please explain me how it helps to describe a property 3 (!) times:

- The name of the property should be descriptive
- The @param/@var in the doc block for auto completion in the IDE
- The description

What is useful about the 3rd one? When I look at core 99/100 times this is simply "The {Name Of The Class} Service."....why is this helpful?

The example clearly shows that you can remove 6 lines of code (comment actually) so we can fit more useful code on the screen and don't have to worry about forgetting to put down a bunch of useless comments. We're developers and we should be able to figure out what the propery is capable of when reading the name + doc block declaration without a required description.

pfrenssen’s picture

-1 One of the main strengths of Drupal is its awesome documentation.

What is useful about the 3rd one? When I look at core 99/100 times this is simply "The {Name Of The Class} Service."....why is this helpful?

First of all that's not true, the majority of classes are properly documented. If you find one that uses this pattern then please file a documentation issue so that it can be fixed.

slootjes’s picture

@pfrenssen When doing a search on "Constructs a" in core/lib PhpStorm finds "100+ matches in 98+ files". Also core is filled with "The something service.". You can do the searches yourself easily. Documentation is good if it adds something but documentation like that is purely added because it's required, not because it actually adds something.

pfrenssen’s picture

@slootjes yes I understand that you don't like writing the same documentation three times in a single document. Nobody likes to do that. But you have to look at the side of the people that have to work with your code.

You as a developer only needs to write it once but then over its lifetime your documentation will be read thousands of times by hundreds of developers who are not familiar with your code, or who do not know what this particular interface represents. It is also parsed to use in online API documentation, and IDEs can use this information to show in tooltips and when doing autocompletion.

slootjes’s picture

You as a developer only needs to write it once but then over its lifetime your documentation will be read thousands of times by hundreds of developers who are not familiar with your code, or who do not know what this particular interface represents.

If the comments would actually make sense in adding something that can't be read by code I completely agree but adding "The something service.", explain me what this adds to the readability of the code? If the interface needs further information then obviously comments should be added but in most cases the name of it is sufficient and the comments are more useful in the actual interface, not the class implementing it.

It is also parsed to use in online API documentation, and IDEs can use this information to show in tooltips and when doing autocompletion.

Again, I agree with this to a certain level when comments add something usefull but there are so many sily comments in core that are just clutter. Besides, and this sounds really harsh, but if someone is looking at __construct() of FooBar class and doesn't understand that it will return an instance of FooBar what does the comments "Construct a FooBar instance." add to the documentation which already shows the name of the class, the function __construct and it's possible arguments. For factories this could actually be helpful, that's why I would like to see it being optional instead of mandatory. Don't let developers write comments for the sake of adding useless comments, let them spend their time developing.

First of all that's not true, the majority of classes are properly documented. If you find one that uses this pattern then please file a documentation issue so that it can be fixed.

What should it be changed to instead then to make it right again so it does add value?

pfrenssen’s picture

@slootjes I understand your point, it's not necessary to repeat the same argument again and again.

I think this is quite telling where your understanding goes wrong about why we have this documentation:

If the comments would actually make sense in adding something that can't be read by code [...]

The point of documentation is to make sure you don't have to read the code. Drupal developers are humans, not PHP parsing automatons. When scanning through a code file it is much easier and faster for a human to read "The bundle info service" than it is to read @param \Drupal\Core\Entity\EntityTypeBundleInfoInterface $entity_type_bundle_info.

I can bet that you don't mind reading this documentation at all. You just don't like writing it. That's fine, nobody likes it. I don't either. But that's not very important, we have so many developers that are involved with Drupal every day that every line of Drupal code is "write once, read 10000 times".

slootjes’s picture

The point of documentation is to make sure you don't have to read the code. Drupal developers are humans, not PHP parsing automatons. When scanning through a code file it is much easier and faster for a human to read "The bundle info service" than it is to read @param \Drupal\Core\Entity\EntityTypeBundleInfoInterface $entity_type_bundle_info.

Maybe this is where we disagree then, I prefer reading the code instead.

I can bet that you don't mind reading this documentation at all. You just don't like writing it. That's fine, nobody likes it. I don't either.

I actually do mind seeing it because as a result I can fit less code on my screen. And again, I don't mind writing comments when needed to explain a piece of code, otherwise I find it a waste of time and space.

But that's not very important, we have so many developers that are involved with Drupal every day that every line of Drupal code is "write once, read 10000 times".

I wonder how many developers actually read these "The something service." and find them useful, I guess we'll never know.

What I'm stil wondering about though, you said in #5 that such comments should be fixed... what should it be fixed in or did you mean something else?

borisson_’s picture

I'm not sure about this.
I agree, there's a lot of properties where this documentation is added just to conform to the standards without adding value.
But I also agree with @drunken monkey, this would lead to more bikeshedding.

joachim’s picture

> The point of documentation is to make sure you don't have to read the code. Drupal developers are humans, not PHP parsing automatons. When scanning through a code file it is much easier and faster for a human to read "The bundle info service" than it is to read @param \Drupal\Core\Entity\EntityTypeBundleInfoInterface $entity_type_bundle_info.

Very well put!

-1 to this proposal.

Wim Leers’s picture

This makes perfect sense. I've long wished for the same thing. 👍

I do mean exactly that. When using proper names of classes, methods and variables it should be pretty much self-explaining. Please explain me how it helps to describe a property 3 (!) times:

I agree with this in the specific example you cite: in the case of dependency injection.

But then you also said:

Good code doesn't need comments

This is most definitely untrue. Nontrivial code needs comments. The examples you cite are trivial code. Not all code is trivial.

Dane Powell’s picture

I think we all understand the potential benefits and downsides to enforcing comments on variables in any given situation. Sometimes it's impossible to name a variable in a simple self-explanatory way, and in those situation comments are necessary. Other times, a comment is wildly redundant and just wastes bytes and screen real estate.

The question is what's the ratio of those situations, and what should we be willing to accept as an appropriate tradeoff.

My experience is that the ratio of worthless to useful comments is quite high, and my opinion is that our tolerance for worthless comments should be low. It's one of those situations where we can enforce a standard, but we can't enforce the spirit of the standard.

The only way we can enforce the spirit of the standard is via a human reviewer, and at that point an automated rule does nothing but create noise. So +1 from me.

Dane Powell’s picture

Title: Do not require to put comments everywhere » Do not require descriptions in variable comments

Renamed the issue to be somewhat more precise.

TravisCarden’s picture

I think there are some good arguments on both sides of this issue. Let me see if I can catalog those that have been marshaled thus far.

In favor of making comments optional:

  • Many comments add no new information.
  • Good code doesn't need comments. (Contested. Perhaps we can all agree on a modification: The more clearly code is written, the less it needs comments.)
  • Comments reduce readability by decreasing the amount of code that can fit on screen at once.
  • The ratio of worthless comments to valuable ones is too high.

In favor of keeping comments required:

  • An unconditional standard leads to greater consistency.
  • API documentation parsers and IDEs can use the comments.
  • Comments remove the need to read the code, which is a benefit to humans.
  • Comments favor the larger use case--reading code as opposed to writing it.
  • A subjective standard will lead to bikeshedding.

I'd like to add a couple of arguments that haven't been mentioned against always requiring comments.

  • Comments can get out of sync with code, and IDEs probably make the problem worse rather than better. Nothing renders a ton of comments inaccurate faster than a PhpStorm refactoring, for example. "Constructs a Foo" may seem innocent enough. But the __construct() method may not even be on the screen when you rename your class to Bar, and it's a rare patch reviewer who will think of it. I've found such artifacts in core before. This humorous code sighting may not be entirely ridiculous after all.
  • Even accurate comments can be inconsistent. It was argued that an absolute standard creates consistency, but it doesn't do so exclusively. I personally spend more time than I'd like to looking through core to see what the "standard comment" is for a given service I'm injecting. Sometimes this turns out to be a fool's errand. To illustrate with a typical example, I decided to see how many different ways core describes the entity type manager service.
    $ REF="\Drupal\Core\Entity\EntityTypeManagerInterface"; (ag -QA1 --nofilename " * @var $REF" | grep '* [^@]' | cut -c 6-; ag -QA1 --nofilename "@param $REF" | grep '* [^@]' | cut -c 8-) | sort | uniq -c | sort -nr
     304 The entity type manager.
      68 The entity type manager service.
      12 The entity manager.
      11 The entity type manager used for testing.
       7 The mocked entity type manager.
       7 Entity type manager service.
       6 Entity type manager.
       4 The entity manager service.
       3 The mocked entity manager.
       2 The mocked entity type manager used in this test.
       2 The entity type manager, used to fetch entity link templates.
       2 The entity type manager
       2 Entity Type Manager service.
       2 Entity Type Manager Service.
       1 The mock entity type manager.
       1 The entity typemanager.
       1 The entity type manager to load the revision.
       1 The entity manager used for testing.
       1 The Entity manager.
       1 The Entity Type Manager service.
       1 Stores the entity type manager.
       1 Entity type manager which performs the upcasting in the end.
    

In my mind, the strongest argument in favor of always requiring comments is probably that making them optional adds a subjective element that can't be programmatically tested for. I wonder if we could find some kind of common sense rule that doesn't have that weakness. I can imagine a satisfying compromise that begins with the following:

  1. Class constructors don't require a comment because constructors universally do and mean the same thing.
  2. Class members (@var) and function arguments (@param) that store or take services don't require a comment because keeping language consistent across the codebase is near impossible and they provide no information that the class references doesn't. I should think it would be easy to make phpcs/Coder distinguish between primitive types and class references in order to still enforce the rule and avoid subjectivity.
  3. Test objects (e.g., PhpUnit tests) don't require any comments at all, because 1) test methods tend to be highly self-explanatory (e.g., function testWitInvalidDates()), 2) the audience for non-production code is much smaller, and 3) the people who read it tend to be the kind of highly skilled and experienced developers who don't benefit from being told that EntityTypeBundleInfoInterface refers to the bundle info service.
pfrenssen’s picture

It looks to me that there is a pattern in the comments here, most people that are in favour of this are mentioning specifically that the parameter descriptions are pointless in the case of injecting dependencies from the container in a class constructor.

If the scope of this would be restricted to this particular case I would be in favor for this. In the case of service constructors it is clear that the parameters contain the dependencies from the container, and these are already specified in the *.services.yml file.

I'm even fine with leaving out the entire @param definitions. We should require the use of the "service" qualifier in the docblock so that it is clear for users that this is a service that uses dependency injection. For example:


  /**
   * Constructs a new Entity plugin manager ***service***.
   */
  public function __construct(\Traversable $namespaces, ModuleHandlerInterface $module_handler, CacheBackendInterface $cache, TranslationInterface $string_translation, ClassResolverInterface $class_resolver, EntityLastInstalledSchemaRepositoryInterface $entity_last_installed_schema_repository) {

I think that all other methods (be it public ones defined in an interface, and protected ones in individual classes) should still be properly documented.

slootjes’s picture

Wim Leers

This is most definitely untrue. Nontrivial code needs comments. The examples you cite are trivial code. Not all code is trivial.

I stand corrected; I think we are on the same page regarding this however you describe it in a better way.

pfrenssen

We should require the use of the "service" qualifier in the docblock so that it is clear for users that this is a service that uses dependency injection. For example:

/**
   * Constructs a new Entity plugin manager ***service***.
   */
  public function __construct(\Traversable $namespaces, ModuleHandlerInterface $module_handler, CacheBackendInterface $cache, TranslationInterface $string_translation, ClassResolverInterface $class_resolver, EntityLastInstalledSchemaRepositoryInterface $entity_last_installed_schema_repository) {

The part "Constructs a new Entity plugin manager ***service***." is exactly what I believe should not be required as it does not add value to what (code) is already there. The whole idea behind the __construct is to create a new instance of the current class, why does this need to be repeated in every class in the code base? If a function is named properly and does not contain non trivial logic I don't see the point in adding a comment.