Recently I read a comment on Twitter by Nikola Poša:
I guess the discussion on my thread is going in the wrong direction because I left out a crucial hashtag: #NoCommentsInCode - avoid comments in code, write descriptive classes, methods, variables.https://t.co/MuHoOFXCvV
— Nikola Poša (@nikolaposa) July 13, 2018
He was providing us with a useful suggestion, one that I myself have been following ever since reading "Clean Code" by Robert Martin. The paraphrased suggestion in that book, as well as in the tweet, is to consider a comment to be a naming issue in disguise, and to solve that issue, instead of keeping the comment. By the way, the book has some very nice examples of how comments should and should not be used.
The code comment as a naming issue
I think in most cases, indeed, we don't need comments. Common suggestions are, like Nikola suggests as well:
- Introduce a variable with a meaningful name, which can hold the result of a complicated expression.
- Introduce a method with a meaningful name, which can hide a piece of complicated logic.
The "hiding" that a method does, provides you with two interesting options:
- The method name can summarize a number of statements.
- The method name can represent a concept that is of a higher level than the lower-level things that are going on inside the method.
Option 1 is useful, but if you can go for option 2, it'll be the most valuable option.
Consider Nikola's example:
if ($date->hour < 9 || $date->hour > 17) { //Off-hours?
}
Option 1 would entail something like:
if ($this->hourIsBetween9And17($date->hour)) { //Off-hours?
}
Option 2 would be much more interesting and useful:
if ($this->isAnOffHour($date->hour)) {
}
This introduces abstraction, where the client doesn't care about the concrete details of determining whether or not a certain hour is an off-hour, but only wants to know if the given hour is indeed an off-hour.
#NoComments?
I realized I'm always applying the following rule: if I feel like writing a comment, I look for a way in which I can change the code so that I don't have to write a comment anymore. Also, when I encounter a comment in an existing code base, I apply the same rule, and look for ways to remove the comment. Because, as you may know, code can't lie, but comments can, so let's get rid of them before they become a risk.
However, contrary to my own advise, I also realized that I've actually been writing more and more comments over the past few months. So what's going on?
A little soul-searching revealed that I've been adding code comments that can be categorized as follows:
- Explanatory comments: ~70%
- TODO comments: ~20%
- WTF comments: ~10%
Explanatory comments
I've been adding explanatory comments to parts in the code that need an explanation, or a canonical description of the situation. Most often these comments can be found near a couple of statements in the domain model. The comment then explains why a certain business rule should be applied, and how the code beneath the comment actually accomplishes this. Of course, I try to match the words in the comment with the words in the code itself, so they are tied together.
Before adding an explanatory comment, I often try to rephrase the explanation as a test first. This is usually the better option, since it will make sure that the code and its documentation will never diverge. The test is the documentation that describes a piece of the code, and if the code is no longer truthful to that description, the test will fail. An even better option is to apply behavior-driven development, where tests can actually be written as stories with examples, and they will serve as automated tests at the same time.
Still, some things just need a whole paragraph of explaining, and in that case, I happily put the explanation in the code, and take my time (and a long-form commenting style like /* ... */
) to explain what's going on.
"Here's a question: why don't you create a separate document, instead of a code comment?"
Good question; in my experience most developers don't care about documentation. They don't read it, don't write it, and don't update it. This means that documentation becomes untrustworthy very, very quickly. And still, nobody cares. All everybody cares about is diving into the code and making that change that needs to be done. So, knowing ourselves, I think it's much better to add a comment, than to write separate documentation.
"Okay, another question: don't you think a commit message would be more suitable for explanatory comments?
I agree, a commit message is great for a bit of explanation from your side. However, commit messages aren't as readily available as code comments are. They aren't searchable. They are not "in your face" - you don't see them automatically when you open the code. In fact, you'd have to dive into the history of that code, to figure out all the reasoning behind a piece of code. And besides, everybody would have to make small commits and write proper commit messages, which definitely doesn't happen in the real world.
By the way, I often stumble upon comments like this:
autoSelect: false, // important !!!!
I think these are meant to be explanatory, but what they're missing is the "why" part of the explanation. Whenever you add an explanatory comment, make sure you don't forget it!
TODO comments
Besides adding explanatory comments to code, TODO comments are my favorite when doing a major refactoring, Mikado style. I sprinkle lots of these little comments across the area of the code base that I'm working on. You have to remember the one rule that should be applied when using TODO comments: when merging/releasing your work, those TODOs should be gone. This means that, as part of a code review, you should verify that no TODOs were added inside the new branch. Just like comments in general become outdated very quickly, TODO comments expire very soon as well. What happens most of the time is:
The TODO comment never was an intention-of-future-work, it actually was an apology in disguise.
// TODO we should inject this service as a constructor dependency instead
The work to be done is too much and will never be done.
// TODO rewrite using Twig
The TODO comment marks some technical debt that was introduced. The work will also never be done.
// TODO make it possible to use this with a different tenant
From years of experience with TODO comments, my advice is to turn them into DONE comments right away, and move the comment to your commit message.
WTF comments
These comments are very much like TODO comments, because you're basically saying: "this ain't right", but you're also saying: "We're not going to fix this". Either because it works (even if that's a miracle), or because it would take too much time to make it right. You should still add a comment, to explain what's going on, so the next person who reads this code understands what's going on, and doesn't have to spend a lot of time trying to figure it out, just like you did now.
Conclusion
When I write a comment, I do it so that the next person who looks at this piece of code doesn't have to figure it out again. Fun fact: this is the same approach I have to many other things in life. For example, I don't want to spend time and energy over the same thought again and again, in particular if it's a troublesome thought. Same for things I have to do: if I think of it once, I write it down, so I never have to be interrupted by the thought again (until I check my TODO list, that is!).
While thinking about code comments, I became aware of an implicit rule about how I write code: I always aim to de-personify it. I try to write code that doesn't show hints of the author as a person (with emotions, habits, years of experience, etc.). When adding the types of comments discussed earlier, the code starts to have more signs of human authorship. And I actually think that's a good thing: we're working on this code together, but we're also experiencing it. Adding comments increases ownership of the code, and shows empathy towards other programmers, like our current and future team members. And to ourselves, when we dive back into the code after a holiday break. I'd say: happy commenting!
Very nice article. I completely agree that most code comments are signs of inappropriate abstraction and bad class/method design. If we end up with big methods with hundreds lines of code, which need comments for each of their sub-sections, then it is a good time to refactor code and extract this piece of code/section into a method of its own(preferably a private method since it should not be part of the public API).
Thanks for this article... one thing that I really hate are @deprecated comments without a explanation :/
Good point; yes, that definitely needs an explanation.
Great post Matthias, I agree completely.
Just want to comment on the point about commit messages not being "in your face". I have been working lately on some TypeScript projects using VS Code (instead of PhpStorm/WebStorm) and there is an extension called GitLens which shows commit message for the current line the carret is on as well as above blocks such ass classes. It's been great and I have started to rely on commit messages as kind of like "notes on the side" or the commenting feature you have in MS Word (and Google docs).
https://github.com/eamodio/...
So I think it really shows how conventions like these discussed in this post can change depending on the tools you are using and as certain features become ubiquitous across most tools. Can't wait for this feature to be available in PhpStorm.
Thanks!
Well, that sounds like a very interesting tool to use. I'll wait for that moment then, and agree with you that practices will change over time because of these tools. Just consider what PhpStorm itself did for all those PHP projects!
Great post!
I've found that sometimes referencing a particular issue/ticket in the code is explanatory, not as a TODO, not as an explanation, but as a reference to a discussion. Example: excluding some items from certain calculations, in this case, is a business requirement and using a proper naming it's ok (EXCLUDED_ITEMS). While this explains the purpose of the variable/change (and the intended use in the code) it fails to explain why.
Sometimes this explanation is easy (and could be inlined as an explanatory comment), but bringing a full discussion (that could happen on a ticket) into the code is not ideal also, what we do is just reference/mention the ticket and a clear (and short) enough description. This allows the next developer (or future me) to go into a detailed conversation about why some change was introduced.
Thank you very much!
Okay, that's an interesting option. I'm a bit skeptic to be honest though. I'd be worried about keeping the two together (basically the issue system is now part of your software and has to be available as long as the code is used). Also, I wonder if it's really helpful to have to whole discussion available. I'm sure it depends on what you're describing but I'm always hesitant to let developers go over and over the same reasoning. You could also apply the rather brutal "summarize & shut up" strategy ;) An alternative would be to keep a decision log in a project file. You could summarize the discussion, point out how it's not ideal, and how it's the most practical thing to do at this point.
It's true that it doesn't work for all cases and shouldn't be applied blindly. Like everything in our profession, there is no silver bullet 😂.
I think that on the codebase of a company probably the issue trackers live even more than the code itself 🤔 (or than some developers).
What I don't like about the decision log is the duplication. It's really easy for the decision log to fall off sync with the original ticket/issue. I guess it also depends on the discipline of the developers. I wouldn't encourage to "annotate" every line of code, the same way that putting comments on everything is counter-productive. But only on those cases where some additional context is needed.
jorgelbg 5+
Indeed :)
Also, yes, if any strategy you apply (or any advice I give) comes with risk of documentation becoming outdated, then maybe look for something else that works for you.
What's your opinion on TODOs with with a reference to a corresponding ticket number, as a replacement to WTF type comments.
I think they will tend to have the same issue as most backlog items have: they are never going to be fixed. In particular if there's not too much value in it for the business or the development team.
As long as people don't confuse code comments with PhpDoc blocks, I agree with this article. However, at times, adding a little bit more information to a method may help future devs. For example, if i have a repository method names: getCustomerByEmailAdress(EmailAdress $emailAdress): ?Customer; I might add the following Doc Block /* Gets the Customer by their Email Address if found. Of course, the return type shows that it can be null but doesn't say why. Also, if someone is browsing the API docs, then it is easier to understand without looking at the code. Yet again, I agree, if the code needs comments to describe what it does, then the code should be refactored, especially the naming of things.
Sounds about right :) I think throwing an exception (and announcing it in the docblock using @throws) might help as well, as it might communicate the reason for failure in a better than
null
.Sure thing but in this case (my example) can search for a Member and determine if they're registered or not. However, in other circumstances, when a Member is required to be fetched, then I'd throw an exception as you said :)
Ah, in that case I would go for a boolean return value ("isRegistered") and don't rely on null or an instance of check.
You're correct for a Domain Repository but Infrastructure wise, it is faster to carry check and fetch at once.
A code without comments or document is crap. It's simple, IT'S CRAP. What Robert Martin says is arrogance.
Mathias, thanks for sharing your experience. I think it was Uncle Bob who also said that code is 1-part art and 1-part science. Given that, I'd suggest that there's no clear rule for how many comments to write, but that experience helps you make an informed decision.
Sure thing! I only realized that my recent experiences have been different from previous ones and that in fact my opinion was a bit shifting on this matter. Like we always have to reevaluate these rules I guess :)