Thou shall not write any comments ;-)
Writing clean code is one of those things that separates a good programmer from a bad one. If you can write code that is easy to follow/understand, it will save you and other programmers countless time, prevents bugs and in general will make you a happier programmer.
There was a time when I believed you had to write comments to document how your code works. Then I read Clean Code from Robert C. Martin or also know as "Uncle Bob". A whole chapter is dedicated to why you should not write any comments. I do not think you are a bad programmer if you write comments but you should probably be asking yourself if you can improve the readability of your code in other ways.
What's wrong with comments?
For starters, comments can contain false (outdated) information. The comment can come from a time when the programmer initially wrote the code but it was never updated with the latest updates, business requirements, bug fixes, ... Now the comment will contain wrong information and will only add to the confusion. It is doing to opposite of the initial goal: to clarify. Or perhaps, the comment was written as a prank. Whatever the reason: never trust a comment, always follow the code.
Than there are some comments that are completely redundant. Do you really need to write a comment "increment i with 1" with the code $i++? Of course not, the code is simple enough to follow by itself.
But how do I understand what is going on if I don't write comments?
If you need to add comments to help you understand your code, you are probably doing something wrong. Did you use meaningful names for your variables, methods and classes? Do you adhere to the Single Responsibility Principle? Your controller, method should only do one thing and it should do it good. If there is too much going on, parts of the code could be abstracted.
Take the following (fictional) Laravel example:
/**
* Get the overview of the articles for the authenticated user, they can be limited by providing type param
* @param Request $request
* @return View
* @throws ValidationException
*/
public function index(Request $request): View
{
//validate the request
//check if topic was provided with a correct option
if ($request->has('topic') && !in_array($request->get('topic'), ['php', 'html', 'css'] ) {
throw new ValidationException('type needs to be one of the following options: php, html, css');
}
//check if order_by was provided with a correct option
if ($request->has('order_by') && !in_array($request->get('order_by'), ['date_created', 'archived', 'topic'] ) {
throw new ValidationException('type needs to be one of the following options: date_created, archived, topic');
}
//check if order_dir was provided with a correct option
if ($request->has('order_direction') && !in_array($request->get('order_direction'), ['asc', 'desc'] ) {
throw new ValidationException('order_direction needs to be either asc or desc');
}
//get the authenticated user
$user = Auth::user();
//prepare the query
$articles = Article::where('author_id', $user->id);
//did the user provide a topic?
if ($request->has('topic') {
$articles = $articles->where('topic', $articlesQuery->get('topic'));
}
//did the user provide an order by?
if ($request->has('order_by') && $request->has('order_direction')){
$articles = $articles->orderBy($request->get('order_by'), $request->get('order_direction'));
} else {
//use the default ordering
$articles = $articles->orderBy($request->get('date_cre', 'desc'));
}
//return the view
return View('articles.index', [
'title' => __('Overview of your articles"),
'articles' => $articles->get()
]);
}The comments above are rather redundant, incomplete and contain wrong information (can you find it?).One improvement would be to use better names, some constants, the Laravel validate helper tool and the default values that you can provide as a the second argument for getting a request. Comments are no longer needed:
/**
* @param Request $request
* @return View
* @throws ValidationException
*/
public function getArticles(Request $request): View
{
$request->validate([
'topic' => ['sometimes', Rule::in(SELF::ALLOWED_TOPICS)],
'order_by' => ['sometimes', Rule::in(SELF::ALLOWED_ORDER_BY)],
'order_direction' => ['sometimes', Rule::in(SELF::ALLOWED_ORDER_DIRECTION)],
]);
$articlesQuery = Article::where('author_id', Auth::user()->id);
if ($request->has('topic')) {
$articlesQuery->where('topic', $request->get('topic'));
}
$articlesOrderBy = $request->get('order_by', SELF::DEFAULT_ORDER_BY);
$articlesOrderDirection = $request->get('order_direction', SELF::DEFAULT_ORDER_DIRECTION);
$articlesQuery = $articlesQuery->orderBy($articlesOrderBy, $articlesOrderDirection);
return View('articles.index', [
'title' => __('Overview of your articles'),
'articles' => $articlesQuery->get()
]);
}
Another improvement would be to use an invokable controller, for instance: "getAuthenticatedUserArticlesController", a custom formRequest and a service. I would recommend to do this as you can also better test the separate parts and it is much more extensible without polluting the controller.
/**
* @param GetAuthenticatedUserArticlesRequest $getAuthenticatedUserArticlesRequest
* @param GetUserArticlesService $getUserArticlesService
* @return View
* @throws ValidationException
*/
public function __invoke(
GetAuthenticatedUserArticlesRequest $getAuthenticatedUserArticlesRequest,
GetUserArticlesService $getUserArticlesService
): View {
$articles = $getUserArticlesService->execute(Auth::user(), $getAuthenticatedUserArticlesRequest->toDataObject());
return View('articles.index', [
'title' => __('Overview of your articles'),
'articles' => $articles,
]);
}What about docblocks/PHPDoc?
Docblocks are after all also comments, although specifically formatted.
My opinion is that if doesn't add any value, it is not needed and it distracts your attention. The latest versions of PHP will allow you to define the return types and strong type the arguments so the code is self-explanatory. There are some cases that add value: thrown exceptions and certain type declarations. Take the following interface:
/**
* @param GetQuestionsRequest $getQuestionsRequest
* @return array
*/
public function getQuestions(GetQuestionsRequest $getQuestionsRequest): array
All the information in the docblock above is not only redundant. Two important pieces of information are missing.
GetQuestionsRequest validates the request and will thrown an exception if the conditions are not met. Otherwise an array of Question objects will be returned.
With that information, we can update the docblock below. This will also help our IDE to bubble up the thrown exceptions and to autocomplete the public methods of Question.
/**
* @param GetQuestionsRequest $getQuestionsRequest
* @return Question[]
* @throws ValidationException
*/
public function getQuestions(GetQuestionsRequest $getQuestionsRequest): array