5 ms·

Usually you can check the commit history for the unexpected line to figure out what's up, though, to let you figure out if the bug is in the code or the comment

Usually you can check the commit history for the unexpected line to figure out what's up, though, to let you figure out if the bug is in the code or the comment. Comments that have the same content as the code but written in English will have code drift problems. But most comments aren't like this; they can provide context or explain what's happening at a higher level, ideally.

danuker 4y ago
Indeed, comments should answer the "why". But ideally the comments should be executable, as unit tests, making you read them if and only if you break them. For this to be a tolerable development experience, test as much as you can while keeping your tests away from slow dependencies like networking, DB, disk I/O..., and try to keep tests relevant to what you're modifying executable locally in a few seconds. Maybe even refactor your app to have dependencies at the top, so that most code doesn't have access to them.
- TeMPOraL 4y ago
  > But ideally the comments should be executable, as unit tests For the kinds of comments that answer the "why", if we could do that, we wouldn't need the actual code in the first place. > making you read them if and only if you break them. A good "why" comment is supposed to inform you beforehand, so you can make changes effectively and without introducing extra bugs in the process. Unit tests are more of a safety net.
  - dhosek 4y ago
    I’m imagining the poster is thinking about things like rust doctests where code in the comments will be executed as tests when you run cargo test on the project. It’s a nice way of being able to ensure that (at least part of) the documentation will correspond to the behavior of the code.
    - danuker 4y ago
      I am thinking of any fast unit test system. Going in seconds from red to green is a thrill.
      - TeMPOraL 4y ago
        Unit tests suck at being documentation, and are not a good substitute for the "why" comment. They can catch some mistakes you make with the code under test, but they can't tell you why it is the way it is. At best, they can help you guess, at the cost of having to study the test code itself (which is usually much bigger than the code it tests, and often more complicated). But the thing is, the knowledge of "why" is most valuable to have before you start making changes and break some tests.
        dhosek 4y ago
        This is true, test coverage, especially in code that has to interact with other systems in particular ways will often have ten lines of setup that only matters in the test for every line of actual verification.
- dhosek 4y ago
  On the flip side, I’ve had things like: // in case this is malformed, fix formatting so it will still parse input = fixInputFormatting(input); and had a code reviewer ask, “why are you calling fixInputFormatting”? Nothing to raise the blood pressure like a code review question that is literally answered by a comment on the line immediately preceding where they left the question.
  - wizofaus 4y ago
    But in this case surely you're better off with input = fixInputFormattingIfMalformed(input); or even if (isMalformed(input)) input = fixInputFormatting(input);
  - scatters 4y ago
    I'm with the reviewer on this one. Why is it malformed? Why are you fixing it here and not earlier? Why are you fixing it and not rejecting it? The comment tells me nothing.
    - dhosek 4y ago
      I don’t remember the exact comment/cope pair anymore, but it was something that the answer they wanted was exactly what was on the preceding line. Coming up with a simple example to demonstrate that is a surprisingly hard thing to do.
sroussey 4y ago
Speaking of commit history, bad merges can leave comments and code together that shouldn’t be. And it will pass tests.
- layer8 4y ago
  Bad merges can also leave code and code together that shouldn’t be, even if it passes tests. Always check your merges.
- sdenton4 4y ago
  Curiously this seems like exactly the story of thing that code ML should be able to identify and flag. 'Hey, the comment says this, but the code is doing something totally different.'
  - bluocms 4y ago
    Then the comment can be removed, as is duplicate of code and has no use.
  - nextaccountic 4y ago
    However, it might be trained on code with erroneous comments, either because it was mangled by a merge or because it's outdated. The more times this happen, the more confused the AI model will be. Which makes me to think that the AI models should be trained on the code evolution of commit chains and not just on isolated snippets of code. That way, the AI could analyze your own commits to detect when a comment becomes outdated.
  - ilyt 4y ago
    I'd settle for AST-aware merge. That also fixes comments not glued to code they were attached to before
tetha 4y ago
A good rule of thumb I've heard about a few years ago: Assume you're explaining this chunk of a system to a team member standing next to you. Write that down. And don't worry about the tone feeling casual. And once or twice at the top of a weird class/task file/section/..., don't be afraid of being a bit verbose and explain it until it's obvious, and then one more level. Stuff tends to be obvious while you have all the context uploaded in your mental caches - but a year down the line, it'll be rather confusing. Still, having such long comments too much in straight line code tends to make it harder to read.

danuker 4y ago

Indeed, comments should answer the "why". But ideally the comments should be executable, as unit tests, making you read them if and only if you break them. For this to be a tolerable development experience, test as much as you can while keeping your tests away from slow dependencies like networking, DB, disk I/O..., and try to keep tests relevant to what you're modifying executable locally in a few seconds. Maybe even refactor your app to have dependencies at the top, so that most code doesn't have access to them.

TeMPOraL 4y ago
> But ideally the comments should be executable, as unit tests For the kinds of comments that answer the "why", if we could do that, we wouldn't need the actual code in the first place. > making you read them if and only if you break them. A good "why" comment is supposed to inform you beforehand, so you can make changes effectively and without introducing extra bugs in the process. Unit tests are more of a safety net.
- dhosek 4y ago
  I’m imagining the poster is thinking about things like rust doctests where code in the comments will be executed as tests when you run cargo test on the project. It’s a nice way of being able to ensure that (at least part of) the documentation will correspond to the behavior of the code.
  - danuker 4y ago
    I am thinking of any fast unit test system. Going in seconds from red to green is a thrill.
    - TeMPOraL 4y ago
      Unit tests suck at being documentation, and are not a good substitute for the "why" comment. They can catch some mistakes you make with the code under test, but they can't tell you why it is the way it is. At best, they can help you guess, at the cost of having to study the test code itself (which is usually much bigger than the code it tests, and often more complicated). But the thing is, the knowledge of "why" is most valuable to have before you start making changes and break some tests.
      - dhosek 4y ago
        This is true, test coverage, especially in code that has to interact with other systems in particular ways will often have ten lines of setup that only matters in the test for every line of actual verification.
dhosek 4y ago
On the flip side, I’ve had things like: // in case this is malformed, fix formatting so it will still parse input = fixInputFormatting(input); and had a code reviewer ask, “why are you calling fixInputFormatting”? Nothing to raise the blood pressure like a code review question that is literally answered by a comment on the line immediately preceding where they left the question.
- wizofaus 4y ago
  But in this case surely you're better off with input = fixInputFormattingIfMalformed(input); or even if (isMalformed(input)) input = fixInputFormatting(input);
- scatters 4y ago
  I'm with the reviewer on this one. Why is it malformed? Why are you fixing it here and not earlier? Why are you fixing it and not rejecting it? The comment tells me nothing.
  - dhosek 4y ago
    I don’t remember the exact comment/cope pair anymore, but it was something that the answer they wanted was exactly what was on the preceding line. Coming up with a simple example to demonstrate that is a surprisingly hard thing to do.

sroussey 4y ago

Speaking of commit history, bad merges can leave comments and code together that shouldn’t be. And it will pass tests.

layer8 4y ago
Bad merges can also leave code and code together that shouldn’t be, even if it passes tests. Always check your merges.
sdenton4 4y ago
Curiously this seems like exactly the story of thing that code ML should be able to identify and flag. 'Hey, the comment says this, but the code is doing something totally different.'
- bluocms 4y ago
  Then the comment can be removed, as is duplicate of code and has no use.
- nextaccountic 4y ago
  However, it might be trained on code with erroneous comments, either because it was mangled by a merge or because it's outdated. The more times this happen, the more confused the AI model will be. Which makes me to think that the AI models should be trained on the code evolution of commit chains and not just on isolated snippets of code. That way, the AI could analyze your own commits to detect when a comment becomes outdated.
- ilyt 4y ago
  I'd settle for AST-aware merge. That also fixes comments not glued to code they were attached to before

tetha 4y ago

A good rule of thumb I've heard about a few years ago: Assume you're explaining this chunk of a system to a team member standing next to you. Write that down. And don't worry about the tone feeling casual. And once or twice at the top of a weird class/task file/section/..., don't be afraid of being a bit verbose and explain it until it's obvious, and then one more level. Stuff tends to be obvious while you have all the context uploaded in your mental caches - but a year down the line, it'll be rather confusing. Still, having such long comments too much in straight line code tends to make it harder to read.