Hacker News Reader

Comment by piess | Hacker News Reader

You shouldn't need many comments if your code is clearly written and named. Just a summary above each method.

Oh yes you do need. Plenty of time you need to explain why the code does what it does. You rarely need to explain only how it works.

You can put this in the summary

You can't more often than not. Often the "why" is an implementation detail regarding a single statement, and those shouldn't make their way to a function summary.

If there's hidden meaning in an implementation detail regarding a single statement extract that value to one of the following: a) a variable b) a method c) a class All of these provide a mechanism for documenting the code without comments. Show me where you think this doesn't apply.

I've seen lots of people make this assertion, most of the code I've seen doesn't need more than the how. Can you give some examples of well written code where there's significant amounts of comments that explain the why?

Where the code itself looks like it's written crappily, but cannot be implemented in a clean way, due to the API used or performance or other issue.

  class CrappyApiWrapper
  {
    CrappyApi _crappyApi;
  
    void DoSomethingNice() {
      crappyApi.DoSomethingCrap();
    }
  }

In certain types of code, the purpose of some logic is not clear by itself.

A comment can be very valuable to answer the question "why the heck is this being done here?"

It's valuable because it represents hard-earned knowledge.

That thing that is being done there wasn't being done there before, and it took weeks to figure out that it had to be done there, for the sake of such and such a race condition that can occur, when the interrupt goes off just when we are doing such and such elsewhere in the driver. It was hard to reproduce the ensuing problem and trace it to this.

The comment explains how the system will fall down if not for those lines of code.

Systems that have a lot of (or not even a lot of) concurrency going on can be very hard to understand from the static view. Our aim is to produce the dynamic view: to bring to life the collaboration or sequence diagram that we had on the whiteboard. But it's not obvious how (or whether) the code actually does that (and only that, ruling out unwanted scenarios).

"Why" comments can also make the code more navigable, because they mention other identifiers elsewhere in the code which are related to this code here in ways which are not obvious. The text editor can jump from these mentions to their definitions, which is a big time saver for someone trying to understand that situation.

Sometimes you have to work with a third-party system or interface that is sending your application dirty data, and you need to account for it in your code in creative ways that may not be obvious to someone who is just reading it.

For example in our current project w'ere working on a front-end piece that connects to a CRM application where no rules or constraints have been set up or enforced for a long time. We're talking stuff like zip code fields having values like "I don't know" or "XXXXX". So for every function that receives such values from a lookup, we have to clearly document those edge-cases in the comments and explain how the function deals with them.

  ZipCode GetZipCode()
  {
     var result = _crm.GetZip();
     return isValidZip(result) ? new ValidZip(result) : new InvalidZip(result);
  }

  class ValidZipCode : ZipCode { ... }
  class InvalidZipCode : ZipCode { ... }

No edge cases, perfectly 'documented'.

How do you consider that to be documented?

Why do you return a different class for invalid zips? Is it OK to use invalid zips in all cases? If not, why not throw an error? If so, what benefit does the separate class give you?

Because that code doesn't handle bad data - something in the objects or some other function must handle it, and this code doesn't really tell us what is going to happen with a bad zip. It just tells us that the code noticed it.

This is exactly why comments can help.

See the null object pattern. Encapsulate all such behavior you've mentioned with appropriate OO patterns, and stop writing procedural code spread all over the place that has to rely on comments to inform you about the right way to handle the bad data. Handle the bad data cases simply and specifically.

Feel free to add some code that explains your use case and where you think such comments are required...

Yes, all over the place, explaining the business reasons behind the algorithms, or why some code was put in place to resolve a specific browser oddity (usually with dates and versions so we also know when it can be removed in the future). We also use comments to indicate when code is written to follow a specific decision from the CEO, and was not just an arbitrary design choice by us coders.

In short, we make sure that a coder can read the code and understand not just the code, but how it fits into the larger business.

  var browserOddityHandlers = [
    { browser: "IE", date: "2001-08-27", version: 6, odditiesHandlers: [ ... ] },
    ... ];

Code is rarely an arbitrary design choice. Such design choice decisions as you mentioned belong in your source repository message IMO, rarely in the code.

So when you are coding, and start looking at an existing function, you are telling me that you go back to your repository and read through the entire commit history for that function?

If any function has such a significant commit history that this is a problem in your code, there's probably a violation of the SRP right in front of you. Stop modifying it. Write modules that are small enough that their usage is obvious and you'll find it less likely that you're changing code so often that commit history on a function matters.

In the case of the CEO mandated approach, write a test that explains it. Why comment something that can be tested.

  [Fact]
  void FooReturnsBazBecauseJoeMadeUsDoThisInsteadOfBar()
  {
    var actual = _someObject.Foo();
    Assert("baz", actual);
  }

This way, if that code ever is changed, the comments and the code don't get out of whack with each other. Instead, you'll have a broken test. At that point it becomes obvious to reach for understanding as to why that test is there. The first step is read the commit history. If your commit history is useless, you talk to the dev that implemented it, or go back to the spec that drove it. These solutions are still 100x more useful than a comment.

Here's the easiest example I can come up with:

    // Optimized this code as it's a hotspot. Does <x> but quickly.

how is this better than:

  void DoActionOptimized() {
  ...
  }

Optimized how? Faster? Less memory? Something more subtle?

That's what naming is for. MemoryOptimizedFoo, FastFoo, ...

In fact you might also want the un-optimized code around and run the optimized version "against" the un-optimized version (in tests) to see if they actually do the same thing.

Extract an interface (IFoo). Implement Foo : IFoo and FasterFoo : IFoo. Write tests to confirm all implementations of IFoo act equivalently. Write performance characteristic tests around FasterFoo.

Exactly.

Because at one point, the code was not optimized, so it already had a function name. Are you really going to rename all instances where that function is called so it is named based on a single commit, instead of its actual purpose?

There are many ways that are non breaking to implement this. I chose a particular one for example's sake. In reality, extracting an interface is a more valuable refactoring, and a likely valid one given that the optimization tradeoff is happening. So MemoryOptimizedFoo : IFoo, SpeedOptimizedFoo : IFoo...

I'm going to pull the "shutup and show me the code" plug on replies to this. If you're still writing prose in response to my question, you're in the exact same bucket at those I mentioned making the assertion.

Nope. In my career I've never seen this mythical code that doesn't need comments. Tell me about preconditions, postconditions, performance characteristics, quirks, absolutely anything and everything that could affect the caller.

One of my favorites was a hex string (ascii) to binary string (ascii) converter that ran all on one line. I think my colleague found it online, because they were nowhere near technical enough to implement it, let alone on a single line. Nary a comment or clue in the context to figure out why or what it was doing.

I broke it into 3 lines and commented each.

The point is that writing comments isn't a substitute for writing clear code. I have seen way too much code that someone puked up without thinking about readability, and then tried to save it by sprinkling it with comments.

So the first thought when writing code that isn't obvious should not be to comment it, but to figure out if there is a way to write it clearer.

But of course, sometimes we need to write code that isn't obvious, and then a few well thought through comments will save the day.

I've often seen assertions like this that all / most code requires comments. I hold a personal view that it's actually the opposite, but can't seem to get anyone above to bite and show me useful real code provides a counterpoint to my view.

Can you point to an open source repo somewhere that demonstrates your point that comments are preferable to writing clear code without comments?

I'm sure you can point to repos where poorly written code requires comments in order to be properly explained. This is not really what I'm after, as I'd contend it's likely that writing the code better would be more useful to that project.

This is already a softer statement than the rule proclaimed in the article ("not many" vs "none").

And if you have to do something unusual due to limitations (say, a bug in a 3rd-party library) in your code I don't want to know about that when reading the method's documentation (what you refer to as "summary") to be able to use your code. I want to be bothered with it when I have to read its implementation because I have to fix some bug in it.

Reply on news.ycombinator.com