Hacker News Reader

I have heard this so many times:

> Comments: I find comments, outside of docstrings, a smell.

But is this really the case? I find comments like these invaluable:

     /*
      * Mark walreceiver as running in shared memory.
      *
      * Do this as early as possible, so that if we fail later on, we'll set
      * state to STOPPED. If we die before this, the startup process will keep
      * waiting for us to start up, until it times out.
      */
     SpinLockAcquire(&walrcv->mutex);

https://github.com/postgres/postgres/blob/master/src/backend...

Part of the job of a developer is to communicate

- domain model (Concepts, Relations between them)

- intention behind the code (Why is this code here?)

- possible pitfalls (I tried this, it does not work because ...)

- limitations (TODO: This special case does not really work, yet)

for your colleagues and future heirs.

How would you ever cramp information like this into variable names and types? (... without introducing so much abstractions that the code becomes unreadable).

This is EXACTLY where comments should be used, great example!

Extremely high-performance database code is not what most of us are doing, however. We are doing enterprise software development to send emails and scrape money out of people.

I would want that function to be multiple smaller functions with docstrings and such, but obviously, that would add stackframes to the call stack and be slower.

Additionally, many modern compilers have an easier time reasoning about and optimizing multiple smaller functions as opposed to huge functions with a ton of variables and logic. Plus large functions have an impact on garbage collection etc. etc.

> Extremely high-performance database code is not what most of us are doing, however.

That's true. But look at the overall coding style in that file:

Code is structured into cohesive blocks, which are prefixed by a comment which explains in english language the intention, pitfalls, limitations of the following code.

I find this quite elegant and readable. An ideal to aspire to.

I am not advocating nonsensical JavaDoc:

    // Get's foo from bar
    private Foo get_foo(Bar bar);

If you are able to just use names and types to document your intentions, pitfalls etc ... then by all means, do just this. E.g.

     for( person in address_book ) {
          body = template.render( name => person.name, ... )
          mail = Mail.new(person.email, body)
          mailgun.queue(mail)
     }

However, if stuff get's more nuanced don't be afraid to use english language to explain what you are doing. You future self will thank you : )

> I would want that function to be multiple smaller functions with docstrings and such [...]

I have tried that style in the past, and have reverted to writing longer functions with intermittent comments. I now consider "single caller functions" a smell.

If logic is serial (do A, then B, then C) it's fine to have serial code:

   main() {
     ... A ...

     ... B ...

     ... C ...
   }

The issue with splitting out the intermediate steps into functions is, that the serial flow is broken. Instead of the logic now looks something like

    A(...){ ... }
    C(...){ ... }
    main() { 
       A(...)
       ... B ...
       C(...)
    }

Where B might have been a tiny task, that is not worth splitting out. In effect:

- The ordering of the logic is broken.

- Passing arguments can be a pain, if a lot of pieces are touched.

And what have you gained? Not sure there are many upsides besides debatable aesthetics of avoiding inline code comments.

> If you are able to just use names and types to document your intentions, pitfalls etc ... then by all means, do just this

That's sort of the point. Step 1, your code should be self explanatory. When the natural interpretation a reasonable person would have from reading the code differs from reality: that's when you add Step 2: add comments to "fix" the gap between perception and reality. The less Step 2 is necessary the better.

An example straight from a new hire's code review a few weeks ago:

  // foo = 12345

  doThing1(12345, 5) // foo
  doThing2(12345, 8) // foo

Preferring self-descriptive code instead of comments helps stop them from doing things like this.

> If you need to comment on what something is doing, usually you can move that behaviour into a function and test that function.

That's the usual argument, and the counter-argument is always the same: you shouldn't comment the what (though I'll argue there's always exceptions), you should comment the why.

Why are you doing this processing here rather than there? Why do you not apply the usual pattern in this specific case? Why does this code, that will make you want to refactor, has been written like this?

> If you need to comment on what something is doing, usually you can move that behaviour into a function and test that function.

Strong disagree. As a general principle, a refactoring that changes no behavior should break no tests. It's all well and good to factor out a chunk of code into a separate internal function for code clarity, but don't go writing a test for it. Don't plumb your tests through the internals of the code. If you're factoring that piece of functionality out into the public interface of another component, sure, you should test the public interface of that component. But if you're factoring it out into an implementation detail of the same component it was originally part of don't write a test that cares about the details of you implementation. The functionality it provides should already be testable via calling the function it was originally factored out of.

> Comments: I find comments, outside of docstrings, a smell.

Neither function/type/whatever names, nor unit tests document architecture, concurrent behaviour, locking, ... to a significant degree. For simpler stuff that's fine, but it doesn't take that complicated a problem that some interdependencies aren't obvious anymore.

Even in the simpler cases that then often leads to APIs/Architecture that appears "fractured". Because the intent from the time the code was originally written isn't visible enough, subsequent work is designed differently, rebuilds parts of infrastructure, ... That leads to harder to maintain code over time.

IME there's a significant difference in how much effort is worthwhile to put into comments and understandable code depending on the expected lifetime, staff turnover, total size of a project.

the more testing the better? while having tests at unit and integration levels are good...

Test as little as possible/responsible.

I think it was Kent Beck who first articulated that, though it was something many people converged on in the early 2000s in the XP world. Of course it doesn't mean test nothing, it's recognizing tests are liabilities (as all code is) and we need to make sure they are serving a purpose otherwise delete them / refactor them to get rid of duplication of testing and overly verbose long winded tests that keep repeating things over and over again and again, saying the same thing repeatedly, doubling up on testing :)

Wasn't it Kent Beck who came up with Extreme Programming and Test-Driven Development? I think testing was one of the big corner-stones of XP. At what point did that turn into "Test as little as possible"?

"Unit tests are one of the CORNER STONES of Extreme Programming (XP)"

http://www.extremeprogramming.org/rules/unittests.html

Also see: https://hygger.io/blog/tests-in-extreme-programming/

If development is tests-driven and you should minimize the amount of tests then doesn't that also mean you should minimize the amount of development?

Well of course, do not develop more than is needed. But that kind of trivializes the whole agile ideology: "Develop As Little As Possible, that allows you to remain the most Agile"

Minimizing the amount of development also minimizes the number of bugs. Sounds like a great concept.

its the same concept as minimizing the amount of code through design and refactoring. Let the tests drive the code, refactor, and refactor / delte your tests when not needed anymore. :)

this is his SO answer where he talks about it... https://stackoverflow.com/questions/153234/how-deep-are-your...

Makes sense. But it does seem like a shift in position from the very "test-centric" view of XP.

In the above link I don't see anything about "delete your tests when not needed anymore", or did I miss it?

How do you know when some tests are "not needed any more"? Isn't the idea of regression-testing to keep tests around so you know if any new code will break them? That should give you the "agility" to refactor your code, I think, is what the proponents of XP would say.

> Unit tests are for refactoring

I would say that unit tests impede refactoring. If your refactoring changes classes interfaces or API, you are likely to have to change your unit tests as well.

> Code that is easy to unit test tends to be more modular and have better architecture. If you are struggling to test a function

Same goes with documentation. If you can't explain simply what a function does, you may want to rewrite it.

> I would say that unit tests impede refactoring. If your refactoring changes classes interfaces or API, you are likely to have to change your unit tests as well.

Strongly disagree. How do you modify a function or interface if you do not know what breaks when you use that function?

Personally, I do not like to have to grok the entire codebase and keep a map of it in my brain at all times. I like to concentrate on the behaviour I am currently implementing and be VERY sure that the changes I am making are not breaking elsewhere.

If changing a function breaks a bunch of tests: Good. Otherwise, all of those broken tests would have been bugs I get to find in production and explain to my boss.

Suppose you have a service A, that uses two classes X and Y. Then you realize that it would be better for A to use X' and Y' (with different interfaces). If you have unit tests for X and Y, you will have to change these tests as well. If you do a lot of refactoring, you will spend a lot of time maintaining the unit tests. On the other hand, if you rely on integration tests (which test A), you still test X and Y, but your tests are less likely to change over time.

I'm not saying unit tests don't have their use, but as far as refactoring goes, I think they are a hindrance. Unless we talk about rewriting just a function.

Interesting. I love the idea of generating documentation from code - it does a lot of things right: Single source of truth (right now we have 3), easier to update when changing code as it's right there.

But I'm not sure everything fits in there. Where do you document processes like how to test stuff that isn't possible to test in a local-dev environment?

Or .. what the IDs that have some business meaning, mean.

> But I'm not sure everything fits in there. Where do you document processes like how to test stuff that isn't possible to test in a local-dev environment?

I feel like that would go into the README or an attached wiki page.

It is funny, after writing my MILLIONTH validation library, I've been thinking OpenAPI or APIBlueprint driving my validation and parsing library at my web application boundaries is the way to go... which is kind of the opposite. Documentation driving my software xD