6 ms·
Documentation is essential. How things work is an important thing to document. Ideally it should be in version control and be generated from the code,
by JohnBooty 7y ago
Documentation is essential. How things work is an important thing to
document. Ideally it should be in version control and be generated
from the code, because then it's less likely to go out of date.
My solution to this is old and fairly unpopular, but I stand by it: anything in the codebase that's not obvious to a new maintainer should have a brief, explanatory code comment.
Generally, this falls into two categories.
1. Hacks/kludges to get around bugs in hardware, external services, or included libraries. These manifest in code as incomprehensible, ugly bits of code that are difficult to distinguish from code that is simply "sloppy" or uninformed. More importantly, they represent hard-won knowledge. It often takes many programmer-hours to discover that knowledge, and therefore many dollars. Why throw it away? (Tip: include the version of the dependency in the comment, ie)
# work around bug in libfoo 2.3, see blahblahblah.com/issues/libfoo/48987 for info
# should go away once we can upgrade to libfoo 3..
if error_code == 42 reset_buffer()
...so that future programmers (including you) can more easily judge whether the kludge is still needed in the future.
2. Business logic. This too is difficult/impossible to discern from looking at code. Often, one's git commit history is sufficient. But there are any number of scenarios where version control history can become divorced from the code, or require a fair bit of git/hg/svn/whatever spelunking to access. And this of course becomes increasingly onerous as a module grows. If there are 200 lines of code in a given module, it is a significant time investment to go git spelunking for the origins of all 200 lines of code. Some concise internal documentation in the form of code comments can save an order of magnitude or two of effort.
It still has problems (What do you do when the code and the
documentation disagree? Which is correct?), but they're not as
severe as the problems that arise when there is no documentation at all.
This is pretty easy to enforce at code review time, prior to merging.
In the first place, only a true maniac would intentionally update
# no sales tax in Kerplakistan on Mondays
return nil if country_code==56 and day_of_week==1
...without updating the associated comment. If they do neglect to update it, that's an easy catch at review time.
- irishsultan 7y agoWhile I still would add a comment about the why, your last bit of code probably should be written without magic constants. # Some countries have sales tax rules dependent on the day of the week return nil if country_code==KERPLAKISTAN and day_of_week==MONDAY The exact comment here could probably be more specific (e.g. where do you find these rules), but it also most likely shouldn't repeat the code (and the code should make clear what it represents).
- mikekchar 7y agoIf you do the substitution as you suggest and then add a unit test, then you have something ;-) Something on the lines of "describe countries with sales tax dependent upon the days of the week => Kerplakistan doesn't have sales tax on Mondays" So now it's self documented and self testing. But I agree with your statement that there should be a pointer to the business rules somewhere. Otherwise it's difficult to have a meeting with the business side and ask, "Has anything here changed?" I think that's the biggest thing people miss out -- It's not that hard to find the thing in the code if things change. It's super hard to make sure you are on top of all the business requirement changes.
- arethuza 7y agoBut don't do what one memorably awful project I had to maintain did - to use that example they would have done: country_code==FIFTY_FIVE and day_of_week==ONE
- deleted 7y ago[deleted]
- spuz 7y agoBut what if the definition of 55 changes? You'll be glad to have your table of constants then.
- arethuza 7y agoThe project also defined HTTP, COLON, SLASH, WWW and DOT so that you would have: string url = HTTP + COLON + SLASH + SLASH + WWW + DOT ... I swear I'm not making this up....
- rpmisms 7y agoSounds like a PHP codebase I'm currently working in. I shit you not, $LI = '<li>' is in the functions file, along with $LI_END.
- hnick 7y agoCount me in as another old timer who agrees. I had a friend once throw the "code should be self-documenting" line at me once and it upsets me. That only really applies for code that is so simple it writes itself, and never has any gotchas hiding (and which useful project is like that?). Leaning towards commenting "why" not "what" is another good general rule. "Self-documenting code" with sensible function and variables names and logical flow already cover the "what" fairly well.
- jbverschoor 7y agoYup. This + some diagrams for models and infrastructure is plenty
- 0xffff2 7y ago>anything in the codebase that's not obvious to a new maintainer should have a brief, explanatory code comment I'm not at all convinced that this is unpopular, but I think it's a whole lot harder than you're letting on. Unless you have a constant stream of new people coming in and you can convince them to give honest feedback, you don't actually know what's not obvious.
- base698 7y agoWhy not: return nil if country_code==KERPLAKISTAN and day_of_week==MONDAY Then you don't need comments and the sync problem goes away?
- TeMPOraL 7y agoExcept this doesn't retain the crucial information: why? It looks arbitrary. The thought that "some countries have sales tax rules dependent on the day of the week" may or may not be obvious from the context. At the very least, the comment pins a point in the space of all possible reasons for that piece of code - with it, you know it's related to sales tax and week days, and isn't e.g. a workaround for the bug with NaNs in tax rates that you saw on the issue tracker last week.
- JohnBooty 7y agoThis is admittedly a trivial example, but ideally you want developers who understand why we're doing this. Is this a quick thing somebody hacked in for a special, one-off, tax-free month in Kerplakistan as the country celebrates the birth of a princess? Is this a permanent thing? Will there eventually be more weirdo tax rules for this country? Will there be others for other countries? Knowing the "why" would help a developer understand the business, and reason about how best to work with this bit of code... should we just leave this ugly little special case in place? Should we have a more robust, extracted tax code module, etc.? Commit messages help to accomplish this too, and can offer richer context than inline comments. Each has their place. Sifting through hundreds of commit messages in a frequently-updated module is not a great way to learn about the current state of the module, as the majority of those commit messages may well be utterly stale. Ultimately the cost of having some concise inline comments is rather low, and the potential payoff is very large. Remember that the longer term goal (besides the success of the business) of software is to have your developers gain institutional knowledge so that they can make more informed engineering decisions in the future.
- collyw 7y agoI agree. The most obvious place to find an explanation of a piece of code, is right beside that code. Not hidden away in some git commit message or nested away in confluence.