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.
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();
});
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.
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.
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.
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
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.
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
// a total of
// - backup interval = 24
// - approx backup duration = 2
// - “wiggle room” = 2
ageAlertThresholdHours = 28
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.
const int backupIntervalHours = 24
const int approxBackupDurationHours = 2
const int wiggleRoomHours = 2
const int ageAlertThresholdHours = backupIntervalHours + approxBackupDurationHours + wiggleRoomHours;
static_assert(28 == ageAlertThresholdHours);
ageAlertThresholdHours = 24 + // backup interval
2 + // approx backup duration
2; // "wiggle room"
For example:
local
val backupIntervalHours = 24
val approxBackupDurationHours = 2
val wiggleRoomHours = 2
in
val ageAlertThresholdHours = backupIntervalHours + approxBackupDurationHours + wiggleRoomHours
end
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.
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").
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.
"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?
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.
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.)
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. ;)
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
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.
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.
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."
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.
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....)
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.
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
> 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)
I build accounting software and half my "what" comments are actually explaining business rules that would be impenetrable otherwise. Something like:
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.