What do people think of this?
I’m about 90% in agreement with it. One of my biggest gripes in modern trends is docblocks above method headers that do nothing but repeat information available in the method header. Repeating the argument names is a horrible waste of time.
A high percentage of comments are useless. As a simple experiment I looked at a common and popular component: Symfony HTTP Kernel to look at the comments:
The header’s fine… but look at this:
/**
* Constructor.
*
* @param EventDispatcherInterface $dispatcher An EventDispatcherInterface instance
* @param ControllerResolverInterface $resolver A ControllerResolverInterface instance
* @param RequestStack $requestStack A stack for master/sub requests
*
* @api
*/
public function __construct(EventDispatcherInterface $dispatcher, ControllerResolverInterface $resolver, RequestStack $requestStack = null)
{
$this->dispatcher = $dispatcher;
$this->resolver = $resolver;
$this->requestStack = $requestStack ?: new RequestStack();
}
… “Constructor”, 100% useless.
* @param EventDispatcherInterface $dispatcher An EventDispatcherInterface instance
* @param ControllerResolverInterface $resolver A ControllerResolverInterface instance
This tells us nothing that isn’t in the method header.
* @param RequestStack $requestStack A stack for master/sub requests
Gives us some information that isn’t in the method header but it’s not particularly useful.
/**
* {@inheritdoc}
*
* @api
*/
Absolutely useless for anyone looking at the code. More useful for an IDE, perhaps, but an IDE should probably inherit docs from the interface anyway. 100% pointless.
/**
* @throws \LogicException If the request stack is empty
*
* @internal
*/
public function terminateWithException(\Exception $exception)
{
Finally a helpful comment: We know that it can throw an exception, something we can’t see from the method header… No description of what the method actually does though.
/**
* Handles a request to convert it to a response.
*
* Exceptions are not caught.
*
* @param Request $request A Request instance
* @param int $type The type of the request (one of HttpKernelInterface::MASTER_REQUEST or HttpKernelInterface::SUB_REQUEST)
*
* @return Response A Response instance
*
* @throws \LogicException If one of the listener does not behave as expected
* @throws NotFoundHttpException When controller cannot be found
*/
private function handleRaw(Request $request, $type = self::MASTER_REQUEST)
A mixed bag here. @param mostly repeated information from the method header. Listing available constants for the $type param is helpful, although not very useful without explaining the difference between the types, to work that out I need to look through the code anyway.
// request
$event = new GetResponseEvent($this, $request, $type);
$this->dispatcher->dispatch(KernelEvents::REQUEST, $event);
What is //request adding here? If anything it’s confusing. What is it referring to? There is a variable called $request but it seems unrelated to the comment. Removing the comment would not change the understandability of this code
// load controller
if (false === $controller = $this->resolver->getController($request))
is there a difference between get and load? If so, either the comment or method name is wrong, if not, the comment is irrelevant.
// controller arguments
$arguments = $this->resolver->getArguments($request, $controller);
Arguably useful, although probably not given the aptly named variables.
// call controller
$response = call_user_func_array($controller, $arguments);
100% pointless comment.
// view
if (!$response instanceof Response) {
$event = new GetResponseForControllerResultEvent($this, $request, $type, $response);
$this->dispatcher->dispatch(KernelEvents::VIEW, $event);
//view unhelpful, adds nothing of use at all.
// the user may have forgotten to return something
if (null === $response) {
$msg .= ' Did you forget to add a return statement somewhere in your controller?';
}
throw new \LogicException($msg);
This is well explained by the exception message, I’m not sure the comment adds anything here.
/**
* Filters a response object.
*
* @param Response $response A Response instance
* @param Request $request An error message in case the response is not a Response object
* @param int $type The type of the request (one of HttpKernelInterface::MASTER_REQUEST or HttpKernelInterface::SUB_REQUEST)
*
* @return Response The filtered Response instance
*
* @throws \RuntimeException if the passed object is not a Response instance
*/
private function filterResponse(Response $response, Request $request, $type)
{
filters a response object. We can see that from the header, the description does not explain what the function actually does. Filters it in what way?
* @param Request $request An error message in case the response is not a Response object
This is at best confusing… an error message is generally a string, why is this a Request object?
* @param int $type The type of the request (one of HttpKernelInterface::MASTER_REQUEST or HttpKernelInterface::SUB_REQUEST)
Again, explaining the options is useful, but no detail on what they actually do.
I’m not going to go through the rest of the class but it continues in this vein. Most of the comments either add nothing or repeat information that is in the method header. There are a few arguably useful comments but they are far outnumbered by pointless ones.
Interestingly, the most ambiguously named method does not have any comments at all:
``php
}
private function varToString($var)
{
Just to be clear, I'm not picking on this class/project/author, I just wanted to use it as a case-study and look at how comments are actually used in the wild.
It was also the first class I looked at so may not be representative, but it does mirror my own experiences and reinforce my (mostly) agreement with the article I linked to.
With PHPMetrics and other tools using % of comment lines as an important metric, it just seems these comments exist to improve the score under analysis without actually helping developers looking at the code.
…j/k
Saying that comments are evil is far too extreme, however I agree that redundant comments are bad practice and descriptive function and variable names are better. However, there are cases where descriptions are necessary to make the code clear.
