I'm having some discussions with my new colleagues regarding commenting. We both like Clean Code, and I'm perfectly fine with the fact that inline code comments should be avoided and that class and methods names should be used to express what they do.

However, I'm a big fan of adding small class summaries that tries to explain the purpose of the class and what is actually represents, primarily so that its easy to maintain the single responsibility principle pattern. I'm also used to adding one-line summaries to methods that explains what the method is supposed to do. A typical example is the simple method

public Product GetById(int productId) {...}

I'm adding the following method summary

/// <summary>
/// Retrieves a product by its id, returns null if no product was found.
/// </summary

I believe that the fact that the method returns null should be documented. A developer that wants to call a method should not have to open up my code in order to see if the method returns null or throws an exception. Sometimes it's part of an interface, so the developer doesn't even know which underlying code is running?

However, my colleagues think that these kinds of comments are "code smell" and that "comments are always failures" (Robert C. Martin).

Is there a way to express and communicate these types of knowledge without adding comments? Since I'm a big fan of Robert C. Martin, I'm getting a bit confused. Are summaries the same as comments and therefore always failures?

This is not a question about in-line comments.

  • 37
    Robert Martin said "Comments are alwaysfailures"? Well, then he's a fringe extremist, and should be taken with a pinch of salt. (Yes, I'm aware that he writes like this for rhetorical purposes, to get his message across. My point is, so should you.) – Kilian Foth Jun 4 '15 at 8:35
  • 16
    Uncle Bob's books should come with a 1kg bag of salt... – AK_ Jun 4 '15 at 9:44
  • 6
    If you are following Robert Martin, the documentation for the null case should be a test. That is, you should have a test showing in which case the method can return null. Alternatively, since this is Java, a @Nullable annotation would also be better than the comment. – Martin Epsz Jun 4 '15 at 11:45
  • 12
    @Bjorn I own a copy of Clean Code and have read it cover to cover more than once. Yes, Uncle Bob prefers code to be self-documenting, but there are multiple examples of comments in his own code in the book. The point is if you feel compelled to write a comment, try really hard to change the code rather than adding the comment, but do not disallow comments entirely (even in-line comments). – user22815 Jun 4 '15 at 14:59
  • 6
    The method should be called TryGetById and the comment should be removed. – usr Jun 4 '15 at 15:14
up vote 114 down vote accepted

As others have said, there's a difference between API-documenting comments and in-line comments. From my perspective, the main difference is that an in-line comment is read alongside the code, whereas a documentation comment is read alongside the signature of whatever you're commenting.

Given this, we can apply the same DRY principle. Is the comment saying the same thing as the signature? Let's look at your example:

Retrieves a product by its id

This part just repeats what we already see from the name GetById plus the return type Product. It also raises the question what the difference between "getting" and "retrieving" is, and what bearing code vs. comment has on that distinction. So it's needless and slightly confusing. If anything, it's getting in the way of the actually useful, second part of the comment:

returns null if no product was found.

Ah! That's something we definitely can't know for sure just from the signature, and provides useful information.


Now take this a step further. When people talk about comments as code smells, the question isn't whether the code as it is needs a comment, but whether the comment indicates that the code could be written better, to express the information in the comment. That's what "code smell" means- it doesn't mean "don't do this!", it means "if you're doing this, it could be a sign there's a problem".

So if your colleagues tell you this comment about null is a code smell, you should simply ask them: "Okay, how should I express this then?" If they have a feasible answer, you've learned something. If not, it'll probably kill their complaints dead.


Regarding this specific case, generally the null issue is well known to be a difficult one. There's a reason code bases are littered with guard clauses, why null checks are a popular precondition for code contracts, why the existence of null has been called a "billion-dollar mistake". There aren't that many viable options. One popular one, though, found in C# is the Try... convention:

public bool TryGetById(int productId, out Product product);

In other languages, it may be idiomatic to use a type (often called something like Optional or Maybe) to indicate a result that may or may not be there:

public Optional<Product> GetById(int productId);

So in a way, this anti-comment stance has gotten us somewhere: we've at least thought about whether this comment represents a smell, and what alternatives might exist for us.

Whether we should actually prefer these over the original signature is a whole other debate, but we at least have options for expressing through code rather than comments what happens when no product is found. You should discuss with your colleagues which of these options they think is better and why, and hopefully help move on beyond blanket dogmatic statements about comments.

  • 9
    Or the Linq-equivalent of Try..., ...OrDefault, which return default(T) if the clause would lead to an empty result. – Bart van Nierop Jun 4 '15 at 10:29
  • 4
    I really appreciate your distinction between inline-code-comments and documentation comments, and the examples given :) – Rachel Jun 4 '15 at 15:41
  • 2
    The possible return values of a function should be evident by its signature. The TryGetValue pattern is a reasonable way of doing this in C#, but most functional languages have a better way of representing a missing value. Read more here – AlexFoxGill Jun 4 '15 at 16:40
  • 2
    @BenAaronson: If one wants to have a generic interface that can support covariance, one could use either T TryGetValue(params, ref bool success) for any type T, or T TryGetValue(params), with null indicating failure, for class-constrained type T, but the TryGetXX pattern that returns bool is incompatible with covariance. – supercat Jun 4 '15 at 17:07
  • 6
    In Java 8, you can return an Optional<Product> to indicate that there might not be a Product returned from the method. – Wim Deblauwe Jun 5 '15 at 6:18

The Robert C. Martin quote is taken out of context. Here is the quote with a bit more context:

Nothing can be quite so helpful as a well-placed comment. Nothing can clutter up a module more than frivolous dogmatic comments. Nothing can be quite so damaging as an old crufty comment that propagates lies and misinformation.

Comments are not like Schindler's List. They are not "pure good." Indeed, comments are, at best, a necessary evil. If our programming languages were expressive enough, or if we had the talent to subtly wield those languages to express our intent, we would not need comments very much -- perhaps not at all.

The proper use of comments is to compensate for our failure to express ourself in code. Note that I used the word failure. I meant it. Comments are always failures. We must have them because we cannot always figure out how to express ourselves without them, but their use is not a cause for celebration.

So when you find yourself in a position where you need to write a comment, think it through and see whether there isn't some way to turn the tables and express yourself in code. Every time you express yourself in code, you should pat yourself on the back. Every time you write a comment, you should grimace and feel the failure of your ability of expression.

(Copied from here, but the original quote is from Clean Code: A Handbook of Agile Software Craftsmanship)

How this quote is reduced into "Comments are always failures" is a good example of how some people will take a sensible quote out of context and turning it into stupid dogma.


API documentation (like javadoc) is supposed to document the API so the user can use it without having to read the source code. So in this case the documentation should explain what the method does. Now you can argue that "Retrieves a product by its id" is redundant because it is already indicated by the method name, but the information that null may be returned is definitely important to document, since this in not in any way obvious.

If you want to avoid the necessity of the comment, you have to remove the underlying problem (which is the use of null as a valid return value), by making the API more explicit. For example you could return some kind of Option<Product> type, so the type signature itself communicates clearly what will be returned in case the product is not found.

But in any case it is not realistic to document an API fully just through method names and type signatures. Use doc-comments for any additional non-obvious information which the user should know. Take say the API documentation from DateTime.AddMonths() in the BCL:

The AddMonths method calculates the resulting month and year, taking into account leap years and the number of days in a month, then adjusts the day part of the resulting DateTime object. If the resulting day is not a valid day in the resulting month, the last valid day of the resulting month is used. For example, March 31st + 1 month = April 30th. The time-of-day part of the resulting DateTime object remains the same as this instance.

There is no way you could express this using just the method name and signature! Of course your class documentation might not require this level of detail, it is just an example.


Inline comments are not bad either.

Bad comments are bad. For example comments which only explains what can be trivially seen from the code, the classical example being:

// increment x by one
x++;

Comments which explains something which could be made clear by renaming a variable or method or otherwise restructuring the code, is a code smell:

// data1 is the collection of tasks which failed during execution
var data1 = getData1();

These are the kind of comments Martin rails against. The comment is a symptom of a failure to write clear code - in this case to use self-explanatory names for variables and methods. The comment itself is of course not the problem, the problem is we need the comment to understand the code.

But comments should be used to explain everything which is not obvious from the code, e.g. why the code is written a certain non-obvious way:

// need to reset foo before calling bar due to a bug in the foo component.
foo.reset()
foo.bar();

Comments which explains what an overly convoluted piece of code does is also a smell, but the fix is not to outlaw comments, the fix is the fix the code! In the real word, convoluted code does happen (hopefully only temporarily until a refactor) but no ordinary developer writes perfect clean code the first time. When convoluted code happens it is much better to write a comment explaining what it does than not write a comment. This comment will also make it easier to refactor later.

Sometimes code is unavoidably complex. It may be an complicated algorithm, or it may be code sacrificing clarity for performance reasons. Again comments are necessary.

  • 13
    There's also the case where your code handles a situation that just is complicated, and no simple code can handle it. – gnasher729 Jun 4 '15 at 9:02
  • 6
    Good point, gnasher. This seem to often happen when you have to optimize some piece of code for performance. – JacquesB Jun 4 '15 at 9:07
  • 9
    Even a comment in x++ may be good if it's something like "increment x by one, wrapping around if it's UINT32_MAX"; anyone who knows the language spec would know that incrementing a uint32_t will wrap, but without the comment one may not know whether such wrapping was an expected part of the algorithm being implemented. – supercat Jun 4 '15 at 17:10
  • 5
    @l0b0: I hope you are joking! – JacquesB Jun 4 '15 at 23:48
  • 9
    @l0b0 There is no such thing as temporary code. The code never gets refactored because the business was happy with the outcome and won't approve funding to fix it. Five years from now, some junior developer will see this code, you're not even using WizBug 4.0 anymore since you've replaced it with Bugtrocity v9, so "Bug123" means nothing to him. He now thinks this is how permanent code is supposed to be, and proceeds to be a terrible developer his entire career. Think of the children. Don't write temporary code. – corsiKa Jun 5 '15 at 17:44

There is a difference between commenting your code and documenting your code.

  • Comments are needed to maintain the code later, that is change the code itself.

    Comments may indeed be perceived as problematic. The extreme point would be to say that they always indicate a problem, either within your code (code too difficult to understand) or within the language (language unable to be expressive enough; for instance, the fact that the method never returns null could be expressed through Code contracts in C#, but there is no way to express it through code in, say, PHP).

  • Documentation is needed to be able to use the objects (classes, interfaces) you developed. The target audience is different: it's not the persons who will maintain your code and change it that we are talking about here, but persons who barely need to use it.

    Removing documentation because the code is clear enough is insane, since documentation is here specifically to make it possible to use classes and interfaces without having to read thousands of lines of code.

  • Yes, but at least part of Martin's point is that with modern development practices, the tests are the documentation, not the code itself. Assuming the code is being tested with a BDD-style test system, e.g. specflow, the tests themselves are a directly readable description of the behaviour of the method ("given a database of products when GetById is called with the id of a valid product then the appropriate Product object is returned [...] when GetById is called with an invalid product id then null is returned" or something like this). – Jules Jun 7 '15 at 2:45

Well, it seems your colleague reads books, takes in what they say, and applies what he learned without thinking and without any consideration of context.

Your comment about what the function does should be such that you can throw away the implementation code, I read the comment of the function, and I can write the replacement for the implementation, without anything going wrong.

If the comment doesn't tell me whether an exception is thrown or whether nil is returned, I can't do that. Furthermore, if the comment doesn't tell you whether an exception is thrown or whether nil is returned, then wherever you call the function, you must make sure your code works correctly whether an exception is thrown or nil is returned.

So your colleague is totally wrong. And go ahead and read all the books, but think for yourself.

PS. I saw your line "sometimes it's part of an interface, so you don't even know what code is running. " Even just with virtual functions, you don't know what code is running. Worse, if you wrote an abstract class, there isn't even any code! So if you have an abstract class with an abstract function, the comments that you add to the abstract function are the only thing that an implementor of a concrete class has to guide them. Those comments may also be the only thing that can guide a user of the class, for example if all you have is an abstract class and a factory returning a concrete implementation, but you never see any source code of the implementation. (And of course I shouldn't be looking at the source code of one implementation).

  • I have not commented code in 10 years. Comments are bloat, garbage. Nobody comments code these days. We focus on well formed and named code, small modules, decoupling, etc. THAT makes your code readable not comments. Tests ensure that if you throw away code, that nothing goes wrong, not comments. Tests tell you how you use the code you write, how you call them, and why they are there in the first place. You're way too old school you need to be learning about testing and clean code my friend. – PositiveGuy Jun 17 '17 at 19:03

There are two types of comments to consider - those visible to people with the code and those use to generate documentation.

The type of comment that Uncle Bob is referring to is the kind that is only visible to people with the code. What he is advocating for is a form of DRY. For a person that is looking at the source code, the source code should be the documentation that they need. Even in the case where people have access to the source code, comments aren't always bad. Sometimes, algorithms are complicated or you need to capture why you're taking a non-obvious approach so that others don't end up breaking your code if they try to fix a bug or add a new feature.

The comments that you are describing are API documentation. These are things that are visible to people using your implementation, but that may not have access to your source code. Even if they do have access to your source code, they may be working on other modules and not be looking at your source code. These people would find it useful to have this documentation available in their IDE as they are writing their code.

  • I honestly never thought of DRY applying to code + comments, but it makes perfect sense. Sort of like the "increment X" example in @JacquesB's answer. – user22815 Jun 4 '15 at 15:04

The value of a comment is measured in the value of the information it gives minus the effort needed to read it and/or ignore it. So if we analyze the comment

/// <summary>
/// Retrieves a product by its id, returns null if no product was found.
/// </summary>

for value and cost, we see three things:

  1. Retrieves a product by its id repeats what the name of the function says, so it's cost without value. It should be deleted.

  2. returns null if no product was found is very valuable information. It likely reduces the times other coders will have to look at the implementation of the function. I'm quite certain that it saves more reading than the reading cost it introduces itself. It should stay.

  3. The lines

    /// <summary>
    /// </summary>
    

    carry no information whatsoever. They are pure cost to the reader of the comment. They may be justified if your documentation generator needs them but in that case you should probably think about a different documentation generator.

    This is the reason why the use of documentation generators is a disputable idea: They generally require a lot of additional comments that carry no information or repeat obvious stuff, just for the sake of a polished document.


An observation that I have not found in any of the other answers:

Even comments that are not necessary to understand/use the code can be very valuable. Here is one such example:

//XXX: The obvious way to do this would have been ...
//     However, since we need this functionality primarily for ...
//     doing this the non-obvious way of ...
//     gives us the advantage of ...

Likely a lot of text, completely unnecessary to understand/use the code. However, it explains a reason why the code looks the way it does. It will stop people short that look at the code, wonder why it's not done the obvious way, and would start refactoring the code until they realize why the code was written like this in the first place. And even if the reader is smart enough to not jump to refactoring directly, they still need to figure out why the code looks the way it does before they realize that it should best stay the way it is. This comment could literally save hours of work. Thus the value is higher than the cost.

Likewise, comments can communicate the intentions of a code, instead of just how it works. And they can paint the big picture that's usually lost in the minute detail of the code itself. As such, you are right in being in favor class comments. I value class comments the most if they explain the intention of the class, how it interacts with other classes, how it is meant to be used, etc. Alas, I'm not a big author of such comments...

  • 2
    Wow - change your document generator because it requires a couple extra lines of html for parsing? Let's not. – corsiKa Jun 5 '15 at 17:45
  • 2
    @corsiKa YMMV, but I for one would prefer a documentation generator that reduces the costs in comments to a minimum. Of course, I'd also rather read a well written header file than a doxygen documentation that's out of sync with the actual code. But, as I said, YMMV. – cmaster Jun 5 '15 at 18:06
  • 4
    Even if a method's name describes its purpose brilliantly, repeating that purpose using natural language may make it easier for someone reading it to associate that purpose with any caveats that follow. A restatement of what's described in the name will be brief enough that even if the value is low, the cost will also be low. I thus disagree with the first part of your post. +1, however, for the second part. Documentation of alternative approaches that were evaluated and rejected can be extremely valuable, but such information is seldom given the attention it deserves. – supercat Jun 5 '15 at 20:54
  • GetById raises the question, what is id, and also, get what from where. Documentation comment should allow development environment to display answer to these questions. Unless it is explained elsewhere in the module doc comments, it would also be one place to tell why one would get the id anyway. – hyde Jun 8 '15 at 5:48
  • Comments blow, clean code (self describing), TDD (getting feedback often and getting feedback on your design often), and tests (give you confidence and document behavior) rules! TESTS people TESTS. An nobody here is talking about that. wake up – PositiveGuy Jun 17 '17 at 19:04

Uncommented code is bad code. It is a widespread (if not universal) myth that code can be read in much the same way as, say, English. It must be interpreted, and for any but the most trivial code that takes time and effort. Plus, everybody has a different level of capability in both reading and writing any given language. Differences between the author and the reader's coding styles and capabilities are strong barriers to accurate interpretation. It is also a myth that you can derive the intention of the author from the implementation of the code. In my experience, it is rarely wrong to add extra comments.

Robert Martin et al. consider it a "bad smell" because it may be that the code is changed and the comments not. I say it is a good thing (in exactly the same way that a "bad smell" is added to domestic gas to alert the user to leaks). Reading comments give you a useful starting point to interpreting the actual code. If they match then you will have increased confidence in the code. If they differ then you have detected a warning smell and need to investigate further. The cure to a "bad smell" is not to remove the smell but to seal the leak.

  • 2
    Given that you are an intelligent author and the text is for the benefit of an intelligent audience; You draw the line using common sense. Obviously, as it stands, the example is dumb but that's not to say that there may be not a be a very good reason to clarify with a comment why the code includes an i++ at that point. – Vince O'Sullivan Jun 4 '15 at 13:39
  • 8
    i++; // Adjust for leap years. – Vince O'Sullivan Jun 4 '15 at 13:54
  • 5
    "Robert Martin et al. consider it a "bad smell" because it may be that the code is changed and the comments not." That's only part of the smell. The worst of the smell comes from the idea that the programmer decided not to try to write the code in a more descriptive way, and instead chose to "seal the leak" with a comment. His assertion is that rather than slap a "// adjust for leap years" comment, one probably should have a "adjustForLeapYears()" method in the code itself (or something similar). The documentation comes in the form of tests that exercise the leap year logic. – Eric King Jun 4 '15 at 15:39
  • 3
    I would consider adding a method call to replace a single line of code with a comment overkill, particularly since the name of the method is actually itself just a comment that labels a piece of code and no guarantee of better documentation accuracy. (If that line occurred in two or more places then introducing a method call would, of course, be exactly the right solution.) – Vince O'Sullivan Jun 4 '15 at 18:10
  • 1
    @Jay The reason is that making your abstractions explicit (by, e.g., introducing methods) is the rule. Not doing so because you might end up with a method that has one line is the exception. Let me paraphrase the real arbitrary rule: "Use your programming language's structures (typically methods and classes) to introduce abstractions, unless the implementation of that abstraction can be expressed as one line of code, in that case specify the abstraction in natural language by adding a comment." – Eric Jun 5 '15 at 15:18

In some languages (F# for instance) this entire comment/documentation can actually be expressed in the method signature. This is because in F#, null is generally not an allowed value unless specifically allowed.

What is common in F# (and also several other functional languages) is that instead of null you use an option type Option<T> which could either be None or Some(T). The language would then understand this and force you to (or warn you if you don't) match on both cases when you try to use it.

So for example in F# you could have a signature that looks like this

val GetProductById: int -> Option<Product>

And then this would be a function that takes one parameter (an int) and then either returns a product, or the value None.

And then you could use it like this

let product = GetProduct 42
match product with
| None -> printfn "No product found!"
| Some p -> DoThingWithProduct p

And you would get compiler warnings if you do not match both possible cases. So there's no possible way to get null reference exceptions there (unless you ignore the compiler warnings of course), and you know everything you need just by looking at the function signature.

Of course, this requires that your language is designed in this way - which many common languages such as C#, Java, C++ etc are not. So this may not be helpful to you in your current situation. But it's (hopefully) nice to know that there are languages out there that lets you express this kind of information in a statically typed way without resorting to comments etc. :)

There are some excellent answers here and I don't want to repeat what they say. But let me add a few comments. (No pun intended.)

There are lots of statements that smart people make -- about software development and many other subjects, I suppose -- that are very good advice when understood in context, but which silly people than take out of context or apply in inappropriate situations or take to ridiculous extremes.

The idea that code should be self-documenting is one such excellent idea. But in real life, there are limitations on the practicality of this idea.

One catch is that the language may not provide features to document what needs to be documented clearly and concisely. As computer languages improve, this becomes less and less of an issue. But I don't think it has completely gone away. Back in the days when I wrote in assembler, it made a lot of sense to include a comment like "total price=price of non-taxable items plus price of taxable items plus price of taxable items * tax rate". Exactly what was in a register at any given moment was not necessarily obvious. It took many steps to do a simple operation. Etc. But if you're writing in a modern language, such a comment would just be a restatement of one line of code.

I always get annoyed when I see comments like, "x=x+7; // add 7 to x". Like, wow, thanks, if I had forgotten what a plus sign means that might have been very helpful. Where I might really be confused is in knowing what "x" is or why it was required to add 7 to it at this particular time. This code might be made self-documenting by giving a more meaningful name to "x" and using a symbolic constant instead of 7. Like if you had written "total_price=total_price + MEMBERSHIP_FEE;", then a comment is probably not needed at all.

It sounds great to say that a function name should tell you exactly what a function does so that any additional comments would be redundant. I have fond memories of the time I wrote a function that checked if an item number was in our database Item table, returning true or false, and which I called, "ValidateItemNumber". Seemed like a decent name. Then someone else came along and modified that function to also create an order for the item and update the database, but never changed the name. Now the name was very misleading. It sounded like it did one small thing, when really it did much more. Later someone even took out the part about validating the item number and did that somewhere else, but still didn't change the name. So even though the function now had nothing to do with validating item numbers, it was still called ValidateItemNumber.

But in practice, it is often impossible for a function name to completely describe what the function does without the name of the function being as long as the code that makes up that function. Is the name going to tell us exactly what validations are performed on all parameters? What happens on exception conditions? Spell out every possible ambiguity? At some point the name would get so long that it just becomes confusing. I'd accept String BuildFullName(String firstname, String lastname) as a decent function signature. Even though that does not spell out whether the name is expressed "first last" or "last, first" or some other variation, what it does if one or both parts of the name are blank, if it imposes limits on the combined length and what it does if that's exceeded, and dozens of other detail questions one might ask.

protected by gnat Jun 5 '15 at 15:32

Thank you for your interest in this question. Because it has attracted low-quality or spam answers that had to be removed, posting an answer now requires 10 reputation on this site (the association bonus does not count).

Would you like to answer one of these unanswered questions instead?

Not the answer you're looking for? Browse other questions tagged or ask your own question.