Comments Are a Code Smell (2019) | Hacker News Reader

Comments Are a Code Smell (2019)

aarongreenwald.com

18 pointskugurerdem14 comments

The assertion that comments are a code smell assumes that the source of complexity in your codebase is either developer taste or some computer-sciency problem that becomes apparent to anyone reading the code in a few minutes or so. But sometimes the complexity is intrinsic to the problem you're solving; sometimes there's some quirk or edge case that means you can't do the obvious thing. In those circumstances, you need comments to explain what you're doing, how you're doing it, and why.

Exactly. I use comments either when some inherent complexity is at play or when I've written seemingly weird code and need to make it easier for myself and others to understand why it's hacky.

I generally try to avoid the latter case, but sometimes, especially when I can't afford to refactor, I just do it, and in such cases having comments is just better than having nothing.

The code tells what the program actually does.

The comments tell what the program should do.

This distinction is so important that English marks it syntactically. Here's an example of how this shakes out:

BAD: // this routine takes its input, multiplies it by 9/5, and then adds 32

Good // this routine converts Celsius to Fahrenheit

Another example: is this code buggy? for(i=0;i<=10;i++) { // do something }

No way to know if you don't know the programmer's intensions, which is to say, if you don't know what the code should do.

You could comment “this routine converts Celsius to Fahrenheit”

Or your could just name the method “celsiusToFahrenheit”

In this example, I’d rather the comment didn’t exist and the method was named appropriately.

That said, there is definitely a place for comments in code, especially for explaining the business context behind a counter-intuitive implementation decision.

// In this example, I’d rather the comment didn’t exist and the method was named appropriately. //

Well-named functions and identifiers are great.

But what should happen, if, say, the user passes in a number which is below absolute zero, kelvin?

Whatever the function does in that case, it is not converting celsiusToFahrenheit. So what is it doing? What should it be doing?

If the function breaks down outside of a certain range, I would have the function return an "out-of-range" failure. Force the caller to handle this case and use the type name as documentation. Example Scala 3 code:

  enum TemperatureError:
    case BelowAbsoluteZero(actual: Double)

  def celsiusToFahrenheit(celsius: Double): Either[TemperatureError, Double] =
    if celsius < -273.15 then
      Left(TemperatureError.BelowAbsoluteZero(celsius))
    else
      Right(celsius * 9.0 / 5.0 + 32.0)

I love this way of handling errors. Very nice.

Yet still, the code is a description of what the code does, not what it should do.

For example, I'm sure Scala has very well defined behavior when the input is, say, a NAN. So we know what the code does do in this case.

Does that tell us what it should do?

In Scala there's no such thing as NaN because Scala has a strong type system (one of the most powerful type systems available in any language).

That said, since it's a JVM language, Scala does unfortunately still support nulls. Scala developers don't ever use null for anything (other than interop with legacy Java libraries) but still, the threat is there.

I'm interested in your comments about intent. What improvements would you implement in the example code I gave?

NAN is a part of the IEEE standard for floating point operations. AFAIK there's nothing which would prevent a NaN from being passed into your function--because NaN is a possible value for a double.

I believe what would happen in that case is that the comparison with (celsius < -273.15) would return false, and so it would not return an error object, but it would fall through into multiplying the Nan by 9, dividing by 5, and then adding 32.

All of these results would return a NaN, due to Nan propagation, and then the function would return a NaN.

I don't know Scala at all, so please let me know if any of this is wrong.

--//--

As for what changes to make: well, I'd add a comment which said that this function should take a temperature measured in Celsius and return the same temperature measured in Fahrenheit.

No matter how much time you spend designing a function (and, ipso facto, how much money your employer spends for you to design that function) you are never going to foresee all possible ways that function will be used.

But that's ok--somebody can come along and fix a bug, or enhance it, if they know what the function should be doing.

This just can't be the code itself saying what it should be doing. If the code itself documents what the function should be doing, then it can't have a bug--what is is doing is exactly identical to what it should be doing.

Yes, it's just hard to come up with good examples off-the-cuff where function/variable naming wouldn't serve sufficiently in place of the comments (you could name the function celsiusToFarenheit, as a sister comment mentions).

But there are cases where this isn't enough context to understand the function's limitations, for example https://github.com/kstenerud/ksbonjson/blob/main/library/src...

That function could be modified to compensate and calculate up to 64 bits, but since none of the callers require all 64 bits to be counted, it would merely add extra useless instructions slowing down the hot path for no good reason.

Then there are cases where the code would be difficult to follow without comments due to the unusual operations it must do: https://github.com/kstenerud/ksbonjson/blob/main/library/src...

In this case, even with descriptive names the branchless algorithm would be much more difficult to follow without comments.

Then there are cases where at first glance it doesn't make sense why you're doing something a certain way: https://github.com/kstenerud/ksbonjson/blob/main/library/src...

A byte array of sizeof(valueBits) + 1 would technically work, but then you'd lose out on an aligned memory write.

Then you have comments that help describe the situation the code is dealing with, in order to make it easier to reason about what the code SHOULD be doing: https://github.com/kstenerud/ksbonjson/blob/main/library/src...

> Well, I dunno. I think the smell was there before you wrote the comments. The comments are febreze.

Awesome! I will never look at comments the same again!

I use a lot of comments during development, but rarely keep them around in final production code. As features become stable, I tend to subconsciously delete them in the underlying areas.

My rationalization is that source control has already captured this history, and if someone really wanted to see what I was thinking, they could review the commit(s) for that file and get a perfect picture.

There are some places where I think comments should persist, such as where you expect future developers (yourself) will be breaking ground again for additional features. Things like "TODO: New Components go here" on a switch statement, or "Review the following issue # for change process around this type" located at the top of a very important piece of code.

> My rationalization is that source control has already captured this history, and if someone really wanted to see what I was thinking, they could review the commit(s) for that file and get a perfect picture.

I used to think this way too... and I still kind of do if things were under control by me. Unfortunately many times things are not under control by me.

You have no guarantee your commit history will be maintained. I don't mean this as in someone is going to do something silly and edit your commit history. I mean this as in people will move your repo. Multiple times.

In these multiple repo moves, people will absolutely perform an 'initial commit' on your 25 year old SVN/Mercurial/etc. repo into the new location. Even worse are the ones that go private git repo to public git repo where this is basically the minimum expectation.

All those tools that help us migrate from one versioning tool to the next be damned.

Some clever folks even plan for this as their earning potential and end up as extended contractors well into their twilight years. I don't fault them for their plan, but can't say I love them either. I won't say their expertise is not needed, it absolutely was/is, but it wasn't their code that represented their expertise.

You have to inherently have some trust in your successors that things will always be the way they are, and that the plan will always work the way it ought to with the right decisions being made the entire way.

From my experience, it doesn't matter how smart or capable or predecessors or successors will be, it may not even matter when it comes to yourself. The short, fast, least mentally taxing route is the route that is taken and becomes de facto the best route.

I'm not really sure what the takeaway is here. It's not "don't write comments" since that would not have helped the situation.

At the very least, if you're going to write comments, bug reports, and code reviews, each one should be descriptive enough _on its own_. Not the right time for DRY! It is indeed irritating having to follow a thread across several places to get to the answer.

Reply on news.ycombinator.com