DEV Community

loading...
Cover image for Comments Are The Only "Code Smell"

Comments Are The Only "Code Smell"

Adam Nathaniel Davis on October 08, 2020

My regular readers (both of them) may notice that my previous article basically disavowed the idea of the "code smell". The term is too often a sm...
pic
Editor guide
Collapse
valeriavg profile image
Valeria

I disagree. Comment itself is not a "code smell", its misuse is.

There are situations where leaving a comment is a necessity.
Comments are not meant to describe things, but to clarify certain statements and leave notes, regarding use of certain constructions and tools.

For example :

// typeof null is 'object'
function getType(value){
  if(value === null) return 'null';
  return typeof value;
}

// Limit degrees to [0,360), 360 degrees is 0
const degrees = userInput & 360;

// Due to a bug in 3rd party module we need to perform this check
// [link]
const result = thirdPartyModule.exec(validateValue(value));
Collapse
merri profile image
Vesa Piittinen

I disagree with the above samples. I hope I don't go too much into detail :)

On first one I'd remove the comment and write a test which would ensure correct output. If later on somebody gets an idea to remove the value === null because it is "not needed" the test will break and they'll see why.

Second one should be userInput % 360. To me the comment tells what the code does (modulo) so there is no need to reiterate it, and I would remove the comment upon seeing it. It also lies because the code doesn't do anything to check against negative values, so the comment could give a false feeling of correctness. Tests would ensure correctness.

For the third one I'd add emphasis that it should tell exactly which 3rd party module is in question, and also suggest a short comment on why the bug has to be worked around the way it is (especially if the code for it isn't obvious). Reason: I've seen people use these "due to bug in 3rd party..." comments as-is and they're not useful. Of course the link is the meat, but because the Internet cannot guarantee links to work forever I'd always also add the minimum necessary required for understanding into the comment.

Collapse
valeriavg profile image
Valeria

I agree, tests will prevent things from being removed without notice, but having one line right by the function would (hopefully) prevent the "refactoring" altogether.

Good point on modulus, bad example.
It was implied that the comment would state the name of the package or in the worst case link to GitHub issue will.

Bottom line, If you're not sure if you should leave the comment - you should.
It's a text, it's a core feature of every single language and it's meant to be used, not feared.

Thread Thread
merri profile image
Vesa Piittinen • Edited

Maybe I've had bad experiences or bad luck, but I've noticed comments to be rather ineffective against "refactoring". You need only one person with too strong an opinion and less experience who successfully ignores what the comment warns or informs about, and goes ahead changing and breaking things.

Code reviews don't perfectly protect against this as those may get through by another who doesn't stop to care about the comment, possibly because it doesn't appear in the diff. Only tests seem to successfully protect against this, at least against breaking stuff. You'd have to go for slightly malicious mindset to start removing or changing valid tests. These "refactors" are often done thinking it is an improvement, with no malicious intent.

This is one reason why I keep comments as the absolute last tool to convey understanding that the code or naming things can't provide.

Collapse
bytebodger profile image
Adam Nathaniel Davis Author

I'm pretty sure that this is exactly what I described in the article.

Collapse
valeriavg profile image
Valeria • Edited

Please forgive me if I misinterpreted your article.
It seemed to me, that your article main idea was "don't use comments unless you're absolutely sure", opposite to "use comments code wisely".

Collapse
darkwiiplayer profile image
DarkWiiPlayer

OMG! Your variables are so longggg! Everything's so verbose! There's no way that I want to code like this!

I've always found that way of thinking very childish. Code is read way more often than typed, so choosing variable names that are shorter to type is incredibly short-sighted.

It's not that I don't often find myself thinking that. But I often find myself thinking "damn I want a siesta right now" yet I don't just start sleeping in the office.

Comments should never (unless you're hacking bits in an OS kernel or something) explain the code. They should explain design choices:

  • Domain-specific decisions
  • Complicated optimization choices
  • Test and benchmark results that influenced the design
  • References to resources explaining rarely used abstractions
  • Bugs in external software that need to be considered

So overall I'd say I pretty much agree with the article, though I find the title somewhat clickbaity. There's so many valid uses for comments that calling them a "code smell" is really stretching the expression a lot.

Collapse
v6 profile image
🦄N B🛡

Code is read way more often than typed

This has so many underrated implications.

Collapse
bytebodger profile image
Adam Nathaniel Davis Author

So overall I'd say I pretty much agree with the article, though I find the title somewhat clickbaity. There's so many valid uses for comments that calling them a "code smell" is really stretching the expression a lot.

Guilty as charged! But I made a point to illustrate exactly where comments are useful (critical, even). And my examples pretty much line up perfectly with yours.

And with regard to those who complain about variable name lengths, I also find their arguments to frequently be arbitrary. In other words, they complain about the length of the variables you've chosen, but when they decide to use a longer naming convention, it's totally "OK".

Collapse
merri profile image
Vesa Piittinen • Edited

Descriptive, long variable names can be good. However you should still be cautious with length, repetition and context.

If the file already defines context you don't need to repeat that context in each name especially if those variables are not exported.

If you have multiple variables that have repetition of the same things you may want to consider changing the names for visual clarity to make emphasis on how they are different. The more similarity there are between two variable names, and the longer they are, the harder it becomes for human mind to keep track of them.

const checkoutPageCurrentTotalInShoppingCart = getTotalFromShoppingCart();
const checkoutPageCurrentTotalWithTax = calculateSalesTax(checkoutPageCurrentTotalInShoppingCart);

I've seen people do things like this.

I would say currentTotalInShoppingCart is roughly the maximum length you should usually have for a variable name. There are OK exceptions. With this particular case I'd probably remove the word current for quicker distinction of differences, but even that decision depends on other variables in the file.

There are no absolute correct answers, but you can do your best to make the code reading process effortless.

Collapse
bytebodger profile image
Adam Nathaniel Davis Author

Agreed on all fronts. As I stated in my article, I strongly prefer full-word-name variables. What I didn't explain in the article is that this often leads me to think - deeply - about the "exact", most-concise way to name a variable such that it is 1) clear, 2) descriptive, and 3) as short as possible.

It's not uncommon for me to refactor code because, when I was first writing it, I couldn't think of anything clearer than longYellowFruit. But later, while I'm reading the code, I realize that it's soooo much simpler to just call the variable banana.

I also share your frustration with context that is needlessly stuffed into the variable names. One great example of this annoyance is in objects. I see stuff like this far too often:

const names = {
  firstName: 'Adam',
  middleName: 'Nathaniel',
  lastName: 'Davis',
}

Despite my love of descriptive names, code like this drives me nuts. There's usually no reason for the repetition of Name inside each of the keys.

Collapse
merri profile image
Vesa Piittinen

Yup, and my comment wasn't really to question anything in your article, I think it is very much on point. Went more for providing more food for thought for the others that end up here and might find this topic for the first time :)

Collapse
sramkrishna profile image
Sriram Ramkrishna

I tend to over comment my code, mostly because I'll look at it and then 6 months later I will loo at it again. It always ends up that I can't remember why I did anything in the code so the comments are to remind me. This was all internal code at my employer and mostly tools not apps. In the end, I suppose you could instead write a doc, and then add issues instead of adding "FIXME:" type of comments.
I still enjoy adding lots of comments, but most of the time my code tends to be self documenting.

Collapse
bytebodger profile image
Adam Nathaniel Davis Author

I definitely don't like the idea of external documentation - cuz in a quarter century of doing this, I have never seen the external documentation kept up-to-date with the live code.

Also, I don't hate the TODO / FIXME type comments. I personally think they can be useful when you're in a pre-launch state and the code is changing rapidly and you want to remind yourself that you need to do a little more work at this point in the code. But once the app is launched, I've rarely ever seen those comments acted upon. They become just as stale (and as meaningful) as a bit of "Kilroy Was Here" graffiti.

Collapse
sramkrishna profile image
Sriram Ramkrishna

Oh I agree. But I've been really happy with past self for writing for for future forgetful self. In these cases I'm the only person likely to see the code so I guess it works for this particular scenario.

For FOSS projects, I try to write self documenting code, but still try to put some comments in. I'm open to knowing alternative ways to give myself hints on what I was thinking without writing a design document! :-)

Collapse
zoedreams profile image
☮️✝️☪️🕉☸️✡️☯️ • Edited

nice write up. I totally agree with everything you said even if you use // for comments ;) jk. This stuff is my least fav part of this field. Mostly because its the lack of effort and quality people put into code. In fact most of that code above would not even make it past my checkstyle and lint checks. Im sure the following code isn't the best, but its an example of using proper encapsulation, naming, and proper technical writting with imperative voice. have you ever read the source code to react. ya, dont write comments like that.

here is kinda my style of bold node code using a IoC style with dep injection.. smells like teen guice. ;) --

const express = require("express")(),
  server = require('http').Server(express),
  bodyParser = require("body-parser"),
  ResourceAssembler = require("./resources/ResourceAssembler"),
  TalkToClient = require("./resources/TalkToClient"),
  TalkToRoom = require("./resources/TalkToRoom"),
  JoinRoom = require("./resources/JoinRoom"),
  LeaveRoom = require("./resources/LeaveRoom"),
  Util = require("./Util"),
  io = require('socket.io')(server, {
    serveClient: false,
    allowUpgrades: true,
    pingInterval: 20000,
    pingTimeout: 20000,
    transports: ["polling", "websocket"],
    cookie: false
  });

/**
 * The core class that defines who and what the Talk server is
 */
class Talk {

  /**
   * builds the Talk service into a server. stores the components into the service
   */
  constructor() {
    this.express = express;
    this.server = server;
    this.io = io;
    this.connections = new Map();
    this.port = process.env.PORT || 5050;
    express.use(bodyParser.json());
    express.use(bodyParser.urlencoded({extended: true}));
  }

  /**
   * called by the server class to build the service for use
   * @returns {Talk} - the thing we built
   */
  setup() {
    Util.log(null, "Starting Server...")
    this.wireApiToResources();
    this.configureSockets();
    return this;
  }

  /**
   * this function uses our resource assembler to generate the classes and
   * service URLs used for express to handle our POST requests
   * @returns {Talk}
   */
  wireApiToResources() {
    Util.log(this, "Wiring resources together")
    ResourceAssembler.inject(TalkToClient);
    ResourceAssembler.inject(TalkToRoom);
    ResourceAssembler.inject(JoinRoom);
    ResourceAssembler.inject(LeaveRoom);
    return this;
  }

  /**
   * this function is used to configure out socket listeners for socket.io, and
   * these are global listeners. try not to add stuff into here is possible
   * @returns {Talk} - the talk service object for chaining
   */
  configureSockets() {
    Util.log(this, "Configuring io sockets");

    this.io.on("connection", (socket) => {
      let connectionId = Util.getConnectionIdFromSocket(socket);
      let isNewConnection = Util.isNewConnection(connectionId);

      Util.log(this, "connection : " + connectionId + " -> " + socket.id + " = " +
        (isNewConnection ? "fresh transport" : "recycled transport"));
      Util.setConnectedSocket(connectionId, socket.id);

      // TODO implement Util.reportConnection(connectionId);

      // TODO make this comnditional for when we get a bad report back

      socket.on('error', (error) => {
        Util.log(this, "error : " + socket.id + " -> " + error);
      });
      socket.on("disconnect", (reason) => {
        Util.log(this, "disconnect : " + connectionId + " -> " + socket.id + " = " + reason);
      });
    });
    return this;
  }

  /**
   * starts the server on the port specified by the cli argument
   * @returns {Talk}
   */
  begin() {
    server.listen(this.port, () => {
      Util.log(this, `Started on ${this.port}`)
    });
    return this;
  }
}

module.exports = Talk;
const Api = require("../Api");

class ResourceAssembler {

  constructor() {
  }

  static inject(clazz) {
    if(!clazz.hasOwnProperty('resource')) {
      throw new Error("All resources of type 'BaseResource' require the static function 'resource'");
    }
    global.talk.express.post(Api.URI[clazz.name], (..._) => clazz.resource(..._));
  }
}

module.exports = ResourceAssembler;

that is part of a socket io p2p rest server. i wrote for real time messaging. the rest of the code is here github.com/ZoeDreams/talk

thank you for sharing :)

Collapse
bytebodger profile image
Adam Nathaniel Davis Author

Thanks for taking the time to offer your examples!

Collapse
zoedreams profile image
☮️✝️☪️🕉☸️✡️☯️

awh no problem. You have good taste my friend. I will always take the extra time to help people write easier to read code. Laziness breeds efficiency. If you take note of one thing in my code, you will see how i kinda made the setup chain an ad-hoc promise by returning the entire class. That is a super nice trick i do which is really fast in react land to access all sorts of stuff.

Collapse
pjotre86 profile image
pjotre86 • Edited

Hard to disagree. Uncle Bob proclaimed this more than 10 years ago already 🎅

However there's one use case for comments I'd like to add:
When you have to deal/integrate with/extend legacy code and/or 3rd party libraries.
This often puts you in a spot where you can't write code which is easy to read and understand anymore no matter how hard you try to encapsulate it. Comments as a last resort so to say...

Collapse
bytebodger profile image
Collapse
justsharkie profile image
/*Sharkie*/

Back in college, I had one prof who told us to comment EVERYTHING, and would grade us on our comments.

This was also the prof who wanted us to name our variables/functions "x", "foo" or "bar".

I caused him a lot of grief when I said we needed more explanatory names and less comments. I wish I knew his contact info so I could send this too him. 😂

Collapse
frankszendzielarz profile image
Frank Szendzielarz • Edited

I get the point of this and I agree largely. Comments are tools and can be used or abused.

Some things need comments. For example, in C# some absolutely amazing code can be written in (e.g.) LINQ or Rx.NET, and without the comments you might as well be looking at hieroglyphics for a while.

Come to think of it though, LINQ, Rx.NET and perhaps SQL are the only examples of where I think comments are often essential I can think of. I don't even want to think about regular expressions - I prefer to pretend those things don't even exist.

Collapse
bytebodger profile image
Adam Nathaniel Davis Author

I'm not familiar with Rx.NET. But as a C# dev myself (although it's been some years since I was "in the thick of it", this is exactly why I do not like LINQ. I've seen numerous C# devs write some absolute chicken scratch in LINQ, and then swear to anyone who will listen that it's "beautiful". You don't have to agree with me, but I stand by my original contention: If I need comments to understand your (LINQ) code, then it's some crappy code.

As for SQL, no, you absolutely do NOT need comments to understand a good SQL command. Some years ago, I was working with a guy who'd never seen the way that I wrote SQL statements (which was quite different from the way that he - or anyone else - wrote them). He took one look at it, processed it for a few seconds, nodded his head, and said, "Yeah... I get this." And from there forward, he always wrote his SQL statements in the exact same format that I do.

Collapse
webketje profile image
webketje • Edited

Read the article with a frown ready to point out the usefulness of comments to explain the why, then got to that section and thought: well done sir :D

(although there are other nasty code smells that are guaranteed to cause trouble; a common example I've come across is functions returning an empty array/ object synchronously that gets populated async after the return with XHR call results, relying on pass-by-reference, when the proper way would be to return a promise. This kind of shit breaks React re-rendering as the prop doesn't change)

Collapse
jwp profile image
John Peters

I believe this is a common sentiment of lazy programming styles and in general, the common practice of most Javascript only programmers.

Code comments are the best way to expose an API, here's a classic example (shown below). As can be seen, this is an API for the package Restangular. Not a single comment which means, the programmer has to look up each method, read and understand what it does, rather than read what it does via the API.

A colossal waste of time which breeds false job security for those that "learned it" and put into a production system. Thinking my job is secure "just because only (we) know it" is a 1980's thing. Anyone using that model today should be immediately fired.

Today, it's about speed to delivery, any APIs that are poorly documented or have no API comments should be rejected in totality, and removed from current code.

So the real question is how do we use this piece of trash which has 'extraordinario mal olor'?

export declare class Restangular {
    configObj: any;
    private injector;
    private http;
    provider: {
        setBaseUrl: any;
        setDefaultHeaders: any;
        configuration: any;
        setSelfLinkAbsoluteUrl: any;
        setExtraFields: any;
        setDefaultHttpFields: any;
        setPlainByDefault: any;
        setEncodeIds: any;
        setDefaultRequestParams: any;
        requestParams: any;
        defaultHeaders: any;
        setDefaultResponseMethod: any;
        defaultResponseMethod: any;
        setMethodOverriders: any;
        setJsonp: any;
        setUrlCreator: any;
        setRestangularFields: any;
        setUseCannonicalId: any;
        addResponseInterceptor: any;
        addErrorInterceptor: any;
        setResponseInterceptor: any;
        setResponseExtractor: any;
        setErrorInterceptor: any;
        addRequestInterceptor: any;
        setRequestInterceptor: any;
        setFullRequestInterceptor: any;
        addFullRequestInterceptor: any;
        setOnBeforeElemRestangularized: any;
        setRestangularizePromiseInterceptor: any;
        setOnElemRestangularized: any;
        setParentless: any;
        setRequestSuffix: any;
        addElementTransformer: any;
        extendCollection: any;
        extendModel: any;
        setTransformOnlyServerElements: any;
        setFullResponse: any;
        $get: any;
    };
    addElementTransformer: any;
    extendCollection: any;
    extendModel: any;
    copy: any;
    configuration: any;
    service: any;
    id: any;
    route: any;
    parentResource: any;
    restangularCollection: any;
    cannonicalId: any;
    etag: any;
    selfLink: any;
    get: any;
    getList: any;
    put: any;
    post: any;
    remove: any;
    head: any;
    trace: any;
    options: any;
    patch: any;
    getRestangularUrl: any;
    getRequestedUrl: any;
    putElement: any;
    addRestangularMethod: any;
    getParentList: any;
    clone: any;
    ids: any;
    httpConfig: any;
    reqParams: any;
    one: any;
    all: any;
    several: any;
    oneUrl: any;
    allUrl: any;
    customPUT: any;
    customPATCH: any;
    customPOST: any;
    customDELETE: any;
    customGET: any;
    customGETLIST: any;
    customOperation: any;
    doPUT: any;
    doPATCH: any;
    doPOST: any;
    doDELETE: any;
    doGET: any;
    doGETLIST: any;
    fromServer: any;
    withConfig: any;
    withHttpConfig: any;
    singleOne: any;
    plain: any;
    save: any;
    restangularized: any;
    restangularizeElement: any;
    restangularizeCollection: any;
    constructor(configObj: any, injector: Injector, http: RestangularHttp);
    setDefaultConfig(): void;
    static ɵfac: ɵngcc0.ɵɵFactoryDef<Restangular, [{ optional: true; }, null, null]>;
    static ɵprov: ɵngcc0.ɵɵInjectableDef<Restangular>;
}
Collapse
ggenya132 profile image
Eugene Vedensky

I strongly agree with the 'Why' sentiment. Sometimes apps change and our models or modules were not adequately abstracted to gracefully internalize the change. That's when comments are useful like you mention. Tell me why I'm seeing an anti-pattern in the code base not what it does.

Collapse
manchicken profile image
Michael D. Stemle, Jr.

I don't normally agree with articles like this, as I feel so much of what we do in this particular human endeavor is subjective.

I completely agree, though. I'd go one step further and say that we should document our intent. I've gone into far too much code a few years after it was written and wondered why things were done the way they were. It's risky to have code like that, you never know what is intentional, what is a bug, and what is a common-law feature.

Collapse
vaclavhodek profile image
vaclavhodek

Great post. I completely agree that comments should not say WHAT the code does (such comments just duplicate the code), but WHY the code is here or WHY is it written that way, HOW it is supposed to work or WHEN it is supposed to be used.

You described WHAT and WHY.

HOW - For complex problems, briefly describe the logic behind. Not the actual two or three lines, but overall concept. Imagine something more than just a = b * 1.07, where several other methods are called, etc. With proper comment, you may not need to study it deeply. Also, HOW helps with writting tests and debugging.

WHEN - Describe appropriate use case. When it should and when it shouldn't be used. Let's say that you have method that is consuming a lot of time (which may be correct behavior) and it shouldn't be called in UI thread. Of course, only add such comment when there is a possibility of misusing the method.

Collapse
bytebodger profile image
Adam Nathaniel Davis Author

Good points. I could quibble that "how" can often be accomplished by writing cleaner code. But that's not always the case.

I especially agree with "when". Sometimes you open a code file and there's a function/method there that doesn't seem to be called from anywhere within that file, and you literally can't imagine when it would ever need to be called. In those times, it's tempting to consider "cleaning" it up - by removing it. But it can be extremely helpful to have a comment there that explains, "this only gets called by a cronjob that's launched nightly from the client's environment".

Collapse
ecyrbe profile image
ecyrbe • Edited

Hi Adam,

Again, nicely written article.

I would add that using long names to explain your code should be the norm. But good naming is an art.

The team should refuse to validate functional code if there are some naming issues.
Code is not just the instructions you give to the machine. It's also the way you inform the future maintainers of your code what it does. It's your communication tool to the future YOU that will read this code two years from now.

The issue with comments is that they become obsolete when the code evolves.
And the only thing that evolves with your code is your code...so everyone should keep in mind that code that works and have no bugs is not sufficient.

You should strive to produce self documented code. And most of the time self documenting code, means long names that convey meaningfull information.

Collapse
bytebodger profile image
Adam Nathaniel Davis Author

I agree with all of this. And in my article, I completely forgot to point out the tendency of comments to become stale and unrepresentative of the very code in which they're embedded. I particularly enjoy this statement:

And the only thing that evolves with your code is your code

Collapse
joehonton profile image
Joe Honton

Yup, another third-rail topic that's sure to electrocute anyone who touches it.

Here's the counterpoint to your post: You should write a comment on every line of code.

Of course, in the end the only person who will ever read your comments is you. So write them for yourself. Or don't.

Collapse
jfbrennan profile image
Jordan Brennan

I was about to stop reading and immediately blast you when I saw that salesTax.florida line, but I kept reading and you "totally redeemed yourself!" Haha

Good read, thanks!

Collapse
v6 profile image
🦄N B🛡

Here's the only truly defensive code:

github.com/v6/nothing

And I'm proud to say, it requires no comments.

Collapse
cwraytech profile image
Christopher Wray

Very great article! Thanks so much.

Collapse
xyn profile image
Mydrax

I feel like the title of this article contradicts with the point that I think you are trying to make, which is "comments when misused contribute to code smell". Correct me if I'm wrong though.

Collapse
bytebodger profile image
Adam Nathaniel Davis Author • Edited

We're probably picking nits here, but my point is that:

  1. Comments, when misused, often indicate a code smell.
  2. The vast majority of comments I ever see are unnecessary, and are frequently used to cover up obtuse code. In other words, most comments are absolutely misused. So most comments === code smell.
  3. If the comment tries to tell me what the code is doing, it's a "code smell". Period.
  4. The title, specifically, is a bit of a follow-on from the article that I wrote just before this one, where I basically said that "code smells" don't exist - and they're often the result of people making snobby judgments on other peoples' code.
Collapse
hanpari profile image
Pavel Morava

I completely agree.

Collapse
giorgosk profile image
Giorgos Kontopoulos 👀

Great writeup, my view exactly
if you don't have anything useful in the comments there is no use (get rid of them)

Collapse
ben profile image
Ben Halpern

Great post

Collapse
sargalias profile image
Spyros Argalias

Very nice article with great points. Thank you :)

Collapse
jwp profile image
John Peters

For APIs comments are only way to go.