Timeline for How to avoid comments about one line of code for cleanliness
Current License: CC BY-SA 4.0
23 events
| when toggle format | what | by | license | comment | |
|---|---|---|---|---|---|
| Feb 7, 2020 at 15:13 | comment | added | Christian Hackl |
@MechMK1: Yes, I'm sure. XYZExceptWithProductionDataDueToRAMConstraints() is long and convoluted, and also wrong, because the point of the comment is that XYZ is not called at all, even though it would seem a logical choice there at first glance. Turning that simple comment into a function is a solution in search of a problem.
|
|
| Feb 7, 2020 at 15:04 | comment | added | Graham | @R.Schmitz Yes, really. Your application is also supposed to be reviewable and maintainable by another engineer, and that's what makes our job "software engineering" and not "randomly hacking stuff together". :) In that sense, it does not do what it's supposed to do. For a more functional issue, I might also not tell my manager that the code is "broken" because I missed checking memory allocation succeeded. (Hardly ever going to fail, after all.) But I'd track it (formally or informally) as something where the code is incorrect and should be fixed; and the same for the dodgy comment. | |
| Feb 7, 2020 at 15:04 | comment | added | MechMK1 |
@R.Schmitz I generally agree, but it depends on the specific "exception case". If it's something the "average programmer" would intuitively grasp (e.g. supplying null may throw an exception), then a comment is optional. If it's something that's not obvious (e.g. beeps below 37 Hz throw an exception) then I would add some comment in a logical place explaining why that is the case (e.g. where the exception is thrown)
|
|
| Feb 7, 2020 at 14:51 | comment | added | R. Schmitz |
@MechMK1 I prefer DoComplexThing() without a comment // Do complex thing unless arg is null then throw exception. Much, much clearer and not repeating what is already in the code.
|
|
| Feb 7, 2020 at 14:16 | comment | added | MechMK1 |
@R.Schmitz Yes, I'm not used to it, for better or for worse. But if I look at some popular and - by my own opinion - well-designed frameworks, I see methods like DoThing() or DoComplexThing(withArg) and less like DoComplexThingUnlessArgIsNullThenThrowException(withArg).
|
|
| Feb 7, 2020 at 14:12 | comment | added | R. Schmitz | @Graham "Comments are code" - Are they really, though? I guess this is up to different viewpoints, but if the application 100% does what it's supposed to do... I'm not gonna tell my manager that the code is broken because there's 2 outdated comments. | |
| Feb 7, 2020 at 14:08 | comment | added | R. Schmitz | @MechMK1 Well, if you think it's "weird", it just means you are not used to it. That will differ from person to person. In general, I would expect an English speaker with a programming background to not bat an eye at "Avoid excessive RAM usage in production". | |
| Feb 7, 2020 at 13:52 | comment | added | MechMK1 |
I personally prefer comments explaining something unusual over using weird method names. if(this.Environment != Environment.Production) DoStuff(); //Don't call DoStuff() in the production environment because ... is better than DoStuffExceptInProduction(). Even better would be if DoStuff() would raise a compile-time warning to not use this method in production, as it will cause issues.
|
|
| Feb 7, 2020 at 13:47 | comment | added | R. Schmitz |
@MechMK1 That works well if there's an if statement (although I'd probably prefer a shorter version like AvoidExcessiveRamUsageInProduction()). However, if an otherwise expected line isn't there at all in the code, a comment makes more sense than adding a method that does nothing.
|
|
| Feb 7, 2020 at 12:56 | comment | added | MechMK1 |
@ChristianHackl Are you sure? XYZExceptWithProductionDataDueToRAMConstraints() seems like a convenient name.
|
|
| Feb 7, 2020 at 10:34 | comment | added | Frank Hopkins | @MontyHarder the scale of how often comments are the wrong way depends a bit on what you are used to. I've seen code bases where 100% of comments where utter rubbish, because they were outdated or just stated what the next line of code does ("//prints version number" over echo "$version" etc.). Given such code-bases, yes (almost) all comments that people came up with were just adding clutter or plain misleading and thus the code was better off without them. The cases where you need background information or a "why does this look like it's broken but isn't" should be low in most good projects. | |
| Feb 6, 2020 at 23:08 | comment | added | Monty Harder | I downvoted this for "almost always". I can agree that comments are often bad for maintainability, especially when done instead of giving variables and functions good self-documenting names. But "almost always" is a stretch. | |
| Feb 6, 2020 at 21:03 | comment | added | Logarr | @Graham - This is why part of my policy on code reviews is to not only look at the diff. The actual code needs to be looked at. It should become obvious that comments were not updated with the code. | |
| Feb 6, 2020 at 12:06 | comment | added | Christian Hackl |
@Baldrickk: Comments are even better for the why not. For example, // not calling XYZ() here because of it causes too much RAM usage with production data. You cannot simply replace that with a named function.
|
|
| Feb 6, 2020 at 10:50 | comment | added | Graham | @paul23 Much code is more practical to review than test too. And to continue the road sign analogy, do you believe the policeman was saying that there was no need for give way signs, stop signs, traffic lights, or directions to other places? For sure, in places where the code is utterly unambiguous, no comments are needed for those lines. It is not possible to write code where the entire program is unambiguously clear though, any more than it would be possible to drive from New York to LA just by "feeling who can go first". | |
| Feb 6, 2020 at 10:29 | comment | added | Baldrickk | the tl;dr of this is that comments are good for the why, and not the how | |
| Feb 6, 2020 at 7:47 | review | Suggested edits | |||
| Feb 6, 2020 at 11:03 | |||||
| Feb 6, 2020 at 3:56 | comment | added | paul23 | @Graham comments cannot be tested though. And to follow your road sign analogy: I was once taking a course on traffic management, and the police told me "well it's important you don't go against the flow, the road themselves without signs already let users feel who can go first etc". Code too shouldn't need comments. | |
| Feb 6, 2020 at 3:55 | comment | added | Jon Bentley | "I do prefer readable code and organized commits to comments in the code, simply because I've seen too many times where the comments aren't maintained" is equivalent to "I do prefer comments in the code to readable code and organized commits simply because I've seen too many times where the code is not readable" in the sense that they both make the mistake of thinking that there is a binary choice between readable code and maintained comments. As those above me have said, a good programmer should do both. | |
| Feb 6, 2020 at 3:05 | comment | added | Harper - Reinstate Monica | Generally, I do prefer readable code and organized commits (with good commit messages) to comments in the code well, if you had to choose one or the other, sure... but it seems like doing both is the programmer's job. Imagine a lawyer saying "I prefer filing motions on-time to showing up in court" um, it's literally your job to do both absolutely 100%. | |
| Feb 6, 2020 at 2:23 | comment | added | Graham | I've seen too many times where the comments aren't maintained as the code around them changes. Comments are code though. If the comment weren't changed, the code was not competently changed. It's like seeing someone drive into a road sign and saying "let's get rid of road signs". No, the answer is not to allow incompetent people to demolition-derby your code, because road signs are a good thing, and if they're screwing this up then they're screwing up everything else too. | |
| Feb 5, 2020 at 22:35 | vote | accept | Adam B | ||
| Feb 5, 2020 at 17:03 | history | answered | Thomas Owens♦ | CC BY-SA 4.0 |