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

Improve big integer number formatting #75

Merged
merged 3 commits into from
Sep 17, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion README.md
Original file line number Diff line number Diff line change
Expand Up @@ -110,6 +110,7 @@ You can overwrite default `Profile views` text with your own label.

You can provide a `base` number to add to the counter.
This is useful if you are migrating from another service.

For example, a user with 1000 views on another service who wants to migrate to GHPVC will use the following url
to ensure the 1000 views are accounted for:
```markdown
Expand All @@ -128,7 +129,7 @@ This project provides minimalistic counter only. Use [Ÿ HŸPE] service if you w

### How to reset counter?

To reset counter you should login to the [Ÿ HŸPE] service and then you will be able to reset counter on the https://yhype.me/ghpvc page.
To reset counter you should log in to the [Ÿ HŸPE] service, and then you will be able to reset counter on the https://yhype.me/ghpvc page.

### Why does the counter increase every time the page is reloaded?

Expand Down
4 changes: 2 additions & 2 deletions public/index.php
Original file line number Diff line number Diff line change
Expand Up @@ -63,13 +63,13 @@
);
if ($baseCount !== '0') {
$count = $count->plus(
Count::ofString($baseCount)
Count::ofString($baseCount),
);
}

echo $badgeImageRenderer->renderBadgeWithCount(
$badgeLabel,
$count->toInt(),
$count,
$badgeMessageBackgroundFill,
$badgeStyle,
);
Expand Down
17 changes: 15 additions & 2 deletions src/BadgeImageRendererService.php
Original file line number Diff line number Diff line change
Expand Up @@ -36,11 +36,11 @@ public function __construct()

public function renderBadgeWithCount(
string $label,
int $count,
Count $count,
string $messageBackgroundFill,
string $badgeStyle
): string {
$message = number_format($count);
$message = $this->formatNumber($count->toInt());

return $this->renderBadge(
$label,
Expand Down Expand Up @@ -84,4 +84,17 @@ private function renderBadge(
Badge::DEFAULT_FORMAT,
);
}

/**
* This method required because of native `number_format`
* method has big integer format limitation.
*/
private function formatNumber(
Copy link
Owner Author

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.

int $number
): string {
$reversedString = strrev(strval($number));
$formattedNumber = implode(',', str_split($reversedString, 3));

return strrev($formattedNumber);
}
}
50 changes: 31 additions & 19 deletions src/Count.php
Original file line number Diff line number Diff line change
Expand Up @@ -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',
Copy link
Contributor

Choose a reason for hiding this comment

The 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 is_int check here and the assert less than MAX_COUNT assertion in the constructor. We know this wouldn't cause logical problems since MAX_COUNT will always be <= PHP_INT_MAX, but maybe it would be better if they become a single source of truth instead?

If we are sure that MAX_COUNT is always going to be PHP_INT_MAX and won't change in the future, then maybe we can remove the assertion in the constructor (along with MAX_COUNT) since it only receives ints and compares them against PHP_INT_MAX (ints will never exceed PHP_INT_MAX).

If MAX_COUNT might change in the future to be less than PHP_INT_MAX, then maybe we should find a way to do the check in one place or to do the same check here and in the constructor.

With the implementation in the PR, if MAX_COUNT changes, the message "the maximum number of views has been reached" in the plus function implies a different meaning than the same message in the constructor (albeit that doesn't cause any logical problems).

Copy link
Contributor

Choose a reason for hiding this comment

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

If MAX_COUNT might change in the future to be less than PHP_INT_MAX, then maybe we should find a way to do the check in one place or to do the same check here and in the constructor.

Or maybe, in that case, we can change the error message in the plus function to something along the lines of: "the number of views have exceeded the integer limit". This way we wouldn't have one error message having two different meanings (exceeding the integer limit and exceeding MAX_COUNT).

Copy link
Owner Author

Choose a reason for hiding this comment

The 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 😅

Copy link
Owner Author

Choose a reason for hiding this comment

The 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.

Copy link
Owner Author

Choose a reason for hiding this comment

The 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 😞

Copy link
Owner Author

@antonkomarev antonkomarev Sep 17, 2023

Choose a reason for hiding this comment

The 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 .

Copy link
Contributor

Choose a reason for hiding this comment

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

Cool

);
}

return new self($sum);
}
}
Loading