Is a comment aligned with the element being commented a good practice?

Question

I am a home, amateur developer for 25 years and I just had a bright idea regarding comments. Like all such novel bright ideas, someone has probably already done it and there is probably a consensus on its brightness.

I wrote a complex (to me at least) line of code and wanted to clearly comment it so that I can still understand it in a few days:

// get all tags for the line, or empty array
                                          // all tags available
                                                                    // filtered for tags that are set
                                                                                                 // do they incude the current tag of the line?
                                 // add the result of include to the container
(note.tags || []).forEach(tag => inc.push(Object.keys(this.allTags).filter(x => this.allTags[x]).includes(tag)))

Each comment is vertically aligned with the piece of the line it refers to. Is this something acceptable?

The obvious pro is that the comments actually relate to the piece that is being commented.

The cons include apocalyptic line reformatting (losing the match between the indentation and the piece being commented) and probably the surprise to the person reading the code if this is not typical.

Answer 1

No, such aligned comments are not a good practice. It is not clear that the comments relate to specific positions on the line. It just looks like very wildly formatted code.

The comment's indents will also be removed by an auto-formatter. Therefore, if you want to make comments about a specific part of the line, I'd put the spacing/offset within the comment:

(note.tags || []).forEach(tag => inc.push(Object.keys(this.allTags).filter(x => this.allTags[x]).includes(tag)))
//^^^^^^^^^^^^^^^                ^^^^^^^^ ^^^^^^^^^^^^^^^^^^^^^^^^^ ^^^^^^                       ^^^^^^^^^^^^^
// |                              |        |                         |                           do they incude the current tag of the line?
// |                              |        |                        filtered for tags that are set
// |                              |       all tags available
// |                             add the result of include to the container
// get all tags for the line, or empty array

However, this is still fairly unreadable because it's a very long line. It is also difficult to maintain, since tiny changes to the line would cause the ASCII art to get out of sync.

I sometimes use this strategy for very tricky code, in particular for documenting complex regular expressions.

A better solution is to split this expression over multiple lines, and to potentially use intermediate variables to make the code more self-documenting. The variable names can describe the intent of the value they're holding:

const allAvailableTags = Object.keys(this.allTags);
const tagsThatAreSet = allAvailableTags.filter(x => this.allTags[x]);
const tagsOnTheLine = note.tags || [];
// Check if the line tags are set.
// Add the result (true/false) to the `inc` container.
tagsOnTheLine.forEach(tag => {
  inc.push(tagsThatAreSet.includes(tag));
});

Note that extracting the constant expressions also happens to avoid the nested forEach/filter loop.

WoJ's answer also suggests splitting the expression over multiple lines, but without introducing extra variables.

But personally, I'd write it like this:

// Check if this line's tags are set.
for (const tag of (note.tags || [])) {
  inc.push(!!this.allTags[tag]);
}

This uses the !! boolification pseudo-operator that converts a value to true/false. Equivalently, you could use the Boolean(...) function. Your allTag object also allows easy checks to see whether a tag is set, without having to filter the keyset first. Seeing such connections can be easier when the code is well-formatted.

And as a general point, you might re-consider what is worth commenting. Every language has its libraries and idioms (small patterns). It is usually wasted effort to comment things that are very normal in the language, for example the object || default idiom, what the .filter() method does, or how !! works. Therefore:

• Avoid writing very clever code – often a for-loop is clearer than functions like filter/map/reduce.
• Focus comments on the intent, not on the implementation. The why, not the how, because implementation details are easy to look up later.
• Bookmark a JavaScript reference like MDN to quickly look up language details you're not sure about.
• Use an editor/IDE such as VS Code that can show a documentation popup when hovering over a function/variable.

Answer 2

While writing the question, I realized that I can break the lines:

(note.tags || []) // get all tags for the line, or empty array
  .forEach(
    tag => inc.push(  // add the result of include to the container
      Object.keys(this.allTags) // all tags available
        .filter(x => this.allTags[x]) // filtered for tags that are set ...
        .includes(tag)  // ... do they incude the current tag of the line?
    )
  )

This looks much better.

I will still keep the question in case such a visual breakup of the line would be syntactically forbidden, or not readable.

Answer 3

This is an XY problem. There should never be a need to even consider multi-line aligned comments because (a) your code should be self-documenting as much as possible in which case comments are unnecessary, and (b) a line of code which is so complex that it needs 4 comments should be broken up into separate lines, each with their own comment (if needed).

You should almost always prefer readability over packing as much logic as possible into a single line. There may be some rare exceptions (e.g. where performance is a genuine issue), but you'll know those because they will have (hopefully) been identified with a profiler.

Answer 4

I fully agree with your and amon's answer. Then, I would like to offer you an answer with a frame challenge :-)

Have a look at this:

var allTagsAvailable = this.allTags;
var allTagsForLine = note.tags || [];
var inclusionContainer = [];

allTagsForLine.foreach( 
    currentTagOfLine => inclusionContainer.push(
       Object.keys(allTagsAvailable)
          .filter(x => allTagsAvailable[x])
          .includes(currentTagOfLine) 
    )
 ) 

This answer is still about comments, in that they have been completely removed.

Often, one can replace a cryptical name or fragment explaned by a comment by an explaining name or fragment. So the comment becomes unneccessary, and the eyes get the relevant information at the first glance.

Here, you say it yourself:

// ... do they incude the _current tag_  _of_ the _line_?  

So therefore let's choose the explaning name

currentTagOfLine

From here, it is a question of personal gusto, workplace requirements, team agreements, already existing code base, the technical topic etc. Well, you know. Some people would say that my variable names above are too verbosive and explaining too much, and would prefer single letters as variable names.

Answer 5

Beyond the answers already here that make good suggestions about how your code could be formatted to read more easily, I think that here it would be worth considering what the point of a comment should be in your codebase. What you have here is almost a breakdown of exactly what your code is doing:

// get all tags for the line, or empty array
                                          // all tags available
                                                                    // filtered for tags that are set
                                                                                                 // do they incude the current tag of the line?
                                 // add the result of include to the container
(note.tags || []).forEach(tag => inc.push(Object.keys(this.allTags).filter(x => this.allTags[x]).includes(tag)))

I think you need to assume that an incoming developer will be able to decipher the code, and what may be better here would be a comment that signifies the intent of what you were doing, e.g.

//Populate the set of available tags that include the tag for this line
(note.tags || []).forEach(tag => inc.push(Object.keys(this.allTags).filter(x => this.allTags[x]).includes(tag)))

This has the benefit of being easier to read, and is also more likely to stay useful if the code underneath is refactored.

---

One further benefit that I have found in the past is that code complicated enough to have a comment is also more likely to be buggy. A comment that indicates what the developer was trying to do (as opposed to exactly what they thought they did) can be exceedingly useful.

Answer 6

A very good idea is to write a self-explanatory code that doesn't need a comment.

In most use cases comments are a code smell of bad implementation. In my experience, people don't tend to maintain comments along with a code.

How about introducing a couple of abstractions in a form of intermediate variables and methods with names clearly explaining the intention of a control flow?

Assuming this.allTags dataset structure is of Record<string, boolean> type:

{
  cleancode: true,
  refactoring: true,
  oneliner: false,
}

We could structure the code as follows:

hasTag(tags, target) {
  return tags.includes(target);
}

isTagSet(tag) {
  return this.allTags[tag];
}

getSetTags() {
  return Object.keys(this.allTags).filter(isTagSet);
}

getTagResolution(tag) {
  return hasTag(getSetTags(), tag);
}



const { tags = [] } = note;
const resolutions = tags.map(getTagResolution);

inc.push(...resolutions);

This code clearly explains itself, follows the Single Responsibility Principle. Plus, it's super easy to test such a code as @davidbak comments.

If additional methods creation feels like polluting the existing class/object, it's worth abstracting this particular operation to extract tag resolutions into a small, separate class.

Clean code is always simple.