Writing Well-Documented Code – Learn from Examples(codecatalog.org) |
Writing Well-Documented Code – Learn from Examples(codecatalog.org) |
I did write up a thing about it, but it's a long read, so folks don't usually read it: https://littlegreenviper.com/miscellany/leaving-a-legacy/
Has some pretty heavy-duty examples.
I thought I would miss it, but I don't miss the documentation. Code doesn't lie.
I can't believe you've got me justifying this policy. But it does seem to work. shrugs
x = 4 # Sets x to 4
Are of course silly and I'd laugh at them too, but comments when used correctly are hugely valuable.
Even a stale comment can be helpful in giving you a sense of what people were trying to do.
Do you not use commit messages either? Do you have any documentation at all?
Secondly, the implementation for the token bucket seems to just be bad. A simpler implementation is as follows:
- Input number of tokens and refill time.
- Output the constant state describing the limiter: the max number of tokens and the time to get a single token (= refill time / size, ignoring rounding issues which shouldn’t generally matter if your times are in nanos)
- The only mutable state is a single time instant: the time when the limiter becomes full
- to attempt to take a token, compute max(0,full_time - now) / new_token_time and if this is less than or equal to the size of the bucket minus the number of tokens you want, you may withdraw and set full_time := max(full_time, now) + tokens_to_withdraw * new_token_time
If that division is concerning then you should either attempt to withdraw from the rate limiter less frequently or you should not be using one at all.
The rest of the code is about scheduling events to happen when there are free tokens in the ratelimiter. This is a different problem and also over-complicated. You should either poll and retry periodically with your most important event (rather than whatever event you thought was most important when the bucket became empty) though you then want to be careful about stale tasks never running, or you should just have a queue of pairs (Task, cost) where cost is an int. after withdrawing tokens from the ratelimiter or when the queue transitions from empty to non-empty, peek the top item and set a timer for rl.full_time - rl.new_token_time * (rl.size - cost) and when the timer fires (or immediately if the time was in the past) you can dequeue, withdraw cost tokens, and run that top task (then repeat.)
The second one is ok, but please don't follow the first one as a good example, it takes away the ability to scan the code
IMO if at all, this type of comment ("Why was X designed this way") should go to the very bottom of the file (maybe with a very short comment at the top of the file referencing it), so it doesn't bother anyone who works on the code on a regular basis. And one could (should) also decrease its verbosity.
EDIT: Confused first and second example
I wonder if Protobuf wasn't open-sourced by Google, that comment would've been replaced with a link to a design doc. Google docs weren't a thing in early 2000s when Protobuf was originally written though.
So if that’s true redundant comments do hurt you.
Anecdotally in dynamic languages incorrect comments related to types seem to be a constant pain for me. They’re unverified and so a second source of truth that often wrong and can’t be trusted.
One of those habbits is to update documentation (tests/comments) when software is evolving.
Unfortunately it is a very rare habbit it seems.
These examples are a fucking nightmare.
i got an email about code i'd written 10 years ago...thanking me for taking the time to document everything.
Hmm, maybe I'm the odd one out but I actually find them quite annoying if they're longer than just a few lines.
In any case: People often just scroll past and ignore lengthy comments at the top (maybe precisely because they're used to copyright notices), so I think a short comment at the top like "For design considerations and reasons this code has been written the way it is, see the very bottom of this file" will attract much more attention.
> Also, I don't think I've ever seen comments at the bottom of a file!
True, but how often do you see code comments about design considerations in the first place?
> So if that’s true redundant comments do hurt you
Anyway, even if there was causation proven it might need to be weighed against other priorities.
I really dont want to read another developers comments on their mental state while I am feverishly debugging their code during a 3am page.
The number of times I have been misled by an outdated comment. I generally open git blame to see if the comments predate any code now. Tests on the other hand almost never lie.
Readmes or comments explaining why something exists vs what it allegedly does are generally much better. If you can’t write expressive code why would you assume your English is much better?
Having such a limited commenting syntax is like assuming one permanent type of footware and debating which one would be best. I'd much rather have a smarter commenting system.
I'm not talking about supplementary docs stashed elsewhere to gather dust. I'd like to have expandable comments right where they are needed but usable differently for a glance at familiar code or puzzling through unfamiliar code. Something very brief that doesn't clutter the code, a couple of words or no words at all, with an expansion arrow to something more detailed, expanding to something fully documented with links to everything.
And comments with scope (this comment applies to this line, this one to the function, this to a group of functions...) that actively use the scope. By active I mean that if you make any edits to code in the scope of a comment, that comment changes color or some other mechanism similar to a Git merge conflict: You changed the code without changing the comment, so check and make sure the comment says what you want and click OK. You don't have to fix the comments immediately for each edit. Make a few changes, test, make sure it's working, but when it is, you can see at a glance which comments need to be checked.
The general idea is instead of debating The Best Way to Comment, consider ways to improve the commenting system itself.
This is probably the major reason I much prefer introducing new variables and functions solely so I can give them descriptive names over using comments. Comments aren't scoped so they're often ambiguous and imprecise compared to code, which also contributes to them becoming out-of-sync with the code over time. Comments above a function definition are generally better than comments inside a function for example as it's clearer what the scope is.
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
In a vacuum I like them and when I bump into a piece of 10 year old code commented like this it is great.
There is one caveat however, they have to be meticulously maintained and always updated to reflect every change made to the code. It is a huge caveat because more often than not the comments are out dated and no longer reflect the actual code and at that point they are detrimental.
Because English is optimized for communicating human intent?
An example I’m currently working on is:
https://gerrit.libreoffice.org/c/core/+/121403/5/vcl/qa/cppu...
> // Returns None is x = 0
if x = 0 { return None }
I would agree that comments which unnecessarily describe what is to come are bad, in the same way that a novelist should prefer to show, not tell, unless there is a compelling reason for the reader to keep that description in mind as the immediate events unfold.
But to require almost all comments gone in favour of well-named code and external docs feels wrong too, in the same way that one would probably not like to read a book with no chapter headings, no visuals, simply describing the story in a dry (but informative) language, and relying on cliffnotes for actual context.
And if you say code is more like a technical report than a novel, I will disagree. Good code is always a pleasure to read, and typically for similar reasons that a good novel is pleasurable.
[0]: https://web.archive.org/web/20210226004600/http://antirez.co... (I still don't understand why he deleted his blog)
// We still have burst budget for *all* tokens requests.
if self.one_time_burst >= tokens {
...
} else {
// We still have burst budget for *some* of the tokens requests.
enoughBudgetForAllTokenRequests = self.one_time_burst >= tokens
if enoughBudgetForAllTokenRequests
...
else
...
I think a descriptive variable makes the code easier to skim, it's more precise and less likely to become inaccurate. For a big complex conditional, you can break in up into several well-named boolean variables you compose together (but obviously comment the bits that can't be covered by a concise variable name).
https://devblogs.microsoft.com/typescript/announcing-typescr...
The top of the file comment is good and well received. At the same time, you could have a wiki page with that info, where you can also have diagrams etc.
These days I write my code as comments first and then write the code.
But for example this one
// If either token bucket capacity or refill time is 0, disable limiting.
if size == 0 || complete_refill_time_ms == 0 {
return None;
}
This one is great, though at first glance I cant relate the actual code to the comment, at least I understand why the code is there.
// We still have burst budget for *some* of the tokens requests.
// The tokens left unfulfilled will be consumed from current `self.budget`.
tokens -= self.one_time_burst;
self.one_time_burst = 0;Also, one thing I think doesn't get mentioned enough is the use of comments as anchors for text search. Having "disable limiting" expressed in English that way makes it easier for newcomers to explore the codebase.
You hit the nail on the head here. This is the single most important reason for having comments in code (aside from just being a part of a coding standard).
> self-indulgent performative trash
A little harsher than I would have used; but now that you say it.
>that should be rejected in review.
Without question. I would have absolutely denied that until it was trimmed to at most 10% of what was there.
I was working on an unfamiliar codebase yesterday and had to work with the postMessage function. It was React, so one might assume that it sends the message to the backend, which was my initial assumption. Nope. It posted it to the page. Didn't help that another utility had another postMessage that was about what to do after the message was sent.
The takeway is that merely writing very long descriptive names isn’t a silver bullet and could cause harm despite good intentions.
I suggest going back a after a few days for a refactoring (renaming functions or variables) and see how it reads to you. Then repeat the process a few days later when you’re even more detached from that code.
In many cases self documenting code is going to help you remember the model you had in your head when building it. And if done well it will help others as well, your code will end up not giving headaches to future maintainers.
Even something simple like “if the function starts with “is” it will return a Boolean has helped me organize things that return status codes and have side effects. Small thing, but I like it.
Edit: Hah. It does.
> if self.one_time_burst > 0 {
What is this code doing? It's setting up a flow control statement. It's dereferencing the "one_time_burst" property on the object self and comparing the value to 0. If the value is greater than 0, it will execute the proceeding code block.
Why is this being done?
> [to] consume the one-time-burst budget.
Self-explanatory code can't explain themselves if they do not exist
* Common name of the algorithm you're using
* Reference to somewhere else in the code that's related
* "You may think this code only does X, but it really also does Y"
* "We used to also do XYZ here, but that was removed because of ABC"
and so on.
I’ve similarly been paying much more attention to convey INTENT which I think is massively missing from computer science. The code might suck, but if the intent is clearly defined, it helps to refactor later.
Well I stopped reading further.
That's what works for me.
If all your comment does is say what the code on the next line is doing, don't. Instead just try to make the next line more readable.
For instance in the firecracker example, they don’t write 3 pages of block comments to explain the token bucket algorithm (or do they?), as anyone could go read the wikipedia article instead.
Comments living with the code have their benefits, but here I can’t imagine these huge comment blocks or algorithm related warnings get revised anytime outside of project wide refactoring.
What we also miss by having these comments inlined is the higher perspective of how it works with the other classes or how it fits in their running context. Anything that spans multiple class and/or methods makes it difficult to decide where to put the info and what scope it should have. People usually end up explaining the same things at 3 or 4 places in sublty different ways, which makes no one happy.
An exemple of that, this struct description
> /// TokenBucket provides a lower level interface to rate limiting with a /// configurable capacity, refill-rate and initial burst.
This comment is hinting at a sequence where ‘TokenBucket’ needs to be a configurable lower level interface. But what is that sequence, what needs it to be lower level, is there higher level interfaces ? I’ll sure end up spelunking the rest of the doc to get answers to that.
Keeping documentation in another document would see this going out of sync over time until the document doesn't at all reflect how the code looks. Not updating anything at all will leave others clueless of why the change was needed 4 months later and might see the other classes grow to the same size. Thus, eventually the code base will once again be opaque unless you ask the nearest person that worked on it last.
A nicer way of handling the flow of the document would be to use a literate programming tool that allows for keeping code snippets in an order which makes sense for the documentation rather than sprinkling as comments between the code. That way, code review happens at the same time as documentation review with code being local to the text documenting it in a format which is easy to understand for both.
Comments tend to just become a place for misinformation or get disconnected from the actual logic.
Adding more comments sometimes doesn't clarify the situation, it just acts as a second source of truth.
Rather.. I think part of the point is, context matters. If the team is small, and the project is changing rapidly.. I think the needs and expectations for documentation are different than if the project or team is large and change is slow.
Here's the problem: "Experts" (or "veterans" as the author calls them) are not created equal: There's the expert that already knows the code because she's worked on it countless times and already has a very detailed mental model of the code and there's the expert who's a very experienced programmer but has never seen the code before.
Even experts of the latter category appreciate good comments when they read code (surely not comments of the "n00b" kind portrayed in the article but comments nonetheless).
Want well documented (and tested and thought out) code?
Then give the coders people outside of their team / hierarchy who are going to check it / read it / use it.
And then give them time.
And repeat.
What would be even better is to create a model of the system using something externally altogether, like TLA+. It need not be perfect right at the beginning, but over time, invariants can be put in place, and even possibly mechanically tested by a model checker. Just even the Next state disjunct would work great - it'll give me the superset of actions the system is capable of.
To me, well-commented functions are all well and good; but without a high level understanding of how everything fits and works together, I doubt it'd be very helpful. It's like winning the battle but losing the war.
I'm not a fan of relying on IDE-centric features either. What if my IDE is not yours?!
// Hit the bucket bottom, let's auto-replenish and try again.
self.auto_replenish();
All of the comments in reduce are redundant, this is what I would write at the top, so that anyone using the method doesn't have to page through to figure out what it does, it also makes the "smells" more obvious:
// reduce: attempt to consume tokens from bucket,
// starting with one_time_burst, overflowing to
// budget.
// (keep comment only if _expensive_):Performs
// auto-replenish when
// burst is emptied, and budget has insufficient
// tokens.
// Asserts size, but only on replenishment.
// On OverConsumption, entire bucket is emptied.
// On Failure, only one_time_burst is emptied.
// In all cases, tokens is reduced by amount
// fulfilled.
I'm also curious why burst is consumed, then budget. I would expect _budget_ to be consumed first (with refill) with overflow into burst? My expectation is for burst and budget to have different refill schedules in auto_replenish, so using burst first would result in more failures by missing refill opportunities.
I'm also curious why the size assertion is only on requests that result in refills. Is it guaranteed that size = max(one_time_burst + budget)? Why care about the difference? Is hogging the tokens "bad"?
Then, I'd work to remove the need for those comments - particularly the need to explain what happens on error - which should be "nothing", or that the bucket is emptied of whatever is available, not something in between.
Finally I'd remove the mutation of tokens, returning either unfulfilled or fulfilled in the return value.
Comments are not the first tool people should try to reach because:
1. That's a lazy way to make the code clear, most code should be self-descriptive. If the teams are spamming comments it's likely to be a bad code base already.
2. There's no guarantee the comment is compatible with the code: It's natural language and there's no obvious way to test that. After versions of modification, the comments might still look like they make sense but that's not how the code work anymore.
Two alternatives:
1. Declarative programming: Try to leverage declarative programming styles more. For example, top-down style dynamic programming or memoization is always more readable and less error-prone than the bottom-up ones. You don't have to fully adopt functional programming or logic programming, but do take some inspiration from them.
2. Testing. Tests are like comments, they reflect some meaning of the code. Tests are not like comments, if the code breaks, they break.
Of course, comments are very valuable, but it's most valuable when applies in the right spots only.
There's a ton of comments in every example (even though they might be redundant across the examples) and the idea is that you should be able to learn this new framework by merely reading through the examples.
I get the whole document with your code, and practice that. But sometimes we end up with variable names that get very long and start leading not the most aesthetic code that gets formatted in a very vertical way.
Don't know the right answer, but suspect the two extreme views are not correct.
I've found that hence e.g. for rather complex statements where the code isn't lacking information, not commenting is the correct approach.
Whereas sometimes, when I'm forced to write exceptional code because of e.g. a workaround, a comment can nicely introduce my future self and other readers to the context.
Comments are not monologues. Explain what the line you are commenting does, and perhaps elaborate why. But always acknoweledge: more words = more patience required = more maintenance cost.
Also, nobody will care about what your name is in 2 years. Do not make the code about you.
It's quite a bit easier to write something the compiler or runtime can understand, there's no need for meaningful names, consistent structure, or conceptual coherence. As long as it builds and passes whatever tests, the computer will accept it.
antirez provides several examples where the code without the comments would be much harder to understand and/or maintain. Whenever this is the case, I would argue that comments are a must and without them the code is at least barely intelligible.
The HTTP version of your URL works fine. The HTTPS version has an expired cert. If I click past that, it redirects to redis.io, then gives a 404.
I know antirez is a smart guy, but if this is some kind of "I'm taking a bold stance against HTTPS" then I'd rather read the archive.org link with functioning HTTPS. My threat model prefers trusting archive.org to correctly reproduce the blog, over trusting everyone between me and antirez.com.
> Sorry, I can't find that page :-/
for several months
I agree at a surface level, however:
- this makes us assume that there are tests in the first place
- this makes us assume that someone will read these tests instead of asking the developer
- this makes us assume that these tests will be readable enough on their own to replace free form text
- this makes us assume that these tests will even be able to contain a representation of why the code exists
- personally, i have never seen a test that explains a business requirement with enough context not to miss out on details
- i have also never seen code that encapsulates these requirements 1:1, without getting too mixed up in implementation details
Doing anything less feels like giving ourselves constraints that will lead to suboptimal results because the tooling and testing solutions aren't quite there yet. Maybe when frameworks and languages will become more expressive, we'll be able to live without comments. However, that day may not come anytime soon.
Nor will companies be interested in the overhead that writing extensive and good tests entail, nor will they want to wait for their engineers to figure out how to convey everything in computer code, as opposed to just dropping some comments into the codebase, right next to the actual code that they concern. Maybe when companies are ready for 50% of the development time to be spent on testing software, that won't be an issue. That day may also not come anytime soon.
As a possible counterpoint, look at RSpec, i think it's headed in the right direction: https://rspec.info/
I was going to say "So for performance "hacks" there should be a clean implementation that's benchmark against the current implementation for example?" as a way to disprove what you said, but while writing it I realized that it may actually be a pretty good idea.
You've just written some messy unclear code for performance reasons, so it deserves more thorough tests than average.
And you've already written the clean version anyway, since you hopefully didn't write the optimized version until after you profiled the slow one. So it's not even significant extra work to turn that into your test suite.
I've changed BucketReduction to be a pair, and fixed the strange behavior on error. Swapping "fromBurst" and "auto_replenish" would reduce the number of throttling events. This solution does have more branches than the original, hence readability vs performance. It's still not exception safe, auto_replenish could throw and leave the state undefined.
BucketReduction reduce(long tokens) {
long fromBurst = min(tokens, burst);
burst -= fromBurst;
tokens -= fromBurst;
if (tokens > budget) {
auto_replenish();
}
long fromBudget = min(tokens, budget);
budget -= fromBudget;
tokens -= fromBudget;
long totalConsumed = fromBurst + fromBudget;
if (tokens == 0) {
return new BucketReduction(Success, totalConsumed);
}
if (totalConsumed + tokens > size) {
return new BucketReduction(OverConsumption, totalConsumed);
}
return new BucketReduction(Failure, totalConsumed);
}I don't find the comments in the original code distracting, but I do like your version better.
> I'm also curious why burst is consumed, then budget. I would expect _budget_ to be consumed first (with refill) with overflow into burst? My expectation is for burst and budget to have different refill schedules in auto_replenish, so using burst first would result in more failures by missing refill opportunities.
This behavior is documented in the public API [0], so whatever is the reason why it was chosen, I don't think it can ever be changed.
> I don't understand why OverConsumption is different to Failure. Both will result in throttling by the caller. The reason for the difference should be documented.
My understanding is this. If the number of tokens requested is greater than the remaining budget but less than the size of the bucket, the call is rejected and the caller is blocked until it has enough tokens. But if the number of requested tokens is greater than the size of the bucket, the caller will never have enough tokens. Instead of blocking the caller forever, the rate limiter lets the call go through, but then blocks the caller for a while to compensate for the over-consumption. Here's the handling [1]. I wish it was documented better.
[0] https://github.com/firecracker-microvm/firecracker/blob/fc2e...
[1] https://github.com/firecracker-microvm/firecracker/blob/2f92...
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.
On the other hand, token bucket implementations are hard to understand and hard to debug. And an ambiguous variable name can cause trouble because some devs might think it "obviously" refers to enough budget for some other slice of requests.
I think my preferred solution here is to extract the condition into a boolean function with a long name like HasBudgetForAllRequests. This solves the long variable name issue and also gives you a useful place to add to your budget computation logic if you need to.
Think of code as mathematical notation. Some degree of familiarity with the domain is expected. You can use shorthand symbols or words as variable names.
Otherwise you're trying to write English sentences into compact notation and that's just noise. Hard to read.
Try read the A* algorithm when the variables have all been expanded out and you'll find it's impossible to see the forest for the trees. Sometimes G is the correct name for the collection of the cheapest scores known so far. It's compact and is best explained with a comment.
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
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.
But separately, if you want to add a performance benchmarking process (great!), you have a reference baseline implementation for free.
Isn't the answer to that obvious? The code is significantly constrained in form by having to be executable. The comments have no such restriction.
> You may think that writing comments is a lesser noble form of work. After all you can code! However consider this: code is a set of statement and function calls, or whatever your programming paradigm is. Sometimes such statements do not make much sense, honestly, if the code is not good. Comments require always to have some design process ongoing, and to understand the code you are writing in a deeper sense. On top of that, in order to write good comments, you have to develop your writing skills. The same writing skills will assist you writing emails, documentation, design documents, blog posts, and commit messages.
In any case, you're making a good point, though I would argue that the reverse of the saying (with the modification that "expectations" get replaced with "trust") also holds:
If someone is not able to write intelligible, stringent English, I wouldn't trust them to write good code, either, because writing good code requires similar qualities (good stringent structure, empathizing with the future reader of your code etc.) and, as antirez argues convincingly, it also requires writing comments (which are in plain English).
The comments don't say the same thing as the code. In the comments, the author might tell us - albeit not clearly or in proper English, what they want to achieve, or why they're doing it this way.
[1]: https://twitter.com/KevlinHenney/status/381021802941906944
The only time I ever feel the need to leave comments is to explain code that doesn't appear to be needed or isn't obvious why it exists. Usually this comes from agreed upon technical debt, short notice requirements with tight deadlines, or some other form of tradeoff.
"We're doing a simple check and throwing an exception here since we have work to introduce a more comprehensive permission system in xyz epic"
or something like
"There's a complicated race condition in xyz so we're doing a simple sleep here since the proper fix involves introducing a distributed lock"
Sure those types of things are cases you want to avoid but unfortunately you occasionally end up there anyway and leaving a short explanation about a compromise is better than ambiguous code that appears to serve no purpose
If you run this command you can see a little more clearly what’s going on:
wget --no-check-certificate -S -O - https://antirez.com/news/124
If you try https://antirez.com/news/124 the web server at antirez.com:443 will answer this:
HTTP/1.1 301 Moved Permanently
Server: nginx/1.10.2
Date: Sat, 04 Sep 2021 18:15:46 GMT
Content-Type: text/html
Content-Length: 185
Connection: keep-alive
Location: https://redis.io/news/124
However, many clients won’t heed this response, since antirez.com:443 serves a certificate that’s not valid for that hostname.
I certainly wouldn't trust Comcast to keep malicious ads out of their ad network, either.
[0] https://www.techdirt.com/articles/20140908/07191228453/comca...