Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove Pointless Excessive Abstraction #2425

Merged
merged 1 commit into from
Feb 15, 2021
Merged

Conversation

erights
Copy link
Member

@erights erights commented Feb 15, 2021

I see no way in which BIG_ONE is clearer than 1n. There is one way in which the constant 10000n is unclear. You might, at a glance, mistake the number of zeros you're looking at. However, BIG_10000n is no clearer. TC39 already addressed the readability issue of big numerical literals like that by letting us use underbars where on paper we'd use commas. 10_000n is clearer than both. It is actually clear.

The issue in this PR is really a microcosm of issues raised by #2420 . For all the wonder of abstraction it can be used excessively, at a cost to readability. We need to be sensitive to both sides of that tradeoff.

Historically, the origin of some of this stuff was when we did not yet have good enough toolchain support for BigInts. Today, some of it may come from worry about the efficiency of small BigInts compared to floats (aka numbers). However, we know that small BigInts are as efficient as floats on all JS engines other than XS. We do not yet know whether the extra cost on XS is significant for us. But we do know that, if it is, we can fix XS rather than make our code less readable.

I expect these points to be controversial. Let's have that discussion first on this PR where the issue is narrow and focused.

@erights
Copy link
Member Author

erights commented Feb 15, 2021

Note that this PR's CI run shows that we still have old parser breakage in our toolchain. No longer on the terminal n but this time on the underbars. We can't merge this PR until that is fixed. This PR should help us track that down.

@erights
Copy link
Member Author

erights commented Feb 15, 2021

See #2427

@FUDCo
Copy link
Contributor

FUDCo commented Feb 15, 2021

I expect these points to be controversial.

I disagree.

Copy link
Contributor

@katelynsills katelynsills left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks fine to me. Btw, this code will be majorly rewritten in the conversion to Nat 4.0.0.

@Chris-Hibbert
Copy link
Contributor

I'm going to mostly agree with your point, but disagree about the implementation of it. Nearly all zeroes and ones are indistinguishable, not because they aren't used for different things, but because the programmer can't conceive of changing them to a different number. That's not at all the case with 10_000n. I spent a couple of weeks at Niantic disentangling all the 8s in our code by purpose. (It was a game, that started out with 8 levels of players, and 8 levels for three different kinds of items, and 8 levels for some constructed objects. When we wanted 16 player levels, we needed every 8 to say what it was for.) Some 10_000s are BASIS_POINTS, which if they're used for entering parameters probably wouldn't change when someone decides we need to track holdings of stable coins to 6 decimal places.

But you're right that the name of that constant shouldn't be BIG_10000. OTOH, when I added BIG_10000, at least one of the manifest bigints I replaced had the wrong number of zeroes.

In the same way I replace 365 with a global YEAR, 10_000 should be replaced with a named global that says what it's for.

@@ -41,10 +38,10 @@ export const getInputPrice = (
assert(inputReserve > 0, X`inputReserve ${inputReserve} must be positive`);
assert(outputReserve > 0, X`outputReserve ${outputReserve} must be positive`);

const oneMinusFeeScaled = BIG_10000 - BigInt(feeBasisPoints);
const oneMinusFeeScaled = 10_000n - BigInt(feeBasisPoints);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please rename to BASIS_POINTS rather than using a manifest constant.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, actual abstraction that says more than the number! Done.

@erights erights enabled auto-merge (squash) February 15, 2021 20:41
Copy link
Contributor

@Chris-Hibbert Chris-Hibbert left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks!

@dtribble
Copy link
Member

Just to be clear, the issue with "magic numbers" (any number other than 0,1, and maybe -1) is not that we should uselessly replace numbers with strings, but that we should replace numbers with semantic meaning. 10000n should not be replaced with BIG_10k, but rather with, as above, BASIS_POINTS or UATOMS_MIN or UNITED_SILVER_QUALIFYING_MILES. You could easily have multiple of these numbers in use in the same place, and not be able to tell which 10k was which.

@erights erights merged commit 14285f5 into master Feb 15, 2021
@erights erights deleted the markm-excessive-abstraction branch February 15, 2021 21:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants