-
-
Notifications
You must be signed in to change notification settings - Fork 358
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
Improve big integer number formatting #75
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -22,38 +22,50 @@ final class Count | |
private int $count; | ||
|
||
public function __construct( | ||
float $count | ||
int $count | ||
) { | ||
$this->count = $count; | ||
|
||
Assert::lessThan( | ||
$count, | ||
self::MAX_COUNT, | ||
'The maximum number of views has been reached' | ||
$count, | ||
self::MAX_COUNT, | ||
'The maximum number of views has been reached', | ||
); | ||
$this->count = intval($count); | ||
Assert::greaterThanEq( | ||
$count, | ||
0, | ||
"Received a negative number of views" | ||
$count, | ||
0, | ||
'Number of views cannot be negative', | ||
); | ||
} | ||
|
||
public static function ofString(string $countStr): self | ||
{ | ||
Assert::digits( | ||
$countStr, | ||
'The base count must only contain digits' | ||
); | ||
$count = floatval($countStr); | ||
return new self($count); | ||
public static function ofString( | ||
string $value | ||
): self { | ||
Assert::digits( | ||
$value, | ||
'The base count must only contain digits', | ||
); | ||
$count = intval($value); | ||
|
||
return new self($count); | ||
} | ||
|
||
public function toInt(): int | ||
{ | ||
return $this->count; | ||
} | ||
|
||
public function plus(self $count): self | ||
{ | ||
return new self($this->toInt() + $count->toInt()); | ||
public function plus( | ||
self $that | ||
): self { | ||
$sum = $this->toInt() + $that->toInt(); | ||
|
||
if (!is_int($sum)) { | ||
throw new \InvalidArgumentException( | ||
'The maximum number of views has been reached', | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. With this check, we currently have two sources of truth for checking whether the maximum number of views has been reached: the If we are sure that If With the implementation in the PR, if There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Or maybe, in that case, we can change the error message in the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah this kinda bad design for now, but in reality I'm not sure we will ever reach the max value 😅 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's just a protection vs type error in cases when people will try to send too big base value. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Right now there are a lot of fraud requests from users, trying to avoid error log from the spam 😞 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'd suggest to push it as is and rethink it later if someone wants to invest time into it . There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Cool |
||
); | ||
} | ||
|
||
return new self($sum); | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've removed floats and wrote own string number formatter, because float numbers are formatting incorrectly anyway.