4 ms·

A perfect example of a missing code-comment.

by Syssiphus 13y ago

mattmanser 13y ago
More like a terribly named variable.
- denizozger 13y ago
  Agreed, comments are generally a way to compensate failure to express ourselves in the code (in this case bad naming).
  - thaumasiotes 13y ago
    Actually, there's a difficulty here that naming can't solve; I don't see a better method than the comment. The goal is to enumerate the powers of 3, 5, and 7, once each. Since power sequences all overlap at x^0 = 1, but we specifically don't want to enumerate 1 three times, we have to give one (or, from an alternative viewpoint, two) of the variables special treatment. Whether you name the variables "three", "five", and "seven", or "next_power_of_three", "next_power_of_five", and "next_power_of_seven", you're doing something strange by starting one of them at 1 and the other two past 1, and that should be commented on. The naming-only solution "powers_of_three_initialized_starting_at_three_to_the_zeroeth", "powers_of_five_initialized_starting_at_five_to_the_first", and "powers_of_seven_initialized_starting_at_seven_to_the_first", is hilariously awful, and still requires a comment to explain why the threes variable is more (or less) special than the other two.
    - stephencanon 13y ago
      The goal is not to enumerate the powers of 3, 5, and 7. The goal is to iterate through the groups which hold BACKUP superblock/GDT copies. “three”, “five” and “seven” are not especially good names for iterator state, nor is there an obvious good reason for exposing the inner details of iterator state.
      - chengiz 13y ago
        Yes thank you! People seem to be missing this point. The variables should be simply named something like group_counter_a, group_counter_b, group_counter_c, with an explanation of why their default values are what they are.
    - mseebach 13y ago
      I think the problem is the mixing of concerns: The generation of the sequence (which apparently is a function of whether the filesystem is sparse or not) and whatever it is trying to verify.
    - userbinator 13y ago
      Or how about just combining all those variables into an array named "iterator_powers" or similar? There could be another one, "iterator_multipliers" containing 3, 5, and 7. I don't know if this is actually a "best practice" or rule, but I've found that if I want to name variables after numbers, usually what I'm trying to do should be using an array.
dangrossman 13y ago
The explanatory comment is 40 lines earlier in the same file where the function those variables are being passed to is defined. It need not be repeated every couple lines; "being clear to outsiders linked to a specific line of a specific file without context" is not a reasonable concern.
- annnnd 13y ago
  In this case I think it is not about comment at all - it is about poor naming. More descriptive (or less misleading) var names would help in this case.
  - spinlock 13y ago
    I agree. 'power_of_three' isn't really that much of a hardship to type and it obviates the need for a comment.
- orclev 13y ago
  The function declared earlier is perfectly fine, the comment is sufficient to explain what it's doing, but functions should be understandable simply by reading their source and in that respect the linked function fails. This would be really simple to solve with a simple comment, E.G. // current power of three, init to 3^0 unsigned three = 1; The fact that this looks like a bug at first glance is a pretty good indication that there should be some explanation of what exactly it's doing.
  - awda 13y ago
    This is actually contrary to Linux kernel "good style". Functions should be short and understandable. Comments should be on the top of the function, describing what the function is for. Comments describing variables in functions are discouraged.
    - jrowen 13y ago
      Are you supporting that dogma or just stating it? This brings to mind the Orwell essay on language usage[1]: Break any of these rules sooner than say anything outright barbarous. IMO having "three = 1" with no immediate context qualifies as barbarous. Yes it would be best to rename the variable, but, failing that, just toss in a freaking comment for common sense's sake. [1]https://www.mtholyoke.edu/acad/intrel/orwell46.htm https://www.mtholyoke.edu/acad/intrel/orwell46.htm
CrystalCuckoo 13y ago
Not really, the problem lies in its naming. Comments that explain what the variables represent signify that they should have been better named in the first place to avoid any confusion.
stefan_kendall 13y ago
Nope. The variable name is bad.

A perfect example of a missing code-comment.

mattmanser 13y ago

More like a terribly named variable.

denizozger 13y ago
Agreed, comments are generally a way to compensate failure to express ourselves in the code (in this case bad naming).
- thaumasiotes 13y ago
  Actually, there's a difficulty here that naming can't solve; I don't see a better method than the comment. The goal is to enumerate the powers of 3, 5, and 7, once each. Since power sequences all overlap at x^0 = 1, but we specifically don't want to enumerate 1 three times, we have to give one (or, from an alternative viewpoint, two) of the variables special treatment. Whether you name the variables "three", "five", and "seven", or "next_power_of_three", "next_power_of_five", and "next_power_of_seven", you're doing something strange by starting one of them at 1 and the other two past 1, and that should be commented on. The naming-only solution "powers_of_three_initialized_starting_at_three_to_the_zeroeth", "powers_of_five_initialized_starting_at_five_to_the_first", and "powers_of_seven_initialized_starting_at_seven_to_the_first", is hilariously awful, and still requires a comment to explain why the threes variable is more (or less) special than the other two.
  - stephencanon 13y ago
    The goal is not to enumerate the powers of 3, 5, and 7. The goal is to iterate through the groups which hold BACKUP superblock/GDT copies. “three”, “five” and “seven” are not especially good names for iterator state, nor is there an obvious good reason for exposing the inner details of iterator state.
    - chengiz 13y ago
      Yes thank you! People seem to be missing this point. The variables should be simply named something like group_counter_a, group_counter_b, group_counter_c, with an explanation of why their default values are what they are.
  - mseebach 13y ago
    I think the problem is the mixing of concerns: The generation of the sequence (which apparently is a function of whether the filesystem is sparse or not) and whatever it is trying to verify.
  - userbinator 13y ago
    Or how about just combining all those variables into an array named "iterator_powers" or similar? There could be another one, "iterator_multipliers" containing 3, 5, and 7. I don't know if this is actually a "best practice" or rule, but I've found that if I want to name variables after numbers, usually what I'm trying to do should be using an array.

dangrossman 13y ago

The explanatory comment is 40 lines earlier in the same file where the function those variables are being passed to is defined. It need not be repeated every couple lines; "being clear to outsiders linked to a specific line of a specific file without context" is not a reasonable concern.

annnnd 13y ago
In this case I think it is not about comment at all - it is about poor naming. More descriptive (or less misleading) var names would help in this case.
- spinlock 13y ago
  I agree. 'power_of_three' isn't really that much of a hardship to type and it obviates the need for a comment.
orclev 13y ago
The function declared earlier is perfectly fine, the comment is sufficient to explain what it's doing, but functions should be understandable simply by reading their source and in that respect the linked function fails. This would be really simple to solve with a simple comment, E.G. // current power of three, init to 3^0 unsigned three = 1; The fact that this looks like a bug at first glance is a pretty good indication that there should be some explanation of what exactly it's doing.
- awda 13y ago
  This is actually contrary to Linux kernel "good style". Functions should be short and understandable. Comments should be on the top of the function, describing what the function is for. Comments describing variables in functions are discouraged.
  - jrowen 13y ago
    Are you supporting that dogma or just stating it? This brings to mind the Orwell essay on language usage[1]: Break any of these rules sooner than say anything outright barbarous. IMO having "three = 1" with no immediate context qualifies as barbarous. Yes it would be best to rename the variable, but, failing that, just toss in a freaking comment for common sense's sake. [1]https://www.mtholyoke.edu/acad/intrel/orwell46.htm https://www.mtholyoke.edu/acad/intrel/orwell46.htm

CrystalCuckoo 13y ago

Not really, the problem lies in its naming. Comments that explain what the variables represent signify that they should have been better named in the first place to avoid any confusion.

stefan_kendall 13y ago

Nope. The variable name is bad.