-
Notifications
You must be signed in to change notification settings - Fork 12
HP-2194 added annotation for Sums class to understand what can be stored in it #79
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
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 | ||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -5,46 +5,56 @@ | |||||||||||||||||||||||||||||||
| final readonly class Sums implements \JsonSerializable | ||||||||||||||||||||||||||||||||
| { | ||||||||||||||||||||||||||||||||
| /** | ||||||||||||||||||||||||||||||||
| * @param int[]|null $values quantity => total sum for the quantity | ||||||||||||||||||||||||||||||||
| * Quantity of what? | ||||||||||||||||||||||||||||||||
| * Sum of what? | ||||||||||||||||||||||||||||||||
| * If you know answers please write in the comment | ||||||||||||||||||||||||||||||||
| * @param array<int, float|int>|null $quantityToSumMap An associative array where: | ||||||||||||||||||||||||||||||||
| * - The key represents the **quantity** of the action being charged for | ||||||||||||||||||||||||||||||||
| * (e.g., the number of years for an SSL certificate). | ||||||||||||||||||||||||||||||||
| * - The value represents the **total sum** or **price** for the given quantity. | ||||||||||||||||||||||||||||||||
| * | ||||||||||||||||||||||||||||||||
| * Example (If used to denote bulk prices): | ||||||||||||||||||||||||||||||||
| * E.g. when you buy an SSL certificate for 1 year – it costs 10$ | ||||||||||||||||||||||||||||||||
| * But for 2 years you pay 15$. | ||||||||||||||||||||||||||||||||
| * | ||||||||||||||||||||||||||||||||
| * It will be is stored as | ||||||||||||||||||||||||||||||||
| * | ||||||||||||||||||||||||||||||||
| * [1 => 10, 2 => 15] | ||||||||||||||||||||||||||||||||
| */ | ||||||||||||||||||||||||||||||||
| public function __construct(private ?array $values) | ||||||||||||||||||||||||||||||||
| public function __construct(private ?array $quantityToSumMap) | ||||||||||||||||||||||||||||||||
| { | ||||||||||||||||||||||||||||||||
| if (!empty($this->values)) { | ||||||||||||||||||||||||||||||||
| $this->validate($this->values); | ||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||
| $this->validateSums($this->quantityToSumMap); | ||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||
| private function validate(array $sums): void | ||||||||||||||||||||||||||||||||
| private function validateSums(?array $sums): void | ||||||||||||||||||||||||||||||||
| { | ||||||||||||||||||||||||||||||||
| if ($sums) { | ||||||||||||||||||||||||||||||||
| if (!empty($sums)) { | ||||||||||||||||||||||||||||||||
| foreach ($sums as $value) { | ||||||||||||||||||||||||||||||||
| if (!is_numeric($value)) { | ||||||||||||||||||||||||||||||||
| throw new PriceInvalidArgumentException('Invalid value for sums parameter'); | ||||||||||||||||||||||||||||||||
| throw new PriceInvalidArgumentException('All sums must be numeric values.'); | ||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||
| public function values(): ?array | ||||||||||||||||||||||||||||||||
| { | ||||||||||||||||||||||||||||||||
| return $this->values; | ||||||||||||||||||||||||||||||||
| return $this->quantityToSumMap; | ||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||
| public function getSum(int $quantity) | ||||||||||||||||||||||||||||||||
| { | ||||||||||||||||||||||||||||||||
| return $this->values[$quantity] ?? null; | ||||||||||||||||||||||||||||||||
| return $this->quantityToSumMap[$quantity] ?? null; | ||||||||||||||||||||||||||||||||
|
Contributor
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. 🛠️ Refactor suggestion Add return type hint to The Apply this diff to add the return type hint: -public function getSum(int $quantity)
+public function getSum(int $quantity): float|int|null
|
||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||
| public function getMinSum(): int|string | ||||||||||||||||||||||||||||||||
| public function getMinSum() | ||||||||||||||||||||||||||||||||
| { | ||||||||||||||||||||||||||||||||
| return min($this->values); | ||||||||||||||||||||||||||||||||
| if (empty($this->quantityToSumMap)) { | ||||||||||||||||||||||||||||||||
| return null; | ||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||
| return min($this->quantityToSumMap); | ||||||||||||||||||||||||||||||||
|
Comment on lines
+47
to
+53
Contributor
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. 🛠️ Refactor suggestion Add return type hint to The Apply this diff to add the return type hint: -public function getMinSum()
+public function getMinSum(): float|int|null📝 Committable suggestion
Suggested change
|
||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||
| public function jsonSerialize(): ?array | ||||||||||||||||||||||||||||||||
| { | ||||||||||||||||||||||||||||||||
| return $this->values; | ||||||||||||||||||||||||||||||||
| return $this->quantityToSumMap; | ||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,96 @@ | ||
| <?php declare(strict_types=1); | ||
|
|
||
| namespace hiqdev\php\billing\tests\unit\price; | ||
|
|
||
| use hiqdev\php\billing\price\PriceInvalidArgumentException; | ||
| use hiqdev\php\billing\price\Sums; | ||
| use JsonSerializable; | ||
| use PHPUnit\Framework\TestCase; | ||
|
|
||
| class SumsTest extends TestCase | ||
| { | ||
| public function testConstructorWithValidData(): void | ||
| { | ||
| $validSums = [1 => 10.5, 2 => 15.0, 3 => 20]; | ||
|
|
||
| $sums = new Sums($validSums); | ||
|
|
||
| $this->assertSame($validSums, $sums->values()); | ||
| } | ||
|
|
||
| public function testConstructorWithInvalidDataThrowsException(): void | ||
| { | ||
| $this->expectException(PriceInvalidArgumentException::class); | ||
| $this->expectExceptionMessage('All sums must be numeric values.'); | ||
|
|
||
| $invalidSums = [1 => 10, 2 => 'abc']; | ||
|
|
||
| new Sums($invalidSums); | ||
| } | ||
|
|
||
| public function testGetSum(): void | ||
| { | ||
| $sums = new Sums([1 => 10, 2 => 15, 3 => 20]); | ||
|
|
||
| $this->assertSame(10, $sums->getSum(1)); | ||
| $this->assertSame(15, $sums->getSum(2)); | ||
| $this->assertNull($sums->getSum(4)); // Not found | ||
| } | ||
|
|
||
| /** | ||
| * @dataProvider minSumDataProvider | ||
| */ | ||
| public function testGetMinSum($input, $expected): void | ||
| { | ||
| $sums = new Sums($input); | ||
|
|
||
| $this->assertSame($expected, $sums->getMinSum()); | ||
| } | ||
|
|
||
| public function minSumDataProvider(): array | ||
| { | ||
| return [ | ||
| // Single element case | ||
| 'single positive integer' => [[1 => 10], 10], | ||
| 'single negative integer' => [[1 => -10], -10], | ||
| 'single decimal' => [[1 => 5.5], 5.5], | ||
|
|
||
| // Multiple elements case | ||
| 'multiple positive integers' => [[1 => 10, 2 => 20, 3 => 5], 5], | ||
| 'multiple with negative integers' => [[1 => -10, 2 => 20, 3 => 0], -10], | ||
| 'multiple decimals' => [[1 => 10.5, 2 => 20.7, 3 => 5.5], 5.5], | ||
| 'mixed integers and decimals' => [[1 => 10, 2 => 20.7, 3 => 5], 5], | ||
|
|
||
| // Edge cases | ||
| 'positive and negative decimals' => [[1 => -1.5, 2 => 1.5], -1.5], | ||
| 'zero and positive integers' => [[1 => 0, 2 => 5], 0], | ||
| 'zero and negative integers' => [[1 => 0, 2 => -5], -5], | ||
| ]; | ||
| } | ||
|
|
||
| public function testJsonSerializableBehavior(): void | ||
| { | ||
| $sums = new Sums([1 => 10, 2 => 15, 3 => 20]); | ||
|
|
||
| $this->assertInstanceOf(JsonSerializable::class, $sums); | ||
|
|
||
| $expectedJson = json_encode([1 => 10, 2 => 15, 3 => 20]); | ||
| $this->assertSame($expectedJson, json_encode($sums)); | ||
| } | ||
|
|
||
| public function testSumsWithNullInput(): void | ||
| { | ||
| $sums = new Sums(null); | ||
|
|
||
| $this->assertNull($sums->values()); | ||
| $this->assertNull($sums->getSum(1)); | ||
| } | ||
|
|
||
| public function testSumsWithEmptyArray(): void | ||
| { | ||
| $sums = new Sums([]); | ||
|
|
||
| $this->assertSame([], $sums->values()); | ||
| $this->assertNull($sums->getSum(1)); | ||
| } | ||
| } |
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.
Validate that the keys are integers in
validateSumsmethodCurrently, the
validateSumsmethod checks that all sums (values) are numeric but does not verify that the keys (quantities) are integers. Since$quantityToSumMapis defined asarray<int, float|int>|null, it's important to ensure that all keys are integers.Consider adding a check to validate that the keys are integers. Here's a possible modification:
private function validateSums(?array $sums): void { if (!empty($sums)) { foreach ($sums as $key => $value) { + if (!is_int($key)) { + throw new PriceInvalidArgumentException('All quantities (keys) must be integers.'); + } if (!is_numeric($value)) { throw new PriceInvalidArgumentException('All sums must be numeric values.'); } } } }📝 Committable suggestion