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

Exceptions for large numbers instead of TypeErrors #9

Merged
merged 2 commits into from May 3, 2019

Conversation

@JeroenDeDauw
Copy link
Contributor

commented Jan 17, 2019

Did not deal with newFromFloat yet.
For floats there is PHP_FLOAT_DIG, added in PHP 7.2

Did not deal with newFromFloat yet.
For floats there is PHP_FLOAT_DIG, added in PHP 7.2
@JeroenDeDauw

This comment has been minimized.

Copy link
Contributor Author

commented Jan 17, 2019

I had a few minutes so I poked at this after seeing some ticket about it earlier today. Don't have the link to the ticket here though.

@JeroenDeDauw

This comment has been minimized.

Copy link
Contributor Author

commented Jan 17, 2019

Tests passed. Issue: master build fails due to PHPCS too old. #10

@@ -105,6 +113,10 @@ public static function newFromFloat( float $euroAmount ): self {
* @throws InvalidArgumentException
*/
public static function newFromInt( int $euroAmount ): self {
if ( $euroAmount > floor( PHP_INT_MAX / 101 ) ) {

This comment has been minimized.

Copy link
@tzhelyazkova

tzhelyazkova Jan 21, 2019

Member

why divide by 101?
The best I could find after googling is the wikipedia article on palindromic primes

This comment has been minimized.

Copy link
@gbirke

gbirke Jan 21, 2019

Member

101 is self::CENTS_PER_EURO + 1.

When using PHP_INT_MAX / self::CENTS_PER_EURO as a value for $euroAmount you still get a TypeError in the constructor, because for some reason, PHP_INT_MAX / self::CENTS_PER_EURO * self::CENTS_PER_EURO is then greater than PHP_INT_MAX 🤷‍♂️ . PHP converts every value above PHP_INT_MAX into a float, which leads to a type error in the constructor.

Good question!

This comment has been minimized.

Copy link
@JeroenDeDauw

JeroenDeDauw Jan 21, 2019

Author Contributor

I expected the /100 to work but it does not. Hence 101. That likely does not allow representing certain big numbers that strictly speaking can be represented though that seems like something that does not actually matter here. There might also be some small range of numbers for which the type error still happens, though again these will be very large ones that don't really matter. I expect this maybe-not-100%-correct-but-definitely-more-correct-than-before change to take care of all errors that currently end up in the log.

Add comments and use constants to make the upper limit more
comprehensible.

Refactor check into function and check upper limit for float values.
@gbirke
gbirke approved these changes Jan 21, 2019
// by self::CENTS_PER_EURO still exceeds PHP_INT_MAX, which leads type errors
// due to float conversion. The safest thing to do here is using a safety
// margin of 1 with self::CENTS_PER_EURO
if ( $euroAmount > floor( PHP_INT_MAX / ( self::CENTS_PER_EURO + 1 ) ) ) {

This comment has been minimized.

Copy link
@JeroenDeDauw

JeroenDeDauw Jan 21, 2019

Author Contributor

Such readability! Indeed better than 101

@JeroenDeDauw JeroenDeDauw merged commit 78d2c6c into master May 3, 2019
2 of 3 checks passed
2 of 3 checks passed
continuous-integration/travis-ci/push The Travis CI build failed
Details
Scrutinizer 9 updated code elements
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@JeroenDeDauw JeroenDeDauw deleted the typeerror branch May 3, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
4 participants
You can’t perform that action at this time.