4 ms·

>The number of times I have been misled by an outdated comment Imo a problem with code review and not leaving comments. Just like tests need changed during ref

by nijave 5y ago

>The number of times I have been misled by an outdated comment Imo a problem with code review and not leaving comments. Just like tests need changed during refactoring, documentation and comments should also be cleaned up as part of the change I have found those as well but place the blame on the person who made the subsequent change and ignored them

oxfeed65261 5y ago
…and on the reviewer(s) who accepted the code change even though the comments no longer match the code.
- Gh0stRAT 5y ago
  I agree, but git and most code reviewing tools I've used only show a few lines of surrounding context so the reviewers might not even realize there's a comment further up that needs updating.
  - msbarnett 5y ago
    Generally speaking, for all but the smallest of changes you should never only review the diff, but pop open the actual file on that branch (GitHub makes this fairly easy, I can't speak to other tooling) and consider the change in the context of the surrounding code. Does it contradict comments? Needlessly duplicate code from elsewhere in the file instead of creating a single function to be used in both places? Shadow variables confusingly? Add yet another 5 lines to a 100 line function instead of leaving the code better than it found it? You can't really judge any of this if you're just eyeballing a diff. Diffs are just the starting point for a review, and while tooling could make this easier, the full file is always just a git checkout away.
prpl 5y ago
There’s not many code review platforms that are great at showing you relevant comments in a diff because they are often in a much different part of the file, so I kind of think it’s hopeless in a large code base to catch this sort of thing in a code review
- tln 5y ago
  I now use VS code for reviewing PRs and it's easy to spot and then touch up comments. With GitHub, not so much. Not sure why code base size would make it harder... Workflow? Code unit size? The larger the code base, the more comment discipline matters IMO
  - prpl 5y ago
    I was mostly thinking about how small changes can sometimes touch a larger quantity of files on moderate or large code bases, so a bit more things to wade through when doing a review, but point taken.
- pcmoney 5y ago
  Comments are not inherently bad but they do have inherent short comings. Still a useful tool but one that should be reached for rarely and last.
  - TheoLib 5y ago
    Comments are "reached for rarely and last". Whatever shortcomings comments have pale in comparison to being faced with and trying to figure out undocumented or too sparsely commented code, which, in my long experience, has been an order of magnitude more prevalent than sufficiently documented/readable code. Heck, even a wrong comment is sometimes preferable to no comment if it at least gives me a clue as to what the purpose of that section of code was at some point in its evolution.
    - prpl 5y ago
      I personally strive for code that can be read as a sentence, especially in complicated sections of code. This sometimes leads to weird intermediate variables which might look superfluous, e.g. intermediates like: foo_has_at_least_1 = len(foo) >= 1 (not a great example, usually it’s a bit more semantics. than that) but later code using those intermediates reads as english, which can reduce cognitive load when reading the code and obviate comments, and preconditions are more readily verified. It’s literate programming and I prefer that to long comment blocks like those in some of the examples. All that said, it can be hard to get coworkers into such a habit, especially those who feel brevity and abbreviation is a virtue.

by nijave 5y ago

oxfeed65261 5y ago

…and on the reviewer(s) who accepted the code change even though the comments no longer match the code.

Gh0stRAT 5y ago
I agree, but git and most code reviewing tools I've used only show a few lines of surrounding context so the reviewers might not even realize there's a comment further up that needs updating.
- msbarnett 5y ago
  Generally speaking, for all but the smallest of changes you should never only review the diff, but pop open the actual file on that branch (GitHub makes this fairly easy, I can't speak to other tooling) and consider the change in the context of the surrounding code. Does it contradict comments? Needlessly duplicate code from elsewhere in the file instead of creating a single function to be used in both places? Shadow variables confusingly? Add yet another 5 lines to a 100 line function instead of leaving the code better than it found it? You can't really judge any of this if you're just eyeballing a diff. Diffs are just the starting point for a review, and while tooling could make this easier, the full file is always just a git checkout away.

prpl 5y ago

There’s not many code review platforms that are great at showing you relevant comments in a diff because they are often in a much different part of the file, so I kind of think it’s hopeless in a large code base to catch this sort of thing in a code review

tln 5y ago
I now use VS code for reviewing PRs and it's easy to spot and then touch up comments. With GitHub, not so much. Not sure why code base size would make it harder... Workflow? Code unit size? The larger the code base, the more comment discipline matters IMO
- prpl 5y ago
  I was mostly thinking about how small changes can sometimes touch a larger quantity of files on moderate or large code bases, so a bit more things to wade through when doing a review, but point taken.
pcmoney 5y ago
Comments are not inherently bad but they do have inherent short comings. Still a useful tool but one that should be reached for rarely and last.
- TheoLib 5y ago
  Comments are "reached for rarely and last". Whatever shortcomings comments have pale in comparison to being faced with and trying to figure out undocumented or too sparsely commented code, which, in my long experience, has been an order of magnitude more prevalent than sufficiently documented/readable code. Heck, even a wrong comment is sometimes preferable to no comment if it at least gives me a clue as to what the purpose of that section of code was at some point in its evolution.
  - prpl 5y ago
    I personally strive for code that can be read as a sentence, especially in complicated sections of code. This sometimes leads to weird intermediate variables which might look superfluous, e.g. intermediates like: foo_has_at_least_1 = len(foo) >= 1 (not a great example, usually it’s a bit more semantics. than that) but later code using those intermediates reads as english, which can reduce cognitive load when reading the code and obviate comments, and preconditions are more readily verified. It’s literate programming and I prefer that to long comment blocks like those in some of the examples. All that said, it can be hard to get coworkers into such a habit, especially those who feel brevity and abbreviation is a virtue.