#418: Comments should be treated as a lesser-evil 'code smell'.

[Reinier Zwitserloot]

On the discussion on whether comments are good or bad, I posit:

Code which is eminently understandable based solely on the name of the method, the code, and the names of variables and parameters is always better than code which requires comments to be easily understood, and also better than code which is just as understandable but which has comments in it (those comments are therefore superfluous). The reason comments are, in a vacuum, 'bad', is that they clutter up code and are impervious to unit testing. In practice, they are also notorious for ending up out of date because those familiar with the code are more likely to work on it, and are also more likely to mentally screen out the comment and thus not realize when it becomes out of date (and thus, misleading, very bad).

Of course, let's be reasonable here: While in theory any and all comments are code smells, they are pretty much always the lesser evil. Anything beyond the most academic of projects is going to have some chunk of code which does something in a somewhat weird way for a good but not immediately obvious reason, and the right choice is definitely to add a comment to explain what's happening.

However, for a great many comments, it could have been solved with helper methods. We should all aim to do that more. Helper methods make it easier to test code and are more effective at helping people new to this code understand it all than comments.

The notion that comments are the lesser evil is even more true for APIs (so, javadoc). The best theoretical API is fully understandable based only on a quick tutorial video and the names of types, methods, and parameters. In practice, there are caveats and other details that you shouldn't bother putting into the tutorial or the names, but do warrant mention. Again, lesser evil: It would be even better if the API was such a fantastic mapping onto the problem domain that there aren't any caveats in the first place, but in practice life just isn't that nice of course.

I believe treating comments as 'bad, but not a big deal' leads to the right decision in general. Comment where neccessary, put in reasonable (but don't go entirely out of your way) effort to avoid writing them by cleaning up code to alleviate the need for them, and at all costs don't comment just for the sake of adding comments.

[Cédric Beust ♔]

Imagine a world where all the standard Javadoc (e.g. java.util.collections, java.util.concurrent, etc...) were stripped out...

I don't know about others, but not a week goes by without me reading and rereading all these Javadocs. I do this all the time. Sometimes for the same class over and over again, because this class is performing complex functionalities with a lot of edge cases (the concurrent classes are a good example of that).

Occasionally, I dig into the source.

Without Javadocs, these libraries would be a lot harder to use. Not because they are badly written or badly designed but because the source only explains *what* the code does, not *why* nor *how* it does it.

The argument "Comments go bad over time and can't be enforced by the compiler" is a straw man, in my opinion: this is more about how you control the quality of the code that gets checked in that about comments themselves. Use proper code reviews and comments won't go bad.

-- 

Cédric

-- 

Cédric

--
You received this message because you are subscribed to the Google Groups "Java Posse" group.
To unsubscribe from this group and stop receiving emails from it, send an email to javaposse+...@googlegroups.com.
To post to this group, send email to java...@googlegroups.com.
Visit this group at http://groups.google.com/group/javaposse?hl=en.
For more options, visit https://groups.google.com/groups/opt_out.
 
 

[Kirk Pepperdine]

Though it pains me... I have to agree with you Cedric  ;-) Another bug-a-boo is intent.. the intent of the author sometimes doesn't come through in the low level view you get from the code. I'd guess another example.. imagine using the regular expression package without both intent and lower level information being included in the javadoc.... it would be reachable to most even if the code were 100% readable... which is rarely ever is.

-- Kirk

[Cédric Beust ♔]

On Sun, Apr 14, 2013 at 12:56 PM, Kirk Pepperdine <kirk.pe...@gmail.com> wrote:

Though it pains me... I have to agree with you Cedric  ;-)

Yeah sorry, I can't have that.

I totally reverse my position on the subject.

-- 

Cédric

[Bruce Chapman]

If what we write first is "the simplist thing that might work", then I'd suggest comments should explain code that is not apparently the "the simplist thing that might work". or "comments should explain why the simplistic thing that might have worked, didn't"


And on  regular expressions - the minimum comment should include at least one example of the "line" being matched against. It makes a huge improvement to the grokkability of a regex if you can see an example of what is being searched.


Bruce

[Cédric Beust ♔]

On Sun, Apr 14, 2013 at 11:38 PM, Bruce Chapman <brucec...@paradise.net.nz> wrote:

If what we write first is "the simplist thing that might work", then I'd suggest comments should explain code that is not apparently the "the simplist thing that might work". or "comments should explain why the simplistic thing that might have worked, didn't"

What you write first is hardly ever what you end up with.

-- 

Cédric

[Kevin Wright]

It should *always* be the simplest thing that could possibly work, my experience is that the bit in doubt is often the definition of "work" in question,  especially if that definition has evolved over time.

For example, "working" could include the need to handle malformed data, or backwards compatibility, or not fixing and old bug that downstream users came to rely on, or working around a bug in some underlying component,  or some strict performance requirements. In which cases the simplest thing might not be so simple.

So when there's room for doubt, document exactly what working means - WHY a piece of code is there, HOW it's expected to be used.

If you can't cleanly express a usage pattern through types and a DSL, then Javadocs and test fixtures are very often my favourite way to do this :)

[Kirk Pepperdine]

Sorry, I didn't mean use of regex.. I meant using the regex classes themselves

-- Kirk

[Eric Jablow]

On Sunday, April 14, 2013 2:44:58 PM UTC-4, Reinier Zwitserloot wrote:

Of course, let's be reasonable here: While in theory any and all comments are code smells, they are pretty much always the lesser evil. Anything beyond the most academic of projects is going to have some chunk of code which does something in a somewhat weird way for a good but not immediately obvious reason, and the right choice is definitely to add a comment to explain what's happening.

There are, however, algorithms that are so tuned for speed and so unlikely to be understandable by the reader that they need nontrivial comments.

Duff's_device and the Quake 3 inverse square root algorithms are the poster chldren for necessary comments.

Eric

[Josh Berry]

At face value, I do not see why programming should be any more unique than any other process description out there.  I like to bring actual cookbooks and furniture assembly examples into play.  Cookbooks are typically more prose heavy then furniture descriptions.  Even there, though, it is not uncommon to see callouts that are essentially comments.

On Mon, Apr 15, 2013 at 7:26 AM, Alexey <inlin...@yahoo.com> wrote:

It seems that a big thing that should always be discussed as part of this topic is what the nature of the comments is.  Javadoc was brought up and I dare say that kind of information is absolutely necessary and is not on par with inline comments that one only sees when looking at the source code.  API comments are directed almost squarely at the consumer of said API and themselves can be broken up into 2 categories:

1. necessary information otherwise not captured by the programming language (so many of API comments I see in Python these days start by saying something about input and output types and so many comments in general talk about possible exceptions otherwise not seen by explicit checked exception declarations)
   
2. some helpful information about general intent and intended use of the API -- could be considered technically superfluous, but sometimes oh so necessary to the human trapped inside of us struggling to get out :)

Personally, I think there's a time and place for both.  It's not possible to capture every good piece of information that's necessary at the time of authorship of the code or in some perceivable future in strict language artifacts alone.  I see and write plenty of code completely devoid of comments.  That sort of thing is usually obvious in its intent in the context of the project.  But not everything is obvious and therefore there's no shame in giving future self or your colleagues or co-conspirators a helping hand.

On Sunday, April 14, 2013 2:44:58 PM UTC-4, Reinier Zwitserloot wrote:

[Jon Kiparsky]

The distinction between API documentation and inline comments is an important one - the two have almost nothing to do with each other. I'll assume that we're all on the same page about what constitutes good API documentation: I want to know what the arguments are and in what ways they're restricted, I want to know what exceptions are thrown and under what circumstances, and I want to know enough about the implementation to judge whether this is a good fit for my purposes. (for example, if you're taking a list and repeatedly accessing elements by index, I want to know about it, so I don't give you a linked list)

That being said, I find myself standing in a middle ground on the issue of inline comments. I see no contradiction between striving for code that needs no comments at all, because it is perfectly clear from start to finish, and also insisting that comments should be included when they are actually needed. So to me, this is largely a non-issue: I know when comments are needed because I am irritated by the lack of them, and I know when they are not needed because I am irritated by their presence. If I am not irritated, the comments are correct.

[Graham Allan]

API vs Inline comments are a good line to divide across, but I suggest a different distinction: whether the audience is in your team or not. If I am writing code for someone in my team I expect them to have: the source checked out and built; tests running; whatever environment they need set up to browse through code; and some understanding of the problem domain*. For people not on my team (e.g. an open-source library or a service used by someone else in the same company), I expect them to: maybe have the source attached from e.g. Maven, but possibly not; definitely not have run the tests; only have a superficial understanding of the domain. 

I struggle to think of a term for this, other than same team = "whitebox" and not on same team = "blackbox".

In the whitebox scenario comments, even API comments, are almost an anti-pattern. Maybe I've just not been exposed to JavaDoc that pulls its weight, but it always seems like better class and method names, or better designs, or descriptive tests, prevent the need for comments.

For blackbox, API comments are necessary. I can't really imagine using something like the JDK or Guava without JavaDoc.

Fun topic.

~ Graham

* I wouldn't expect new hires to learn the domain from the source code alone.

[Fabrizio Giudici]

On Tue, 16 Apr 2013 10:10:13 +0200, Graham Allan <grundl...@gmail.com>
wrote:

> API vs Inline comments are a good line to divide across, but I suggest a
> different distinction: whether the audience is in your team or not.

The difference between withing and outside of the team can be useless in
some contexts. It's ok when there's a carefully build team, with low
turnover. But often there's not such a thing, and in this case you should
comment for your co-workers as they weren't co-workers. Also consider the
case in which a large corporate is partitioned in "islands", where you can
talk of a cohese team, but just one per island. Often they deeply differ
in knowledge of the technology and processes. Most of the produced
software stays inside the original island, but often it "leaks" outside.
Even in this case, while in the same corporate, you should comment as it
is wasn't the case.

I'm following this thread and reading things that I second, but most of
the discussion about commenting depends on details on how the team has
been built, maintained, etc... A theme that has not been dealt (unless
I've lost something) and that it's very important and hard to manage is
the mapping of business rules into code. I see corporates where business
rules are very complex (I'm not in the position of analyzing them, because
it's not my business, but often they are overcomplex), but at the same
time well understood by a restricted bunch of people who are not
developers, or anyway are not OO developers. This often get translated
into overcomplex code, especially when you need optimizations (people who
define business rules usually don't deal with optimization). Optimization
is sometimes dealt "internally" by developers, other times by means of
ad-hoc slightly changes of the business rules, with the agreement of
business people. The result is that you have a sort of hybrid territory,
in which you have artifacts that must be directly consumed by developers,
but at the same time validated by business people, unfortunately the two
groups of people speak different languages. This area is extremely hard to
document - in fact, often this stuff is "maintained" by "oral tradition".
Sure, you can do a lot to improve things: tests, as usual, are good not
only for developing but also to document business rules, and use DSL that
can be at the same time consumed by programmers and directly validated by
business people, but in the end I expect a good deal of inline comments
here.


--
Fabrizio Giudici - Java Architect @ Tidalwave s.a.s.
"We make Java work. Everywhere."
http://tidalwave.it/fabrizio/blog - fabrizio...@tidalwave.it

[Graham Allan]

I see your point. You've picked up on a bias in my experience towards cohesive, self-organising teams, who practice pair programming. I was trying to distinguish organisation from team: if I ship a library jar to be used by the developer sat six feet away in the office, if she's not on my team, there should be API documentation. You're right it's a different scenario where say you have satellite developers working halfway across the globe. In your scenario, it still sounds like comments are a smell, but it's the organisation that's stinky, not just the codebase :)

[Fabrizio Giudici]

On Tue, 16 Apr 2013 11:20:34 +0200, Graham Allan <grundl...@gmail.com>
wrote:

> You're right it's a different
> scenario where say you have satellite developers working halfway across
> the
> globe.

In my experience this just happens across the same floor :-)

In your scenario, it still sounds like comments are a smell, but
> it's the organisation that's stinky, not just the codebase :)

Sure. The point is that the whole discussion about the comments should be
given a context. I assume we're all assuming good code practices, because
otherwise it wouldn't make sense. Good code practices are not pervasive at
all, but I assume we're in a context in which we're striving for them,
which is reasonable, as with the proper effort you can get some results.
We can't assume we're always in a properly organised corporate, though.
And large, badly organised corporates aren't emendable, at least in the
short-medium term. Probably not even in the long.

[Roland Tepp]

While I generally agree that good code needs no comments, as an insider it is sometimes difficult to judge wether some piece of code or a method name satisfies the requirements of obviousness.

Particularily bad is the documentation of the domain model objects and methods. Quite often in my experience, the domain model is anemic and only acts as a dumb record holder with simple getter and setter methods.

While it seems to be obvious at first glance, what the method named getName() does, it is much less obvious what the name field means for the business. Is it merely a human readable label for the object? Is it used to identify the object in some subsystem? What is the format of the name (i.e. does it allow spaces numbers or punctuation) etc?

And that is just questions I get when I see the propert called "name" in the model. What if the property is called "total" or "susg".

[Mark Fortner]

Roland,

We have a similar problem in the bioinformatics world, where a field like "id" could mean an ID from a specific database, an accession (an alphanumeric ID similar to a database ID).  One way around this is to use semantic annotations for fields. Here's an example. http://aspenbio.wordpress.com/2011/01/20/biogroovy-and-the-semantic-web/

Cheers,

Mark

[Fabrizio Giudici]

On Tue, 16 Apr 2013 22:11:04 +0200, Mark Fortner <phid...@gmail.com>
wrote:

> Roland,
> We have a similar problem in the bioinformatics world, where a field like
> "id" could mean an ID from a specific database, an accession (an
> alphanumeric ID similar to a database ID). One way around this is to use
> semantic annotations for fields. Here's an example.
> http://aspenbio.wordpress.com/2011/01/20/biogroovy-and-the-semantic-web/

RDF is great for interoperability; for readability in general cases, it's
still good but requires that the reader knows it.

Generally speaking, annotations can be good for specifying constraints,
pre/post conditions, and improve readability in a easy way. In its small
garden, for instance, things such as @NonNull are simple, intuitive and
deliver some added value.

[Kevin Wright]

I'd have to disagree. For constraints you're much better doing it in the type system, where the compiler can check things for you.

e.g. Provide an argument of type ValidatedId instead of a String with a bunch of annotations.

I'm also curious to see if the new 'Optional' type gets much adoption vs @NotNull and friends, despite its limitations.

--
You received this message because you are subscribed to the Google Groups "Java Posse" group.

To unsubscribe from this group and stop receiving emails from it, send an email to javaposse+unsubscribe@googlegroups.com.

[Mark Fortner]

It would be nice to be able to specify the types of validations to be performed in an ontology.  It would make it easier for systems to share a common, verifiable definition for a value.  This would satisfy the DRY rule across applications, APIs, companies, etc.  But that's perhaps a little beyond the scope of the thread.

Cheers,

Mark

To unsubscribe from this group and stop receiving emails from it, send an email to javaposse+...@googlegroups.com.

[Eric Jablow]

On Tuesday, April 16, 2013 4:11:04 PM UTC-4, phidias51 wrote:

Roland,

We have a similar problem in the bioinformatics world, where a field like "id" could mean an ID from a specific database, an accession (an alphanumeric ID similar to a database ID).  One way around this is to use semantic annotations for fields. Here's an example. http://aspenbio.wordpress.com/2011/01/20/biogroovy-and-the-semantic-web/

Cheers,

Mark

So, when you have a class whose instances can be stored in a relational database with a numeric primary key, do you use a Long for its primary key instance variable, or do you use a static inner class ID that contains a Long? The first is easy to write, while the second is easy to read and ensures that no one compares Employee.ID to Department.ID.

Respectfully,

Eric

[Mark Fortner]

We use longs for ids and strings for accessions or gene symbols. The id's are usually used for joins and the accessions are used by users when doing searches.

Mark

--

[Reinier Zwitserloot]

Actually, if you look at something like ArrayList, I don't think much is lost if all the comments and javadoc just disappeared, particularly the javadoc. 'get(int index)' is all I need to know.

Edge cases is a classic case of lesser evil: Optimally speaking, the API should be so clean that there aren't any edge cases. But, if that isn't practical, then it is better to at least document these in a comment than just leaving it unsaid.

Optimally speaking, The 'what' as defined by the signature (name of types, methods, parameters), and the 'how' as defined by the raw source, should trivially answer the 'why'. In practice, the world just isn't quite that nice, and whenever it isn't, you should comment.

BUT, if by doing some sensible API design and proper creation of helper types and methods you can actually get to the point that the raw names and code provide for all reasonable questions then you should do so because that is better than comments.

If I created the impression that it is usually practical to do that, that's my bad.

[Fabrizio Giudici]

On Thu, 18 Apr 2013 19:16:38 +0200, Reinier Zwitserloot
<rein...@gmail.com> wrote:

> Actually, if you look at something like ArrayList, I don't think much is
> lost if all the comments and javadoc just disappeared, particularly the
> javadoc. 'get(int index)' is all I need to know.

It depends. If one uses sublist(), for instance, he might not precisely
recall whether upper bound is included or not, etc... I don't think get()
is a menaningful example, as most common methods are ok for everybody
who's not a rookie; javadoc is useful for those methods that are useful,
but you seldom use.

[Ricky Clarkson]

In an environment where people don't take the time to update comments, they become worthless.  In an environment where people do put energy into that, they have value.  Simples..

To help others keep comments up to date, reference the class names/method names directly in the comments so that they appear in a usages search (in IDEA anyway, not sure whether Eclipse will do that).

--
You received this message because you are subscribed to the Google Groups "Java Posse" group.

To unsubscribe from this group and stop receiving emails from it, send an email to javaposse+unsubscribe@googlegroups.com.

[Vince O'Sullivan]

On Monday, 15 April 2013 07:38:03 UTC+1, brucechapman wrote:

If what we write first is "the simplist thing that might work", then I'd suggest comments should explain code that is not apparently the "the simplist thing that might work". or "comments should explain why the simplistic thing that might have worked, didn't"

Unfortunately, code that the developer has stripped bare, in order to make it more "simple", is rarely code in a form that is most useful to someone who has to maintain it in the future (including when that someone is the same person and "the future" might only be days later).

Comments are (or, at least, ought to be) good precisely because they are redundant.  Error checking is only possible when redundancy is present.  If the comments and the code match, our confidence in it increases; if not, it decreases.  Unit tests work in the same way.  They are redundant code - often bigger and more complex than the code under test - in that they are written but not shipped (just as comments are written but not executed).  Nevertheless, no one would suggest that removing tests would be a good thing because it would make the overall code base simpler.

[Roland Tepp]

This is so far the best and the most concise comment in favor of code comments I've read so far.

Thanks man.

[Fabrizio Giudici]

On Wed, 01 May 2013 10:57:01 +0200, Roland Tepp <luo...@gmail.com> wrote:

> This is so far the best and the most concise comment in favor of code
> comments I've read so far.
>
> Thanks man.

The problem is: redundant comments, in my experience, tends to be always
out of sync with the code after some time. At that point, what's the right
one? The code or comments? :-) If you're testing at a decent level, it's
the code. Thus, what's that redundancy for?

>
> teisipäev, 30. aprill 2013 21:16.39 UTC+3 kirjutas Vince O'Sullivan:
>>
>> On Monday, 15 April 2013 07:38:03 UTC+1, brucechapman wrote:
>>
>>> If what we write first is "the simplist thing that might work", then
>>> I'd suggest comments should explain code that is not apparently the
>>> "the
>>> simplist thing that might work". or "comments should explain why the
>>> simplistic thing that might have worked, didn't"

[Roland Tepp]

Let's just say, that I've been bitten more often by missing documentation in large and complex legacy applications rather than outdated one.

If I see a discrepancy between documentation and code, I can always either fix that by updating the documentation or fix the code to follow the documentation.

More often, it turns out both are out of sync with business goals and I end up fixing both.

In any case, if there is documentation, that does not match code, it raises my flags and I go to the business analyst or architect and figure out the real requirements. If there is no documentation whatsoever, there is no way for me to know what if anything is wrong with the code (besides it being overly complex and unintuitive -- as far as I can tell, the business requirements might be overly complex and unintuitive)

Then again -- it is mostly large legacy codebases that have seen hundreds of developers before me that are poorly designed, poorly implemented and almost never commented, that I have had to deal with that make me feel that way, so maybe, if I had any real experience with well designed and well implemented code, I would probably have a different view...

2013/5/2 Fabrizio Giudici <Fabrizio...@tidalwave.it>

--
Roland

[Fabrizio Giudici]

On Thu, 02 May 2013 10:46:31 +0200, Roland Tepp <luo...@gmail.com> wrote:

> Let's just say, that I've been bitten more often by missing documentation
> in large and complex legacy applications rather than outdated one.

I don't disagree here and in the remainder of your post - but, again, I
think we're referring to different kind of comments, as it already
happened in this thread. I also often find missing documentation, but it's
not the detailed level inside the code that I was referring to, and that
often gets out of sync. I see lack of architectural and high-level
documentation, let's say at component level.

> In any case, if there is documentation, that does not match code, it
> raises
> my flags and I go to the business analyst or architect and figure out the

Correct, but if a system raises lots of red flags everywhere, red flags
become meaningless.

[Reinier Zwitserloot]

Ah, but tests are testable (the code tests the tests and the tests test the code; if your tests have a bug in it, they will fail, you will figure out it's not your code, and thus, your test). Put differently, if your test becomes 'out of date', you will notice. If a comment becomes 'out of date', it'll remain there and just confuse the heck out of any future readers.

You're basically advocating comments that are like '// add 1 to a'. Which I think we can all agree is not, in fact, a good idea.

[Roland Tepp]

pühapäev, 5. mai 2013 23:06.05 UTC+3 kirjutas Reinier Zwitserloot:

Ah, but tests are testable (the code tests the tests and the tests test the code; if your tests have a bug in it, they will fail, you will figure out it's not your code, and thus, your test). Put differently, if your test becomes 'out of date', you will notice. If a comment becomes 'out of date', it'll remain there and just confuse the heck out of any future readers.

Ah, but tests only answer to the questions "What" and "How". Comments should provide the answer to "Why".
This is specially important for domain model classes, but also to some really complex business rule implementations, where mere code and tests are not really enough...

 

You're basically advocating comments that are like '// add 1 to a'. Which I think we can all agree is not, in fact, a good idea.

BS! This is a deliberate hyperbole.  Nobody claims that you need to comment the obvious stuff... It is just the definition of "not obvious" that is unclear