48

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.

9
  • 17
    If you want my take on breaking up dense one liners you'll find it here. Mar 13 at 13:41
  • 9
    Don't you mean to say each comment is horizontally aligned with the piece of the line it refers to? Mar 13 at 19:59
  • 7
    If you were to tell another developer what that line does, would you say "get all tags for the line, or empty array, add the result of include to the container, all tags available, filtered for tags that are set, do they incude the current tag of the line" (possibly with some extra words in the middle to take it from disjoint comments to a coherent sentence) or would you say something more like "add all tags that include any of note's tags to inc" (or whatever the code actually does)? The latter would make for a much better comment. Mar 13 at 23:54
  • 7
    Completely independently of how they should be aligned in code, the contents of those comments are pretty trivial. I suggest you actually learn the language well enough that you don’t have to leave comments to the effect of ‘i++ /* increment i */ Mar 14 at 7:44
  • 4
    Readability is very important. Whenever you feel tempted to heavily comment a single line of code, that means the code itself isn't readable. Instead of adding comments, re-write the code in such a way that you don't need comments. Mar 15 at 11:00
155

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.
8
  • 8
    Assuming the horribly-named inc doesn't include other things (which, looking at the code, it most likely doesn't), this can be simplified even further to const inc = (note.tags || []).map(tag => !!this.allTags[tag]); Mar 13 at 19:27
  • 24
    This is a great answer +1; I just wanted to challenge the generality of this point: "often a for-loop is clearer than functions like filter/map/reduce" - this depends on (1) how familiar the team is with those concepts, and on (2) wheather they are being used in a sensible way or if you're doing some sort of "code acrobatics". But I do agree that the takeaway for the OP should be not to blindly default to these (or, on the other end of the spectrum, go out of their way to avoid them). Mar 13 at 20:31
  • 4
    Seconding @FilipMilovanović, in some cases filter/map/fold/bind/etc. really are the clearest. Right tool for the job—in particular, I would consider a for loop here because it’s a statement with mutation. or I would write a functional style map (tagsOnTheLine.map(tagsThatAreSet.includes)) which I could append to inc or return the list concatenation or what-have-you. It’s matter of focusing the mutation in a small way to make it more manageable. Mar 13 at 21:37
  • 4
    "I sometimes use this [...] for documenting complex regular expressions." -- BTW, you probably already know, but since you mention it like that. At least some commonly used regex-interfaces have the x modifier pretty much just for that, e.g.: docs.python.org/3/library/re.html#re.X Lacking it, I would probably be tempted to emulate it by hand to help with the commenting...
    – ilkkachu
    Mar 13 at 21:55
  • 1
    @BlueRaja-DannyPflughoeft if inc contains other previous things (however unlikely that is), inc.push(...(note.tags || []).map(tag => !!this.allTags[tag])) would work as well
    – njzk2
    Mar 13 at 23:25
72

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.

21
  • 12
    Any reasonable programming language in which long lines can occur has a way for allowing line breaks.
    – Doc Brown
    Mar 13 at 8:55
  • 17
    @DocBrown Conversely, a language that doesn't allow you to do this can be considered unreasonable, and you should try to avoid using it.
    – Barmar
    Mar 13 at 16:18
  • 7
    ... if you split that one-liner into separate functions each with one responsibility, and each with a proper name, you'll be able to test each one separately, and much easier (test conditions will add, not multiply). And you'll have places to put your comments left-justified in the normal fashion. And, guess what: if you give each of the component functions proper names you might discover you don't need any comments at all to tell the reader what's going on ...
    – davidbak
    Mar 13 at 22:05
  • 11
    I'd say this code you've just posted isn't readable. Its too hard to figure out what's code and what's comment, a glance at it is overwhelming. The top answer of using named variables to reduce the complexity of this line is far better. Or just a single comment above the block explaining what it does. If your code is so unclear that each step of a line needs an explanation, you either need helper functions or you're doing something overly complex. This would be junior level overcommenting. Mar 14 at 1:42
  • 7
    @gabesechan If your editor isn't making what is code and comment clear with colour, it's time to get a better editor. Mar 14 at 8:40
16

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.

13

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.

3
  • 4
    Very good answer. Well chosen naming >> comments. We might quibble about exact naming - I'd consider the "OfLine" in "currentTagOfLine" too verbosive, for example - but I'm confident you'd understand the code if I wrote it in this fashion and vice versa. Mar 14 at 12:45
  • 1
    Thank you @JackAidley. Yes, this is exactly the point, we could read and comprehend our code quickly. - I think I've mentioned the context Line three times, which is too much, you're right. The context Line is set with the foreach, that's enough. Mar 15 at 5:27
  • isTagAvailable = x => this.allTags[x] -> Object.keys(this.allTags).filter(isTagAvailable).includes(currentTagOfLine) seems better to me - extract the operation, rather than just rename a variable. Also probably even easier with less chained operations isTagAvailable = tag => x => this.allTags[x] && x === tag -> Object.keys(this.allTags).some(isTagAvailable(currentTagOfLine)). Now reading the chained .keys() calls explains better what's happening than trying to figure out what the anonymous function is supposed to do.
    – VLAZ
    Mar 15 at 9:53
10

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.

2
  • 2
    Very good. This is in fact often helpful for me: a comment line tells the idea of the next line or group of lines. From here I see the refactoring possibility to create a method with a speaking name instead of a comment, like this: populateTagsIncluding(tag) Mar 16 at 7:45
  • 1
    And by using a method, you could put extra comment in the method that won't clutter code at the call site. In any cases, explaining each line is a bad practice as 90% of the time that code is fixed, comments are not updated. Use proper varaible and function name and you almost not need much comments.
    – Phil1970
    Mar 25 at 23:05
2

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.

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