I think there’s room for nuance in all of these, and they’re not clear-cut refactoring wins.
[for viewDidLoad] Which version is easier to read? My vote goes for the second one - it’s easier to read as the method is now shorter - and easier to refactor, as we no longer have to maintain the comment when changing the code.
The second version of viewDidLoad is easier to read… but I’d argue it’s harder to understand! In the first version, the logic was all in one place. In the second version, it’s in three different places. I have to keep jumping between viewDidLoad, setupNavigationBar, andsetupTableView to get the whole picture. We also have mutations in three different places, which makes tracking the effects harder too.
Writing the function also means other things can use this, too. So do other functions call setupNavigationBar? In the first version, I knew that its logic only happened in one place of the code. Now I don’t anymore. If I modify it, it might inadvertently break a caller I wasn’t paying attention to.
[for willPresentSearchController] The readability of the code is just as good - and the bugfix code is extracted in its own logical item, a method. It’s fine to keep the comment in place - but now it’s obvious that the bugfix method does one thing, providing the workaround.
The original code also did only one thing, and didn’t do anything but the workaround. It has the same understandability issues. More to the point, though, is the name of the new method: preventSearchBarFromDisappearingWhenSearchControllerIsActive. That’s 62 letters long. It’s much harder to read a comment than ten words without spaces.
Finally, in the original method, you’ve remove the why and only left the what. I have to investigate that method to find out it’s only on iOS9. That increases the chances it becomes code rot.
[for setupNativeCalendarAccess] There should be no reason to keep the code in place - this is exactly what source control is there for - if someone will need the original code, they can find it in the commit logs.
That requires them to know it’s in the commit logs. If it’s in the code, everybody knows it’s there, and nobody’s going to forget about it.
The thing with this example is you have a clear scope: uncomment the code when the calendar functionality is removed. If you’re worried about forgetting it, place a timestamp on it and a calendar task to check it in a month or so. Signing and timestamping comments is a good practice, anyway.
Writing the function also means other things can use this, too. So do other functions call setupNavigationBar?
This why I don’t apply this kind of transformation without having a good reason. Getting rid of a comment isn’t a good reason. Making the function more navigable and readable is a better reason (sometimes that logic can get hairy). In the example provided by the OP I am unmoved. When I do feel the need to refactor this way, I make it clear the new function is subservient to the first; scoped inside is the best, if possible.
preventSearchBarFromDisappearingWhenSearchControllerIsActive. That’s 62 letters long. It’s much harder to read a comment than ten words without spaces.
That’s not even the entire problem. Making it a separate method opens up that particular method to be used outside of the context of when it’s appropriate.
“Comment” is a horrible overloaded term. If you can express something in code, it probably should be expressed in code directly. Other than that, the common ones I’m ok with:
“usage” (doctests or examples which are extracted from comments and then compiled automatically).
“justifications” (why it was done this way, such as a weird corner case, a performance reason, or some non-obvious interactions such as with a third party library or other threads or processes)
“annotations” (such as indicating start/end points of deviation from an existing, open source or licensed code base under separate version control not under your control where changes will need to be reintegrated)
This one’s tricky to get right, but tends to be ok if things around your system are fixed (such as your program is receiving/sending to outside vendors and how your system fits it with everything):
“design” (high level description of the system, or entire subsystems).
To add to that list, and using the strange word choices of the OP, sometimes it’s just not practical to write code that your can’t help “apologizing” for. Sometimes the problem you’re solving (especially if it’s performance sensitive) is just plain complex, or perhaps you just don’t have the skill or experience to make the code simpler (happens to me all the time). There’s no real reason to feel bad about it and “apologize.” Instead, do the best you can to document the code and give the next person who comes across your code the best chance to succeed that you can. All this nonsense about comments being bad is just absolutely ridiculous.
Sure, but sometimes the refactoring required is far larger than can be performed at the comment site.
Refactoring away this comment required rewriting large chunks of PyOpenSSL in pure Python (in Cryptography, too, so a change of language and of library API was required.) Along similar lines, refactoring away this comment probably requires destroying Adobe and erasing their proprietary legacy from the planet.
My own humble contribution here will be this comment, which tracks a particular exception thrown by the RPython JIT compiler. This comment is colocated with the code which we control in the exception’s stacktrace. It advises maintainers on the nature of the magic number 42 on the following line. I think that refactoring away this comment will require leaving RPython, since the underlying limitation stems (IIRC) from a hard limitation of 255 constants in a byte-indexed array.
One thing I never see in this “never comment anything” type articles is what I have encountered at work—having to work around bugs in third party libraries that I have no source code to access. Or deficiencies in an API (some of the POSIX semantics for instance). An example from one of my programs:
/*-----------------------------------------------
; Do NOT set SA_RESTART! This will cause the program
; to fail in mysterious ways. I've tried the "restart
; system calls" method and it doesn't work for this
; program. So please don't bother adding it.
;
; You have been warned ...
;-----------------------------------------------*/
Almost all inline comments are ones that could be removed or moved while making the code more readable and easier to refactor.
I’m in agreement with @hwayne here: there’s usually a lot of nuance to take into account. Boiling it down to this and applying it as such is, to me, unwise.
As an example, here’s some code from GCC that I’ve been digging in around lately.
For GCC, this is pretty new code (and easier to work through than, say, the infamous reload). In the linked function I’d say all the inline comments are helpful and there is no need at all to remove them. In fact, the whole file is pretty good.
Refactoring to get rid of inline comments seems to me to be a very weak reason to refactor. There may be a case for it when the code is young, but when it settles, I think that kind of action is disruption for the sake of disruption.
I strongly suspect that these arguments would not be supported by the empirical software engineering literature.
My personal spit-take on this (also not supported by the literature) is that the main issue for comprehensibility in these examples is the confusing state management with what seems to be a retained mode GUI and the use of procedures rather than good, honest-to-dog functions.
But perhaps it isn’t empirical enough for your taste? I haven’t read it. My point was more “there’s no evidence for this” rather than “there is evidence against this”
And claims about short functions have been evaluated, IIRC.
OTOH, I am being a bit unfair, obviously there are more sources of knowledge than academia. I guess I’d just prefer that these blogs not act so authoritative and instead make clear that these are preferences, not universal truths.
Even if we assume the premise of “a comment is an apology for bad code” is correct (I don’t think it is, for reasons others have described, but let’s run with it), then I still don’t agree with “a comment is an invitation for refactoring”.
Working code works. I’ve seen many bugs be introduces because of refactors, and even if it didn’t introduce a bug it’s still a factor that needs to be looked at. It’s also time-consuming, and often not a good ROI.
Refactors should be “evidence-based”: make a clearly-defined improvement, and not just “I think it looks better this way”. Code is about solving real problems, and is not an art project.
There should be no reason to keep the code in place - this is exactly what source control is there for - if someone will need the original code, they can find it in the commit logs.
Hardly any comment about commenting code makes me as furious as this. Most of the time exactly those people who won’t remember what was done 3 weeks ago will use it and never check. Or it’s the people with eidetic memory who can recite the order of commits for the file from last April. Nobody will ever check the history except if there’s a good reason.
Overall, I think this could be summed up by “comment on why, not how”. These comments are almost never superfluous.
Also I think it’s been years since I had outdated comments or too many. “No comments at all” is much more common. And that doesn’t mean that the code is self-explanatory, just badly documented.
That’s a non-sequitur. Depending on the method, I find it easier to read when the details are all inline and I don’t have to jump around the file to understand what’s happening. If the sub-step is complicated, then OK, factor it out — ideally into a nested function so it’s obvious nothing else calls it, if your language can do that — but a three-line method like this does not qualify.
In this case, I’d just delete those comments and leave the code alone. Three consecutive lines setting properties of a table view need neither a “set up the table view” comment nor a method. Just a code paragraph is fine.
I think there’s room for nuance in all of these, and they’re not clear-cut refactoring wins.
The second version of
viewDidLoadis easier to read… but I’d argue it’s harder to understand! In the first version, the logic was all in one place. In the second version, it’s in three different places. I have to keep jumping betweenviewDidLoad,setupNavigationBar, andsetupTableViewto get the whole picture. We also have mutations in three different places, which makes tracking the effects harder too.Writing the function also means other things can use this, too. So do other functions call
setupNavigationBar? In the first version, I knew that its logic only happened in one place of the code. Now I don’t anymore. If I modify it, it might inadvertently break a caller I wasn’t paying attention to.The original code also did only one thing, and didn’t do anything but the workaround. It has the same understandability issues. More to the point, though, is the name of the new method:
preventSearchBarFromDisappearingWhenSearchControllerIsActive. That’s 62 letters long. It’s much harder to read a comment than ten words without spaces.Finally, in the original method, you’ve remove the why and only left the what. I have to investigate that method to find out it’s only on iOS9. That increases the chances it becomes code rot.
That requires them to know it’s in the commit logs. If it’s in the code, everybody knows it’s there, and nobody’s going to forget about it.
The thing with this example is you have a clear scope: uncomment the code when the calendar functionality is removed. If you’re worried about forgetting it, place a timestamp on it and a calendar task to check it in a month or so. Signing and timestamping comments is a good practice, anyway.
This why I don’t apply this kind of transformation without having a good reason. Getting rid of a comment isn’t a good reason. Making the function more navigable and readable is a better reason (sometimes that logic can get hairy). In the example provided by the OP I am unmoved. When I do feel the need to refactor this way, I make it clear the new function is subservient to the first; scoped inside is the best, if possible.
That’s not even the entire problem. Making it a separate method opens up that particular method to be used outside of the context of when it’s appropriate.
“Comment” is a horrible overloaded term. If you can express something in code, it probably should be expressed in code directly. Other than that, the common ones I’m ok with:
This one’s tricky to get right, but tends to be ok if things around your system are fixed (such as your program is receiving/sending to outside vendors and how your system fits it with everything):
To add to that list, and using the strange word choices of the OP, sometimes it’s just not practical to write code that your can’t help “apologizing” for. Sometimes the problem you’re solving (especially if it’s performance sensitive) is just plain complex, or perhaps you just don’t have the skill or experience to make the code simpler (happens to me all the time). There’s no real reason to feel bad about it and “apologize.” Instead, do the best you can to document the code and give the next person who comes across your code the best chance to succeed that you can. All this nonsense about comments being bad is just absolutely ridiculous.
Sure, but sometimes the refactoring required is far larger than can be performed at the comment site.
Refactoring away this comment required rewriting large chunks of PyOpenSSL in pure Python (in Cryptography, too, so a change of language and of library API was required.) Along similar lines, refactoring away this comment probably requires destroying Adobe and erasing their proprietary legacy from the planet.
My own humble contribution here will be this comment, which tracks a particular exception thrown by the RPython JIT compiler. This comment is colocated with the code which we control in the exception’s stacktrace. It advises maintainers on the nature of the magic number 42 on the following line. I think that refactoring away this comment will require leaving RPython, since the underlying limitation stems (IIRC) from a hard limitation of 255 constants in a byte-indexed array.
One thing I never see in this “never comment anything” type articles is what I have encountered at work—having to work around bugs in third party libraries that I have no source code to access. Or deficiencies in an API (some of the POSIX semantics for instance). An example from one of my programs:
Yeah, refactor that …
I’m in agreement with @hwayne here: there’s usually a lot of nuance to take into account. Boiling it down to this and applying it as such is, to me, unwise.
As an example, here’s some code from GCC that I’ve been digging in around lately.
https://github.com/gcc-mirror/gcc/blob/master/gcc/lto-streamer-out.c#L410
For GCC, this is pretty new code (and easier to work through than, say, the infamous reload). In the linked function I’d say all the inline comments are helpful and there is no need at all to remove them. In fact, the whole file is pretty good.
Refactoring to get rid of inline comments seems to me to be a very weak reason to refactor. There may be a case for it when the code is young, but when it settles, I think that kind of action is disruption for the sake of disruption.
I strongly suspect that these arguments would not be supported by the empirical software engineering literature.
My personal spit-take on this (also not supported by the literature) is that the main issue for comprehensibility in these examples is the confusing state management with what seems to be a retained mode GUI and the use of procedures rather than good, honest-to-dog functions.
You’d think so, but I’ve had a surprisingly hard time finding any empirical research on comments at all.
There’s some: https://scholar.google.co.uk/scholar?q=source+code+comments
But perhaps it isn’t empirical enough for your taste? I haven’t read it. My point was more “there’s no evidence for this” rather than “there is evidence against this”
And claims about short functions have been evaluated, IIRC.
OTOH, I am being a bit unfair, obviously there are more sources of knowledge than academia. I guess I’d just prefer that these blogs not act so authoritative and instead make clear that these are preferences, not universal truths.
Even if we assume the premise of “a comment is an apology for bad code” is correct (I don’t think it is, for reasons others have described, but let’s run with it), then I still don’t agree with “a comment is an invitation for refactoring”.
Working code works. I’ve seen many bugs be introduces because of refactors, and even if it didn’t introduce a bug it’s still a factor that needs to be looked at. It’s also time-consuming, and often not a good ROI.
Refactors should be “evidence-based”: make a clearly-defined improvement, and not just “I think it looks better this way”. Code is about solving real problems, and is not an art project.
Hardly any comment about commenting code makes me as furious as this. Most of the time exactly those people who won’t remember what was done 3 weeks ago will use it and never check. Or it’s the people with eidetic memory who can recite the order of commits for the file from last April. Nobody will ever check the history except if there’s a good reason.
Overall, I think this could be summed up by “comment on why, not how”. These comments are almost never superfluous.
Also I think it’s been years since I had outdated comments or too many. “No comments at all” is much more common. And that doesn’t mean that the code is self-explanatory, just badly documented.
That’s a non-sequitur. Depending on the method, I find it easier to read when the details are all inline and I don’t have to jump around the file to understand what’s happening. If the sub-step is complicated, then OK, factor it out — ideally into a nested function so it’s obvious nothing else calls it, if your language can do that — but a three-line method like this does not qualify.
In this case, I’d just delete those comments and leave the code alone. Three consecutive lines setting properties of a table view need neither a “set up the table view” comment nor a method. Just a code paragraph is fine.