Clean Code comments vs class documentation

Question

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.

Answer 1

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.

Answer 2

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.

Answer 3

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.

Answer 4

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).

Answer 5

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.

Answer 6

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...

Answer 7

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.

Answer 8

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. :)

Answer 9

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.

Answer 10

There are only two things that are useful in understanding what some code does, and they are useful because they break the build if they are wrong, and are therefore reliable.

Comments in code are not reliable, cannot be trusted, and are therefore not useful. In many cases they are misleading and therefore harmful.

In the question, the OP was suggesting adding a comment indicating that the method can return null, but what if this method is calling something that calls something that gets changed by somebody to throw an exception instead? Are they going to trace all paths through the code to find and update this comment? Absolutely not, so now the comment becomes incorrect and misleading which actually slows people down and makes them write buggy code.

The two things that are actually useful in understanding what a method does are the method signature and the unit tests. These are useful because they are reliable.

The method signature can't not represent the parameters that are passed or the type that is returned unless the programmer did some horrible type casting (please don't do this). Since the compiler performs type checking, you can rely on the method signature being accurate and a reliable source of information. This is where we come to the hardest problem in programming, naming things. The name you choose for a type will become part of the "documentation" for every method that references that type, both current use cases and future use cased that you haven't thought of yet, and this is why choosing names is considered hard to do, but very important to get right.

Looking at the method signature alone does not convey all of the information about what exactly I can pass in and exactly what I will get back in various circumstances, and there are no programming languages that are rich enough to encapsulate that (Ada came pretty close).

All of the rest of the information is in the unit tests. Unit tests contain statements like:

describe('GetById') {
  it('returns a product for valid product ids') {
  }

  it(`returns null for invalid product ids') {
  }
}

These statements tell you everything else you need to know about this method, what you can pass to it, what gets returned, and what behaviors it implements. Additionally these tests verify that these statements are true, and the build breaks if anything changes.

Take my earlier example of someone changing a deeply nested method somewhere so that it now throws an exception when the product id is invalid. This change will make the GetById unit test fail, this test failure will prevent the changes from being merged until some remedial action is taken.

The fact that the unit tests are run automatically by the CI process means that the statements they make can be relied upon, and are therefore useful. The fact that comments are not verified at all makes them unreliable and therefore useless.

My recommendation: pay close attention to naming things and write good unit tests. Never write comments to describe how the code works. The only place where comments make sense are to document a shared library or an API, but these comments are for consumers of the library or API, not for the maintainers.