HP-2557: Created PriceTypeCollection for simplified work with PriceTypeInterface #109
Conversation
WalkthroughAdds a new PriceTypeCollection class to manage PriceTypeInterface instances with iteration, counting, name listing and O(1) existence checks; moves PriceTypeInterface into namespace hiqdev\php\billing\product\Domain\Model\Price; adds InvalidPriceTypeCollectionException and unit tests covering collection behavior. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Collection as PriceTypeCollection
participant Type as PriceTypeInterface
Client->>Collection: __construct(types[])
Note right of Collection `#E8F5E9`: store types[]\nbuild flipped map (name => true)
Client->>Collection: has("hourly")
alt exists
Collection->>Collection: lookup flipped["hourly"]
Collection-->>Client: true
else missing
Collection->>Collection: lookup flipped["hourly"]
Collection-->>Client: false
end
Client->>Collection: getIterator()
Collection-->>Client: ArrayIterator(types[])
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
…n accordance with DDD architecture
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/product/Domain/Model/Price/PriceTypeCollection.php (1)
12-13: Consider adding class-level PHPDoc.Adding a class-level docblock would improve documentation and clarify the purpose of this collection.
Apply this diff to add class-level documentation:
+/** + * Collection of PriceTypeInterface instances with efficient lookup by name. + */ class PriceTypeCollection implements IteratorAggregate, Countable
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/product/Domain/Model/Price/PriceTypeCollection.php(1 hunks)
🔇 Additional comments (4)
src/product/Domain/Model/Price/PriceTypeCollection.php (4)
24-30: LGTM!The
getIterator()implementation correctly returns anArrayIteratorover the types array. The PHPDoc type annotation is clear and accurate.
32-35: LGTM!The
has()method efficiently checks existence using the flipped array for O(1) lookup. The use ofarray_key_exists()is appropriate for this check.
37-40: LGTM!The
count()method correctly implements theCountableinterface requirement.
42-45: LGTM!The
hasItems()convenience method provides a readable way to check if the collection is non-empty.
|
|
||
| public function __construct(private readonly array $types = []) | ||
| { | ||
| $this->flippedTypes = array_flip(array_map(fn(PriceTypeInterface $t) => $t->name(), $types)); |
There was a problem hiding this comment.
Clarify or prevent duplicate name handling.
When multiple PriceTypeInterface instances have the same name(), array_flip silently keeps only the last occurrence in $flippedTypes. This creates an inconsistency: all duplicates remain in $types (returned by getIterator()), but has() won't distinguish between them. Consider either:
- Preventing duplicates by validating in the constructor
- Documenting this behavior explicitly
Example validation to prevent duplicates:
public function __construct(private readonly array $types = [])
{
+ $names = array_map(fn(PriceTypeInterface $t) => $t->name(), $types);
+ if (count($names) !== count(array_unique($names))) {
+ throw new \InvalidArgumentException('Duplicate price type names are not allowed');
+ }
$this->flippedTypes = array_flip(array_map(fn(PriceTypeInterface $t) => $t->name(), $types));
}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
tests/unit/product/Domain/Model/Price/PriceTypeCollectionTest.php (1)
69-80: Clarify the intended duplicate-name behavior.The test validates that the collection stores multiple instances with the same name, but this creates ambiguous semantics:
- Both instances are stored (count returns 2)
- The lookup map only tracks existence, not which instance
- There's no retrieval method to access specific instances
This behavior could confuse users who might expect either:
- Uniqueness enforcement (like a Set)
- The ability to retrieve all instances with a given name
Consider:
- Adding constructor validation to prevent duplicates and throw a descriptive exception
- Documenting this behavior explicitly in the class docblock if it's intentional
- Clarifying the use case: Is the collection meant only for iteration and existence checks, or should it support retrieval?
Can you clarify the intended behavior? Should the collection enforce uniqueness, or is the current "allow duplicates but only track existence" behavior intentional for your use case?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/unit/product/Domain/Model/Price/PriceTypeCollectionTest.php(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
tests/unit/product/Domain/Model/Price/PriceTypeCollectionTest.php (1)
src/product/Domain/Model/Price/PriceTypeCollection.php (3)
PriceTypeCollection(11-45)hasItems(41-44)has(31-34)
🔇 Additional comments (5)
tests/unit/product/Domain/Model/Price/PriceTypeCollectionTest.php (5)
13-20: LGTM!The empty collection test correctly verifies all three key behaviors: count, hasItems, and has. Good baseline test coverage.
22-29: LGTM!This test correctly validates count and hasItems for a non-empty collection.
31-37: LGTM!The helper method provides a clean way to create test instances. The use of an anonymous class is appropriate for unit testing.
39-49: LGTM!This test provides good coverage of the has() method, testing both existing and non-existent types.
51-67: LGTM!This test thoroughly validates the iterator implementation, checking both the type of yielded items and their order.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/product/Domain/Model/Price/Exception/InvalidPriceTypeCollectionException.php (1)
1-11: Consider adding a docblock and optionally a custom error message.The exception class is syntactically correct and appropriately extends
InvalidArgumentException. However, adding a docblock would clarify its purpose and usage for other developers. Additionally, consider whether a custom constructor or factory method providing details about invalid items would improve debugging (e.g., which item failed validation, expected vs. actual type).Example enhancement:
+/** + * Exception thrown when PriceTypeCollection receives invalid items. + * All items must implement PriceTypeInterface. + */ class InvalidPriceTypeCollectionException extends InvalidArgumentException { + public static function forInvalidItem(mixed $item): self + { + $type = get_debug_type($item); + return new self("Invalid item of type '{$type}'. Expected PriceTypeInterface."); + } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/product/Domain/Model/Price/Exception/InvalidPriceTypeCollectionException.php(1 hunks)src/product/Domain/Model/Price/PriceTypeCollection.php(1 hunks)tests/unit/product/Domain/Model/Price/PriceTypeCollectionTest.php(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/product/Domain/Model/Price/PriceTypeCollection.php
- tests/unit/product/Domain/Model/Price/PriceTypeCollectionTest.php
…sNonPriceType()` method because it makes the domain reason for the exception explicit and self-documenting
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
src/product/Domain/Model/Price/PriceTypeCollection.php (3)
19-23: Prevent or document duplicate price type names.When multiple
PriceTypeInterfaceinstances share the samename(),array_flipon line 22 silently retains only the last occurrence in$flippedTypeNames. This creates an inconsistency:getIterator()returns all duplicates from$types, buthas()andcount()won't reflect them accurately (count will include duplicates, but has() treats them as one).Consider validating uniqueness in the constructor:
public function __construct(private readonly array $types = []) { $this->assertAllPriceTypes($types); + $names = $this->names(); + if (count($names) !== count(array_unique($names))) { + throw InvalidPriceTypeCollectionException::becauseDuplicateNames($names); + } $this->flippedTypeNames = array_flip($this->names()); }You'll also need to add the corresponding factory method to
InvalidPriceTypeCollectionException.
19-19: Add PHPDoc for better IDE support and static analysis.The
$typesparameter lacks documentation indicating it expectsPriceTypeInterface[]. While runtime validation exists, adding PHPDoc improves IDE autocomplete and static analysis tools.+ /** + * @param PriceTypeInterface[] $types + */ public function __construct(private readonly array $types = [])
34-37: Consider adding return type annotation.Adding a PHPDoc return type annotation would improve static analysis and IDE support.
+ /** + * @return string[] + */ public function names(): array
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/product/Domain/Model/Price/Exception/InvalidPriceTypeCollectionException.php(1 hunks)src/product/Domain/Model/Price/PriceTypeCollection.php(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/product/Domain/Model/Price/Exception/InvalidPriceTypeCollectionException.php
🧰 Additional context used
🧬 Code graph analysis (1)
src/product/Domain/Model/Price/PriceTypeCollection.php (3)
src/product/Domain/Model/Price/Exception/InvalidPriceTypeCollectionException.php (2)
InvalidPriceTypeCollectionException(9-20)becauseContainsNonPriceType(11-19)tests/unit/product/Domain/Model/Price/PriceTypeCollectionTest.php (2)
__construct(35-35)name(36-36)src/product/Domain/Model/Price/PriceTypeInterface.php (1)
name(7-7)
🔇 Additional comments (2)
src/product/Domain/Model/Price/PriceTypeCollection.php (2)
25-32: LGTM! Type validation properly implemented.The validation method correctly ensures type safety at runtime and uses the appropriate custom exception with a descriptive error message.
39-60: LGTM! Well-implemented collection methods.The iterator, existence check, and counting methods are correctly implemented:
getIterator()properly returns an ArrayIterator with appropriate PHPDochas()uses efficient O(1) lookup viaarray_key_existscount()andhasItems()provide clear, straightforward functionality
2 participants
Summary by CodeRabbit
New Features
Breaking Changes
Tests