> 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").