| ▲ | jackfranklyn 4 days ago |
| The "what" vs "why" distinction breaks down when your code encodes domain knowledge that readers can't infer from context. I build accounting software and half my "what" comments are actually explaining business rules that would be impenetrable otherwise. Something like: // Bank transfers appear as two transactions - a debit here and credit elsewhere
// We match them by looking for equal-opposite amounts within a 3-day window
That's explaining "what" but also implicitly "why" - because that's how double-entry works and that's the tolerance banks allow for settlement delays. You can't really extract that into a method name without it becoming absurd.The Uncle Bob approach of extractNameMatchingTransfersWithinSettlementWindow() doesn't actually help - now I need to know what settlement windows are anyway, and I've lost the context of why 3 days. |
|
| ▲ | tetha 4 days ago | parent | next [-] |
| > That's explaining "what" but also implicitly "why" - because that's how double-entry works and that's the tolerance banks allow for settlement delays. You can't really extract that into a method name without it becoming absurd. That's why I've also started to explicitly decompose constants if possible. Something like `ageAlertThresholdHours = backupIntervalHours + approxBackupDurationHours + wiggleRoomHours`. Sure, this takes 4 constants and is longer to type than "28 hours". However, it communicates how I think about this, how these 28 hours come about and how to tweak them if the alerting is being annoying: It's easy to guess that you'd bump that to 29 hours if it throws false positives. But like this, you could see that the backup is supposed to take, say, 3 hours - except now it takes 4 due to growth of the system. So like this, you can now apply an "obviously correct" fix of bumping up the approximate backup duration based on monitoring data. |
| |
| ▲ | peheje 3 days ago | parent | next [-] | | I don't necessarily disagree with providing context, but my concern is that comments eventually lie. If the business rule evolves (say the window moves to 5 days) the comment becomes a liability the moment someone updates the code but forgets the prose. The comment also leaves me with more questions: how do you handle multiple identical amounts in that window? I would still have to read the implementation to be sure. I would prefer encoding this in an Uncle Bob style test. It acts as living documentation that cannot get out of sync with the code and explains the why through execution. For example: test("should_match_debit_with_credit_only_if_within_three_day_settlement_window", () => {
const debit = A_Transaction().withAmount(500.00).asDebit().on(JANUARY_1);
const creditWithinWindow = A_Transaction().withAmount(500.00).asCredit().on(JANUARY_4);
const creditOutsideWindow = A_Transaction().withAmount(500.00).asCredit().on(JANUARY_5);
expect(Reconciliation.tryMatch(debit, creditWithinWindow)).isSuccessful();
expect(Reconciliation.tryMatch(debit, creditOutsideWindow)).hasFailed();
});
This way, the 3 day rule is a hard requirement that fails the build if broken rather than a suggestion in a comment block. | | |
| ▲ | kmoser 3 days ago | parent | next [-] | | > I don't necessarily disagree with providing context, but my concern is that comments eventually lie. If the business rule evolves (say the window moves to 5 days) the comment becomes a liability the moment someone updates the code but forgets the prose. Yes, comments can drift out of sync with the code. That doesn't mean the comments were a bad idea; it just means the programmer didn't update the comments when they should have. Similarly, even with zero comments, variable names can drift out of sync with their intended purpose, e.g. when they are repurposed to hold values that are different from what their name implies. Again, the solution is to rename the variable so it reflects its purpose (or possibly create another variable). > This way, the 3 day rule is a hard requirement that fails the build if broken rather than a suggestion in a comment block. What happens when a dev changes the test code, but fails to update the string "should_match_debit_with_credit_only_if_within_three_day_settlement_window"? That's effectively no different than comments being out of sync with the code. | |
| ▲ | tetha 3 days ago | parent | prev | next [-] | | This however misses an important point: 3 is not in our control. 3 in general is controlled by math-people, and that 3 in particular is probably in the hands of a legal/regulation department. That's a much more important information to highlight. For example, at my last job, we shoved all constants managed by the balancing teams into a static class called BalancingTeam, to make it obvious that these values are not in our control. Tests, if (big-if) written, should revolve around the constants to not be brittle. | | |
| ▲ | peheje 3 days ago | parent [-] | | I like the idea of using a LegalConstants namespace or Code Owners to signal that we don't own those values. However, I’d argue that being 'out of our control' is actually the main reason to test them. We treat these as acceptance tests. The goal isn't flexibility, it is safety. If a PR changes a regulated value, the test failure acts as a tripwire. It forces us to confirm with the PM (and the Jira ticket) that the change is intentional before merging. It catches what code structure alone might miss. | | |
| ▲ | 8note 2 days ago | parent [-] | | or very concretely, if legal sets a constant to 0, there should be some test that makes sure the code handles divisions by that number properly, or otherwise fail to deploy. youd need tests both with the current constant for the current state, and with various bounds on that constant to show that the code always works |
|
| |
| ▲ | sbuttgereit 3 days ago | parent | prev [-] | | Until someone changes the test to be four days, or two, but doesn't update the test name. Ultimately disciplined practice requirements rely on disciplined practices to succeed. You can move the place where the diligence needs to taken, but at the end the idea that comments can lose their meaning isn't that different to other non-functional, communicative elements also being subject to neglect. I don't mean to suggest that a longish test title wouldn't be more likely to be maintained, but even with that long name you are losing some context that is better expressed, person-to-person, using sentences or even paragraphs. I had first hand experiences with this, oddly enough, also working on an accounting system (OK, ERP system... working the accounting bits). I was hired to bring certain deficient/neglected accounting methodologies up to a reasonable standard and implement a specialized inventory tracking capability. But the system was 20 years old and the original people behind the design and development had left the building. I have a pretty strong understanding of accounting and inventory management practices, and ERP norms in general, but there were instances where the what the system was doing didn't make sense, but there was no explanations (i.e. comments) as to why those choices had been taken. The accounting rules written in code were easier to deal with, but when we got to certain record life-cycle decisions, like the life-cycle evolution of credit memo transactions, the domain begins to shift from, "what does this mean from an accounting perspective", where generally accepted rules are likely to apply, to what did the designers of the application have in mind related to the controls of the application. Sure I could see what the code did and could duplicate it, but I couldn't understand it... not without doing a significant amount of detective work and speculation. The software developers that worked for the company that made this software were in the same boat: they had no idea why certain decisions were taken, if those decisions were right or wrong, or the consequences of changes... nor did they care really (part of the reason I was brought in). Even an out-of-date comment, one that didn't reflect the code has it evolved, would still have provided insight into the original intents. I know as well as you that code comments are often neglected as things change and I don't take it for granted that understanding the comments are sufficient for knowing what a piece of code does.... but understanding the mind of the author or last authors does have value and would have helped multiple times during that project. When I see these kinds of discussions I'm always reminded of one of my favorite papers. By Peter Naur, "Programming As Theory Building" (https://pages.cs.wisc.edu/~remzi/Naur.pdf). In my mind, comments that were at least good and right at one time can give me a sense of the theory behind the application, even if they cannot tell me exactly how things work today. | | |
| ▲ | 8note 2 days ago | parent [-] | | with code in good shape, i think i prefer having unnamed tests and instead you read the test to see that its an important function. however, ive also done code archaeology, and same thing, old inaccurate comments were one of the few remaining things with any idea what the code was supposed to do and got me enough pointers to company external docs to figure out the right stuff. wiki links, issue links, etc all had been deleted. same with the commit history, and the tests hadnt worked in >5 years and had also been deleted the best comments on that code were about describing Todos and bugs that existed rather than what the code did do. stream of consciousness comments and jokes what Ive left for future archaeologists of my code is detailed error messages about what went wrong and what somebody needs to do to fix that error |
|
| |
| ▲ | ulbu 4 days ago | parent | prev | next [-] | | we tend to talk about type composition, but values too are composed. this suggestion makes the composition clear and, imo, is strictly better that a hardcoded resulting value. even more so if we assign it using some blocks-with-returns feature, if the language has it: we can clarify that the value’s components are not used elsewhere and thus reduce complexity of the namespace. without such feature, I’m not sure complicating the namespace is worth it, and a comment may actually be better. | |
| ▲ | dijksterhuis 4 days ago | parent | prev [-] | | i personally prefer this kind of version — if i want to do the maths to work out tweaks i can, but i’m not forced to do maths in my head to know/tweak the end value // a total of
// - backup interval = 24
// - approx backup duration = 2
// - “wiggle room” = 2
ageAlertThresholdHours = 28
yes lazy devs are lazy and won’t want to or just won’t update the comments (be pedantic in review :shrug:). it’s all trading one thing off with another at the end of the day.edit — also your version forces me to read horizontal rather than vertical, which takes longer ime. sorry, i’ve basically done an unprompted code review. i feel like a bit of a dick now. | | |
| ▲ | Joker_vD 4 days ago | parent | next [-] | | const int backupIntervalHours = 24
const int approxBackupDurationHours = 2
const int wiggleRoomHours = 2
const int ageAlertThresholdHours = backupIntervalHours + approxBackupDurationHours + wiggleRoomHours;
static_assert(28 == ageAlertThresholdHours);
It's a shame more languages don't have static asserts... faking it with mismatched dimensions of array literal/duplicate keys in map literals is way too ugly and distracting from the intent. | | |
| ▲ | pwdisswordfishy 3 days ago | parent [-] | | Mmm... ageAlertThresholdHours = 24 + // backup interval
2 + // approx backup duration
2; // "wiggle room"
No static assert needed, no need to pre-compute the total the first time, and no need to use identifiers like `approxBackupDurationHours`, the cognitive override about the possibility of colliding with other stuff that's in scope, or the superfluous/verbose variable declaration preamble. | | |
| ▲ | feffe 3 days ago | parent [-] | | I'm a believer in restricting the scope of definitions as much as possible, and like programming languages that allows creating local bindings for creating another. For example: local
val backupIntervalHours = 24
val approxBackupDurationHours = 2
val wiggleRoomHours = 2
in
val ageAlertThresholdHours = backupIntervalHours + approxBackupDurationHours + wiggleRoomHours
end
Then it's easier to document what components a constant is composed of using code without introducing unnecessary bindings in the scope of the relevant variable. Sure constants are just data, but the first questions that pops into my head when seeing something in unfamiliar code is "What is the purpose of this?", and the smaller the scope, the faster it can be discarded. | | |
| ▲ | zephen 3 days ago | parent | next [-] | | Mentally discarding a name still takes some amount of effort, even if local. I often write things the way you have done it, for the simple reason that, when writing the code, maybe I feel that I might have more than one use for the constant, and I'm used to thinking algebraically. Except, that I might make them global, at the top of a module. Why? Because they encode assumptions that might be useful to know at a glance. And I probably wouldn't go back and remove the constants once they were named. But I also have no problem with unnamed but commented constants like the ones in the comment you responded to. | |
| ▲ | a day ago | parent | prev [-] | | [deleted] |
|
|
| |
| ▲ | tetha 3 days ago | parent | prev [-] | | > sorry, i’ve basically done an unprompted code review. i feel like a bit of a dick now. That's all fine. Just note that this was one of the easiest examples I could find. For example, for reasons out of my control, the individual network configuration on a linux host is positively nuts. The decision whether to deploy routes, static DNS servers and such depends on 3-5 facts about the datacenter and the provider it's on. In such a case, it is more maintainable to separate the facts about the provider, or the thing we are looking at (e.g. "Does this provider allow us to configure routes in their DHCP server?", from the computation/decision making ("Can the system rely on the routes from the DHCP servers?"), and all of that from the actual action based off of the decision ("Deploy routes statically if DHCP provided routes are not correct"). |
|
|
|
| ▲ | epgui 4 days ago | parent | prev | next [-] |
| What you describe really is describing the "why", not the "what". The line between the two is not that blurry: assume your reader has total knowledge of programming, and no knowledge whatsoever of the outside world. Comments about what the code does to bits are the "what"; comments about how the code relates to the outside world are the "why". The rest is a matter of taste and judgment. |
| |
| ▲ | lukan 4 days ago | parent [-] | | Just curious, you advice against "what" comments? "assume your reader has total knowledge of programming" Because if I know my fellow programmers have like me not a total knowledge of programming, what comments before footguns seem useful to me. Or when there was a hack that is not obvious. To me it mostly is not a question of taste, but context. Who will read the code? | | |
| ▲ | seba_dos1 3 days ago | parent | next [-] | | If you're writing a coding tutorial, you'll want to comment on the "what" indeed. Otherwise it will most likely end up being more distracting than useful, and sometimes even misleading. Exceptions exist, but by virtue of being exceptions there's no catch-all rule for them, so just use your judgment. | |
| ▲ | epgui 3 days ago | parent | prev [-] | | I almost (?) always advise against “what” comments. I have rarely (if ever?) encountered any cases where “what” comments didn’t have a better (and practical/cheap/easy enough) solution. In my experience, when I review junior contributors’ code and see “what” comments, it’s usually caused by 1) bad naming, or 2) abstractions that don’t make sense, or 3) someone trying to reinvent maths but incorrectly, or 4) missing tests, or 5) issues with requirement gathering / problem specification, or 6) outright laziness where the contributor doesn’t want to take the time to really think things through, or 7) unclear and overcomplicated code, or… any number of similar things. At the very least, any time you see a “what” comment, it’s valuable to take notice and try really hard to think about whether the problem the comment tries to solve has a better solution. Take it as a personal challenge. | | |
| ▲ | lukan 3 days ago | parent [-] | | For sure, bad code exists. But if I have to work with bad unclear code, "what" comments are very helpful. Like something really bad x=y //triggers method xyz So I would agree that under controlled conditions, they should not be necessary. | | |
|
|
|
|
| ▲ | coffeebeqn 4 days ago | parent | prev | next [-] |
| I also do that but I’d argue that business rules/quirks count as a “why” |
| |
| ▲ | bostik 4 days ago | parent | next [-] | | While I agree, I think that's still incomplete. To me good comments have always been about "what is being done AND why". Or to put it another way: to provide the necessary context for figuring out why a particular piece of code is written the way it's done. (Or what it's supposed to do.) | |
| ▲ | mashally 4 days ago | parent | prev | next [-] | | I agree with that as well, the "why" explains the business logic and that makes much more sense to me. Otherwise, I might end up explaining the algorithm (the "what") instead of the business behind it. | |
| ▲ | gbin 4 days ago | parent | prev | next [-] | | I agree it is about "what the code is technically doing" vs why which is "what external factor you need to understand to read this" | |
| ▲ | troupo 4 days ago | parent | prev [-] | | They are often a "why" in the form of "what the hell" :) |
|
|
| ▲ | dijksterhuis 4 days ago | parent | prev | next [-] |
| ime summaries/details of the “business what” are the most important part of “why the code exists/is written this way” — it’s the overarching problem that section of code aims to solve / needs to work around! they can also be a good defence against newer seniors on the team refactoring willy nilly — can’t argue with business rules, but can argue over uncle bob. ;) |
|
| ▲ | mgkimsal 3 days ago | parent | prev | next [-] |
| The 'comment becomes a lie' is because you've got magic number there in the comment. If the comment was // We match them by looking for equal-opposite amounts within an X-day window defined by BANK_TRANSFER_MATCH_WINDOW_DAYS
the comment is more evergreen, until the actual logic changes. If/when the logic changes... update the comment? |
|
| ▲ | tyre 4 days ago | parent | prev | next [-] |
| Does the caller of the method need to know the three days? Why? What if you update to FED Now and it changes? It seems like the caller only needs to know that it’s checking within the window, which is then likely a constant (like SETTLEMENT_WINDOW_DAYS) that gives the context. If you need to, you can add a comment to the constant linking to whatever documentation defines why three days. |
|
| ▲ | sublinear 3 days ago | parent | prev | next [-] |
| > The Uncle Bob approach of extractNameMatchingTransfersWithinSettlementWindow() doesn't actually help - now I need to know what settlement windows are anyway, and I've lost the context of why 3 days. When the name is long, it should be more than one function. Finding the transactions within a settlement window seems distinct from matching them up. The comment about 3 days isn't necessary when "numDays" is a variable in the SettlementWindow class (or other relevant scope). I'd be a lot more comfortable reading code that makes this concept a proper abstraction rather than just seeing a comment about a hard-coded 3 somewhere. Magic values are bad and comments don't demystify anything if they get out of sync with the code. |
|
| ▲ | zephen 3 days ago | parent | prev | next [-] |
| > The "what" vs "why" distinction breaks down when your code encodes domain knowledge that readers can't infer from context. Yes, there are multiples levels of knowledge, and the required level of commenting depends on the minimum knowledge expected of a reader in each of several dimensions. In point of fact, the only thing that almost always can do without documentation is a different question, "How." |
|
| ▲ | wizzwizz4 4 days ago | parent | prev | next [-] |
| > and that's the tolerance banks allow for settlement delays This is missing from your comment. While I'd probably be able to pick that up from context, I'd have to hold "this is an assumption I'm making" in my head, and test that assumption against all parts of the codebase I encounter until I become certain: this'd slow me down. I'd recommend including the "why" explicitly. |
|
| ▲ | zahlman 3 days ago | parent | prev | next [-] |
| > The Uncle Bob approach of extractNameMatchingTransfersWithinSettlementWindow() doesn't actually help Sure it does. You've isolated the "what" to the name, and the comments can focus on the "why". (Although I think some of the XP advocates would go off the rails and try to model a settlement window as an object....) |
|
| ▲ | antonymoose 4 days ago | parent | prev | next [-] |
| I always use “what” comments for regular expressions, in addition I provide examples of before and after transformations so that future developers know immediately what’s going on without having to first stop, context-switch, and decipher hieroglyphs. |
| |
| ▲ | epgui 4 days ago | parent [-] | | This is probably one of the best use cases for "what" comments... however in my opinion a much better way to go about this is to have example-based tests (and maybe a decent function name) serve as your documentation. |
|
|
| ▲ | Xss3 4 days ago | parent | prev | next [-] |
| GetNameMatchedTransfersWithin(int Days) Gives a good idea of what but not why and if anyone considers that method name too long they can get back in their cave, tbh. |
|
| ▲ | carlmr 3 days ago | parent | prev | next [-] |
| The within settlement window might still be a good addition to the method name. Since that is something I can at least Google. |
|
| ▲ | locknitpicker 4 days ago | parent | prev [-] |
| > The "what" vs "why" distinction breaks down when your code encodes domain knowledge that readers can't infer from context. Your comment is a poor example because you unwittingly wrote "why" comments, not "what". // We match them by looking for equal-opposite amounts within a 3-day window because bank transfers appear as two transactions - a debit here and credit elsewhere
If anything, you proved the importance of clarifying why things must be the way they are.> The Uncle Bob approach of extractNameMatchingTransfersWithinSettlementWindow() doesn't actually help - now I need to know what settlement windows are anyway, and I've lost the context of why 3 days. You are also completely off in here as well, and I think that your comment shows a deeper misunderstanding of the topic. What you tried to frame as "Uncle Bob approach" actually means implementing your domain in a clear and unambiguous way so that both the "what" and the "why" are clear by looking at the code. Naming conventions are one aspect, but the biggest trait is specifying an interface that forces the caller no option other than doing the what's because of the why's. Without looking too hard into it, whipping out something like this in a few seconds would match the intent GetMatchingTransfers(TransferAmount, SettlementWindow)
|