perf(string-methods): Pre-compute character mappings for O(1) lookup #58
Conversation
Replace runtime array operations with compile-time constants for significant performance improvements in searchWords() and removeAccents(). - Add SEARCH_WORDS_MAPPING constant with lowercase accent mappings - Add ACCENT_MAPPING constant for case-preserving accent removal - Simplify searchWords() to single strtr() call with constant - Simplify removeAccents() to single strtr() call with constant - Remove unused $searchWordsMapping static property - Remove unused applyBasicNameFix() method - Remove legacy REMOVE_ACCENTS_FROM/TO arrays - Add IsValidTimePartTest for 100% code coverage - Fix composer.json psalm plugin dependency Performance: removeAccents +66%, searchWords +104%, nameFix +39% Signed-off-by: Marjo Wenzel van Lier <marjo.vanlier@gmail.com>
|
Caution Review failedThe pull request is closed. Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. 📝 WalkthroughWalkthroughThe pull request refactors accent normalization and string search mapping from runtime-constructed arrays and cached replacements to pre-computed constant associative maps optimized for Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Disabled knowledge base sources:
⛔ Files ignored due to path filters (4)
📒 Files selected for processing (6)
Comment |
Summary of ChangesHello @MarjovanLier, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request focuses on a significant performance enhancement and simplification of string manipulation utilities. By moving from dynamic, runtime array construction and caching to pre-computed, compile-time constant mappings, the methods for accent removal and search word normalization are now more efficient. This change not only boosts performance but also streamlines the codebase by removing legacy logic and improving test coverage to a perfect 100%. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
|
Changelog updates: 🔄 * 2025-12-08Added
Changed
Fixed
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #58 +/- ##
=============================================
+ Coverage 94.62% 100.00% +5.37%
+ Complexity 40 32 -8
=============================================
Files 1 1
Lines 93 61 -32
=============================================
- Hits 88 61 -27
+ Misses 5 0 -5 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
PR Code Suggestions ✨Explore these optional code suggestions:
|
||||||||||||
|
Auto-approved PR |
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
PR Reviewer Guide 🔍(Review updated until commit 105b06e)Here are some key observations to aid the review process:
|
Reflect the new implementation using compile-time constants instead of lazy-initialised static properties. - Update benchmark numbers to reflect +66% to +104% improvements - Replace lazy init examples with pre-computed constant examples - Update "Static Caching" section to "Compile-Time Constants" - Replace "Pre-warm Cache" with "No Warm-up Required" - Update comparison table with new performance figures Signed-off-by: Marjo Wenzel van Lier <marjo.vanlier@gmail.com>
|
Changelog updates: 🔄 2025-12-08Changed
Added
Removed
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This is an excellent pull request that significantly improves performance by replacing runtime array operations with pre-computed constants. The simplification of searchWords() and removeAccents() makes the code much cleaner and more efficient. The removal of unused code and the addition of new tests to achieve 100% coverage are also great contributions. I have a couple of minor suggestions to make the character normalization even more comprehensive, but overall, this is a high-quality submission.
There was a problem hiding this comment.
Pull request overview
This PR optimizes string manipulation methods by replacing runtime array operations with pre-computed compile-time constants, delivering significant performance improvements (66% for removeAccents(), 104% for searchWords(), and 39% for nameFix()). The refactoring eliminates lazy initialization and static caching in favor of constant mappings that provide O(1) character lookup via strtr().
Key changes:
- Introduced
SEARCH_WORDS_MAPPINGandACCENT_MAPPINGconstants with pre-computed character mappings - Simplified
searchWords()andremoveAccents()to singlestrtr()calls - Removed static caching properties,
applyBasicNameFix()method, and legacyREMOVE_ACCENTS_FROM/TOarrays - Added comprehensive test coverage for
isValidTimePart()method to achieve 100% code coverage
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| src/AccentNormalization.php | Replaced legacy parallel arrays with two pre-computed constant mappings: SEARCH_WORDS_MAPPING (lowercase + special chars) and ACCENT_MAPPING (case-preserving accent removal) |
| src/StringManipulation.php | Removed static caching, simplified searchWords() and removeAccents() to use constants, removed applyBasicNameFix() method and LogicException import |
| tests/Unit/IsValidTimePartTest.php | Added comprehensive test suite for private isValidTimePart() method using reflection to cover defensive validation paths |
| tests/Unit/UppercaseAccentMappingBugFixTest.php | Removed cache reset logic (setUp/tearDown/resetStaticCache) as pre-computed constants eliminate need for cache management |
| tests/Unit/ArrayCombineValidationBugFixTest.php | Removed cache reset logic and updated comments to reflect pre-computed constant usage |
| tests/Unit/CriticalBugFixIntegrationTest.php | Removed cache reset logic and updated documentation to reflect constant-based implementation |
| composer.json | Changed psalm/plugin-phpunit version constraint from >=0.19.3 to ^0.19.3 for consistency with similar dependencies |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Extend CI pipeline to test against PHP 8.5 in addition to 8.3 and 8.4. Signed-off-by: Marjo Wenzel van Lier <marjo.vanlier@gmail.com>
Code Review: PR #58 - Pre-compute character mappings for O(1) lookupSummaryThis is an excellent performance optimization that replaces runtime array operations with compile-time constants. The approach is sound, implementation is clean, and test coverage has been improved to 100%. Strengths1. Performance Improvements
2. Code Quality
3. Test Coverage
4. Documentation
Code Quality ObservationsAccentNormalization.php (src/AccentNormalization.php:27-330)Positive:
StringManipulation.php (src/StringManipulation.php:60-72, 193-196)Excellent simplification:
Removed unnecessary code:
IsValidTimePartTest.phpExcellent test design:
Security ReviewNo security concerns identified:
Performance ConsiderationsExcellent improvements across the board:
The stated improvements (+66-104%) are credible given elimination of:
Potential Issues1. Greek Character Mappings (Minor observation)In AccentNormalization.php:
Question: Are Greek characters in searchWords() meant to be normalized to Greek letters rather than Latin? This differs from other accented characters that map to Latin equivalents. If this is intentional for internationalization, please confirm. 2. Uppercase Ł Mapping Inconsistency (Line 222)In ACCENT_MAPPING, uppercase Ł maps to lowercase l, breaking case preservation. Impact: Low (affects Polish names like ŁUKASZ which would become lUKASZ instead of LUKASZ) Best Practices Compliance
Testing RecommendationsSince you have 100% coverage and 201 passing tests:
DependenciesComposer change (composer.json:61):
Good change: ^0.19.3 follows semantic versioning best practices (allows 0.19.x patches but not 0.20.0 breaking changes) Final VerdictAPPROVED with one minor fix recommended Recommendation:Fix the uppercase Ł mapping in ACCENT_MAPPING (line 222) from l to L Optional:Clarify the intentionality of Greek character mappings in SEARCH_WORDS_MAPPING. Summary Score
Overall: 5/5 Excellent work on this optimization! The code is production-ready with the minor Ł fix applied. |
|
Changelog updates: 🔄 2025-12-08 *Changed
Added
Build
|
Code Review - PR #58: Pre-compute character mappings for O(1) lookupSummaryThis is an excellent performance optimization PR that replaces runtime array operations with compile-time constants. The changes are well-architected, thoroughly tested, and deliver significant measurable improvements. ✅ Strengths1. Performance Improvements
2. Code Quality
3. Test Coverage
4. Architecture
5. Documentation
🔍 Observations & Minor Suggestions1. AccentNormalization.php (Lines 27-82)Observation: The ' ' => ' ', // Line 81This is handled by the regex in return trim(preg_replace('# {2,}#', ' ', $result) ?? '');Suggestion: Consider adding a comment explaining this is a preliminary cleanup before the final regex pass, or potentially remove it since the regex handles all multi-space scenarios. 2. Potential Greek Character Inconsistency (Lines 78-80)In 'Ά' => 'α', 'ά' => 'α', // AlphaHowever, in 'Ά' => 'Α', 'ά' => 'α', // Case-preservingQuestion: Is this intentional? The behavior differs from Latin characters. For example:
But in
This appears correct, but worth confirming the Greek mappings are tested. 3. Minor: Ł vs ł Inconsistency (Line 222)In 'Ł' => 'l', // Line 222 - lowercase 'l'
'ł' => 'l', // Line 223 - lowercase 'l'Should 'Ĺ' => 'L', // Line 214 - preserves uppercase
'ĺ' => 'l', // Line 215 - lowercaseImpact: Very minor - only affects case preservation for Polish names with Ł. 4. Dependency Update (composer.json)Changed from "psalm/plugin-phpunit": "^0.19.3"Question: Was this intentional or a side effect? The 🛡️ Security & Safety
🧪 Testing & Static AnalysisAs per your PR description:
Recommendation: Before merging, run the full test suite as specified in CLAUDE.md: docker-compose run --rm test-all📊 Performance Impact AssessmentBefore (Runtime Initialization):// First call: lazy init + operation
// Subsequent calls: operation onlyAfter (Compile-Time Constants):// All calls: operation only (no init overhead)Memory: Constant data is stored in opcache - no per-request allocation 🎯 Final VerdictAPPROVE ✅This is a textbook example of effective optimization:
Action Items Before Merge:
Recommendation:Merge after addressing the minor Ł case preservation issue (or confirming it's intentional). The other observations are informational and don't block the merge. 💡 Future Optimization IdeasFor consideration in future PRs:
Great work on this optimization! The performance gains are significant and the implementation is clean. 🚀 |
Code Review - PR #58: Pre-compute character mappings for O(1) lookupOverall AssessmentThis is an excellent performance optimization PR with well-documented improvements and comprehensive testing. The code quality is high and adheres to the repository standards. The approach of using compile-time constants instead of lazy initialization is sound and shows significant performance gains. Recommendation: ✅ Approve with minor observations Strengths1. Performance Improvements ⭐⭐⭐⭐⭐
2. Code Quality ⭐⭐⭐⭐⭐
3. Test Coverage ⭐⭐⭐⭐⭐
4. Documentation ⭐⭐⭐⭐
Technical Observations✅ Excellent Design Decisions
🔍 Minor Observations
Security Considerations✅ No security concerns identified
Performance Considerations✅ Excellent optimization
Potential micro-optimization (not necessary, but noting for completeness): Testing Assessment✅ Comprehensive and appropriate The new
Test simplification in other files (removing static cache resets) is correct since constants don't need initialization. Code Style Compliance✅ Fully compliant with CLAUDE.md
RecommendationsMust AddressNone - all critical items are already handled correctly. Should Consider
Nice to Have
Checklist
ConclusionThis is a high-quality performance optimization that successfully achieves its goals while maintaining code quality and test coverage. The compile-time constant approach is architecturally sound and the implementation is clean. The only items to verify are the intentional behavior changes (Ł mapping and searchWords() simplification), but these appear to be deliberate improvements based on the test suite passing. Great work! 🚀 |
Psalm does not yet support PHP 8.5. Remove vimeo/psalm and psalm/plugin-phpunit from dependencies while keeping composer scripts for use on PHP 8.3/8.4. - Remove vimeo/psalm and psalm/plugin-phpunit from require-dev - Skip psalm step in CI for PHP 8.5 matrix - Adjust rector step to run after phan when psalm is skipped Signed-off-by: Marjo Wenzel van Lier <marjo.vanlier@gmail.com>
|
Changelog updates: 🔄 2025-12-08 *Changed
Added
CI
|
Code Review - PR #58: Pre-compute character mappings for O(1) lookupSummaryThis PR delivers a well-executed performance optimization by replacing runtime array operations with compile-time constants. The changes are clean, well-tested, and demonstrate measurable performance improvements. ✅ Strengths1. Excellent Performance Improvements
The shift from lazy initialization to pre-computed constants eliminates first-call overhead and runtime array construction. This is a textbook optimization. 2. Code Quality
3. Test Coverage Excellence
4. Simplified ImplementationThe refactoring from lazy initialization to constants is clean and removes complexity. 5. Documentation UpdatesThe 🔍 Areas for Consideration1. Minor: Duplicate Mapping EntryBoth constants include Impact: Very low - serves no functional purpose but does not hurt. 2. Minor: Greek Character Mapping Inconsistency
3. Minor: Psalm Removal
4. Test Naming ConventionSome tests use "HappyFlow" for negative cases (e.g., Impact: Low - purely naming convention. 5. Security: No Concerns ✅
🎯 RecommendationsHigh Priority✅ None - Ready to merge from a functional perspective. Low Priority (Future Enhancement)
📊 Performance Analysis
Why this works:
✅ Final VerdictAPPROVED ✨ This is a high-quality performance optimization that:
The minor issues are cosmetic and do not block merging. Excellent work! 📝 Checklist Verification (per CLAUDE.md)
Great work! 🚀 Reviewed with: Claude Code (Sonnet 4.5) |
Restore vimeo/psalm dependency so it runs on PHP 8.3/8.4. Use --ignore-platform-req=php when installing on PHP 8.5 to allow packages that don't yet declare 8.5 support. Signed-off-by: Marjo Wenzel van Lier <marjo.vanlier@gmail.com>
|
Changelog updates: 🔄 2025-12-08 *Added
Changed
Fixed
|
Pull Request Review - PR #58SummaryThis PR implements a significant performance optimization by replacing runtime array operations with compile-time constants. The changes achieve impressive performance gains (66-104% improvements) while maintaining 100% code coverage and passing all quality checks. ✅ Strengths1. Excellent Performance Optimization
2. Code Quality
3. Testing Excellence
4. Future-Proofing
🔍 Code Review FindingsAccentNormalization.phpSEARCH_WORDS_MAPPING constant: ✅ Strengths:
This is not a bug - Impact: Negligible - doesn't affect correctness or performance. StringManipulation.php✅ Excellent simplification:
✅ removeAccents() perfection:
IsValidTimePartTest.php✅ Comprehensive defensive testing:
Best Practice: Using Reflection to achieve 100% coverage of defensive code paths. CI/CD Changes✅ PHP 8.5 support handled properly:
Excellent forward compatibility work. Documentation✅ Accurate benchmark updates:
🔒 Security Analysis✅ No security concerns identified:
🐛 Potential IssuesNone Critical - Only Minor Observations:
📊 Performance Considerations✅ Excellent optimization strategy:
Benchmark validation recommendation: Consider adding simple benchmark tests to detect performance regressions in future changes. 🧪 Test Coverage✅ Exemplary test coverage:
📝 RecommendationsOptional Improvements (non-blocking):
✨ Final AssessmentOverall Rating: ⭐⭐⭐⭐⭐ Excellent This is a high-quality performance optimization PR that demonstrates:
Verdict: ✅ APPROVED - Ready to MergeJustification:
Congratulations on an excellent optimization! 🎉 Reviewed by: Claude Code |
Remove vimeo/psalm from dependencies due to PHP 8.5 incompatibility. Psalm does not yet support PHP 8.5 and was causing CI failures on the 8.5 matrix. Changes: - Remove vimeo/psalm from require-dev - Remove @test:psalm from tests sequence - Remove @test:psalm from analyse:all - Remove psalm step from CI workflow - Simplify rector step condition Script definitions retained for manual use on PHP 8.3/8.4. Signed-off-by: Marjo Wenzel van Lier <marjo.vanlier@gmail.com>
|
Changelog updates: 🔄 2025-12-08 *Added
Changed
Removed
|
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/Unit/ArrayCombineValidationBugFixTest.php (1)
85-97: Align test description with current behaviour (no LogicException path any more)The docblock still refers to “LogicException validation logic”, but the method now only checks that
searchWords('café')/removeAccents('café')behave correctly without any exception expectations. Consider renaming the test or updating the description to reflect that the previous fatal/LogicException risk is structurally removed by pre-computed constants, and this test is now a simple behavioural regression check.
♻️ Duplicate comments (1)
src/AccentNormalization.php (1)
33-34: Tidy up quote/dash normalisation and fix Ł case-preserving mappingThere are a couple of small inconsistencies left in the mappings that previous reviewers have already pointed out:
Missing U+2018 and en/em dash handling
For better normalisation, it helps to treat the left single quote and common dashes consistently with their ASCII counterparts. At the moment only U+2019/201C/201D are handled; U+2018, U+2013, and U+2014 fall through unchanged.For
SEARCH_WORDS_MAPPING, mapping the left quote to an apostrophe and the dashes to spaces keeps tokenisation consistent with how other punctuation is handled:
"\u{2019}" => "'", "\u{201C}" => '', "\u{201D}" => '',
"\u{2018}" => "'", "\u{2019}" => "'", "\u{201C}" => '', "\u{201D}" => '',"\u{2013}" => ' ', "\u{2014}" => ' ',For `ACCENT_MAPPING`, mapping the left quote to an apostrophe and the dashes to a simple hyphen keeps visual structure without leaking odd Unicode punctuation into normalised output: ```diff
"\u{2019}" => "'",
"\u{2018}" => "'","\u{2019}" => "'",@@
"\u{201C}" => '',"\u{201D}" => '',
"\u{201C}" => '',"\u{201D}" => '',"\u{2013}" => '-',"\u{2014}" => '-',
Case-preserving Ł mapping in ACCENT_MAPPING
The doc forACCENT_MAPPINGsays it “preserves original case (uppercase accents map to uppercase, lowercase to lowercase)”, but the current mapping uses'Ł' => 'l', which breaks that rule. The lowercase mapping inSEARCH_WORDS_MAPPINGis correct, but the accent-removal mapping should stay uppercase:
'Ł' => 'l',
'Ł' => 'L',This will ensure something like `"Łódź"` normalises to `"Lodz"` via `removeAccents()` while still lowercasing correctly via `searchWords()`.These are relatively small changes but they bring the mappings fully in line with the documented intent and previous review feedback.
Also applies to: 91-329
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (3)
.github/workflows/php.ymlis excluded by none and included by nonecomposer.jsonis excluded by none and included by nonedocs/performance.mdis excluded by none and included by none
📒 Files selected for processing (6)
src/AccentNormalization.php(1 hunks)src/StringManipulation.php(3 hunks)tests/Unit/ArrayCombineValidationBugFixTest.php(2 hunks)tests/Unit/CriticalBugFixIntegrationTest.php(1 hunks)tests/Unit/IsValidTimePartTest.php(1 hunks)tests/Unit/UppercaseAccentMappingBugFixTest.php(1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.php
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.php: PHP version must be >=8.3.0
Always use strict typing withdeclare(strict_types=1);at the beginning of files
Follow PSR guidelines with Laravel Pint code style (preset 'per')
Files:
tests/Unit/CriticalBugFixIntegrationTest.phpsrc/StringManipulation.phpsrc/AccentNormalization.phptests/Unit/UppercaseAccentMappingBugFixTest.phptests/Unit/ArrayCombineValidationBugFixTest.phptests/Unit/IsValidTimePartTest.php
⚙️ CodeRabbit configuration file
**/*.php: Review PHP code for adherence to PER Coding Style 2.0 guidelines.
Ensure proper namespace usage, code organisation, and separation
of concerns. Verify that SOLID principles are followed and
encourage FOOP techniques—such as immutable data, pure functions,
and functional composition—to improve maintainability,
testability, and performance.Specific checks:
- Strict typing:
declare(strict_types=1);is required- Explicit type declarations for all parameters and return types
- Final classes with static methods where appropriate
- Comprehensive docblocks with @param, @return, and @example tags
- No methods exceeding 100 lines (PHPMD rule)
- PHP 8.3+ features and patterns
- Proper error handling and null safety
Files:
tests/Unit/CriticalBugFixIntegrationTest.phpsrc/StringManipulation.phpsrc/AccentNormalization.phptests/Unit/UppercaseAccentMappingBugFixTest.phptests/Unit/ArrayCombineValidationBugFixTest.phptests/Unit/IsValidTimePartTest.php
tests/**/*.php
📄 CodeRabbit inference engine (CLAUDE.md)
Use Pest PHP for testing with complete code coverage requirements
New code must have corresponding PHPUnit tests. Critical features require 100% line coverage; standard features require 90% coverage
Files:
tests/Unit/CriticalBugFixIntegrationTest.phptests/Unit/UppercaseAccentMappingBugFixTest.phptests/Unit/ArrayCombineValidationBugFixTest.phptests/Unit/IsValidTimePartTest.php
⚙️ CodeRabbit configuration file
tests/**/*.php: Review test code for:
- TDD compliance (tests should be clear and comprehensive)
- PHPUnit best practices
- 100% coverage for critical paths, 90%+ for standard code
- Fast execution (unit tests <100ms, integration <5s)
- Independent, deterministic tests
- Descriptive test names and clear assertions
- Proper mocking and test isolation
Files:
tests/Unit/CriticalBugFixIntegrationTest.phptests/Unit/UppercaseAccentMappingBugFixTest.phptests/Unit/ArrayCombineValidationBugFixTest.phptests/Unit/IsValidTimePartTest.php
src/**/*.php
📄 CodeRabbit inference engine (CLAUDE.md)
src/**/*.php: Use PSR-4 namespace:MarjovanLier\StringManipulation
Classes must be final and prefer static methods
Use typed class constants (e.g.,private const array FOO = [])
Write comprehensive docblocks for public methods with @param, @return and @example tags
Always use explicit parameter typing and return types in PHP methods
Use PHP 8 attributes like#[SensitiveParameter]where appropriate
Use explicit null checks for null handling; optional parameters should default to empty string
Maintain 100% method documentation coverage with examples
PHP methods must not exceed 100 lines according to PHPMD rules
src/**/*.php: Production code (src/ directory) must not contain debugging functions: var_dump(), var_export(), print_r(), dd(), or dump()
All PHP files in src/ must begin withdeclare(strict_types=1);after the opening PHP tag and before the namespace declaration
Files:
src/StringManipulation.phpsrc/AccentNormalization.php
🧠 Learnings (1)
📚 Learning: 2025-12-08T21:30:38.456Z
Learnt from: CR
Repo: MarjovanLier/StringManipulation PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-08T21:30:38.456Z
Learning: Applies to src/**/*.php : Use PSR-4 namespace: `MarjovanLier\StringManipulation`
Applied to files:
src/StringManipulation.phptests/Unit/IsValidTimePartTest.php
🧬 Code graph analysis (2)
tests/Unit/ArrayCombineValidationBugFixTest.php (1)
src/StringManipulation.php (2)
StringManipulation(31-401)searchWords(60-72)
tests/Unit/IsValidTimePartTest.php (1)
src/StringManipulation.php (1)
StringManipulation(31-401)
🪛 PHPMD (2.15.0)
tests/Unit/ArrayCombineValidationBugFixTest.php
48-48: Avoid using static access to class '\MarjovanLier\StringManipulation\StringManipulation' in method 'testArrayCombineValidationStaticCachingHappyFlow'. (undefined)
(StaticAccess)
🔇 Additional comments (7)
tests/Unit/UppercaseAccentMappingBugFixTest.php (1)
10-17: Docblock correctly reflects pre-computed SEARCH_WORDS_MAPPING approachThe updated FIX note aligns with the new constant-based implementation and the tests still robustly cover upper-case accent behaviour in both
searchWords()andremoveAccents(). Nothing further needed here.tests/Unit/CriticalBugFixIntegrationTest.php (1)
10-15: Integration-test description matches constant-based implementationThe note about using pre-computed constants and dropping cache resets correctly reflects the current design; the tests exercise both
searchWords()andremoveAccents()together without depending on internal state. All good.tests/Unit/ArrayCombineValidationBugFixTest.php (2)
10-15: High-level description matches removal of runtime array_combineThe updated FIX line correctly explains that pre-computed mappings remove the need for runtime
array_combine()and associated validation, which fits the newsearchWords()/removeAccents()implementations. No changes needed here.
47-61: Static-caching test comments now accurately describe constant-based behaviourThe revised comments in
testArrayCombineValidationStaticCachingHappyFlow()correctly state that both calls use the same pre-computed constant rather than building or resetting a cache, which keeps the regression intent clear. Static access toStringManipulationis consistent with the static-utility design, so the PHPMD StaticAccess warning can be safely ignored here.src/StringManipulation.php (2)
47-72: searchWords refactor to pre-computed strtr mapping looks solidUsing
strtr(trim($words), self::SEARCH_WORDS_MAPPING)followed by a single whitespace normalisation pass keepssearchWords()pure and O(n), while matching the intended behaviour (null passthrough, lowercasing, accent removal, and space collapsing). The updated docblock correctly explains the constant-based approach and there’s no observable regression given the existing tests.
183-196: removeAccents now cleanly delegates to ACCENT_MAPPINGSwitching
removeAccents()to a singlestrtr($str, self::ACCENT_MAPPING)call aligns it with the new pre-computed mapping strategy and removes all runtime array work. The method remains pure and side-effect free, and the docblock’s reference toACCENT_MAPPINGkeeps the contract clear.src/AccentNormalization.php (1)
21-82: Pre-computed mappings and typed constants are a good fit hereThe move to
private const array SEARCH_WORDS_MAPPINGwith an explicit@var array<string,string>annotation gives you immutable, fast lookups and keeps all searchWords-related normalisation data centralised. The combination of ASCII upper-to-lower mapping, accent folding, and a small set of punctuation rules reads clearly and matches howStringManipulation::searchWords()now operates. As per coding guidelines, this is a nice FOOP-style, data-driven design.
| /** | ||
| * Test that isValidTimePart returns false for invalid date parts. | ||
| * | ||
| * This covers line 334: checkdate() failure path. | ||
| * | ||
| * @throws ReflectionException | ||
| */ | ||
| public function testIsValidTimePartInvalidDateHappyFlow(): void | ||
| { | ||
| $reflectionMethod = $this->getIsValidTimePartMethod(); | ||
|
|
||
| // February 30th is not a valid date | ||
| $invalidDateParts = [ | ||
| 'year' => 2023, | ||
| 'month' => 2, | ||
| 'day' => 30, | ||
| 'hour' => 12, | ||
| 'minute' => 30, | ||
| 'second' => 45, | ||
| ]; | ||
|
|
||
| self::assertFalse($reflectionMethod->invoke(null, $invalidDateParts)); | ||
| } | ||
|
|
||
| /** | ||
| * Test that isValidTimePart returns false for invalid hour. | ||
| * | ||
| * This covers line 339: hour validation failure path. | ||
| * | ||
| * @throws ReflectionException | ||
| */ | ||
| public function testIsValidTimePartInvalidHourHappyFlow(): void | ||
| { | ||
| $reflectionMethod = $this->getIsValidTimePartMethod(); | ||
|
|
||
| // Hour 25 is invalid | ||
| $invalidTimeParts = [ | ||
| 'year' => 2023, | ||
| 'month' => 9, | ||
| 'day' => 6, | ||
| 'hour' => 25, | ||
| 'minute' => 30, | ||
| 'second' => 45, | ||
| ]; | ||
|
|
||
| self::assertFalse($reflectionMethod->invoke(null, $invalidTimeParts)); | ||
| } | ||
|
|
||
| /** | ||
| * Test that isValidTimePart returns false for invalid minute. | ||
| * | ||
| * This also covers line 339: minute validation failure path. | ||
| * | ||
| * @throws ReflectionException | ||
| */ | ||
| public function testIsValidTimePartInvalidMinuteHappyFlow(): void | ||
| { | ||
| $reflectionMethod = $this->getIsValidTimePartMethod(); | ||
|
|
||
| // Minute 60 is invalid | ||
| $invalidTimeParts = [ | ||
| 'year' => 2023, | ||
| 'month' => 9, | ||
| 'day' => 6, | ||
| 'hour' => 12, | ||
| 'minute' => 60, | ||
| 'second' => 45, | ||
| ]; | ||
|
|
||
| self::assertFalse($reflectionMethod->invoke(null, $invalidTimeParts)); | ||
| } | ||
|
|
||
| /** | ||
| * Test that isValidTimePart returns true for valid date parts. | ||
| * | ||
| * @throws ReflectionException | ||
| */ | ||
| public function testIsValidTimePartValidPartsHappyFlow(): void | ||
| { | ||
| $reflectionMethod = $this->getIsValidTimePartMethod(); | ||
|
|
||
| // Valid date and time parts | ||
| $validParts = [ | ||
| 'year' => 2023, | ||
| 'month' => 9, | ||
| 'day' => 6, | ||
| 'hour' => 12, | ||
| 'minute' => 30, | ||
| 'second' => 45, | ||
| ]; | ||
|
|
||
| self::assertTrue($reflectionMethod->invoke(null, $validParts)); | ||
| } | ||
|
|
||
| /** | ||
| * Test edge case: February 29 in a leap year is valid. | ||
| * | ||
| * @throws ReflectionException | ||
| */ | ||
| public function testIsValidTimePartLeapYearHappyFlow(): void | ||
| { | ||
| $reflectionMethod = $this->getIsValidTimePartMethod(); | ||
|
|
||
| // Feb 29, 2024 is valid (2024 is a leap year) | ||
| $leapYearParts = [ | ||
| 'year' => 2024, | ||
| 'month' => 2, | ||
| 'day' => 29, | ||
| 'hour' => 0, | ||
| 'minute' => 0, | ||
| 'second' => 0, | ||
| ]; | ||
|
|
||
| self::assertTrue($reflectionMethod->invoke(null, $leapYearParts)); | ||
| } | ||
|
|
||
| /** | ||
| * Test edge case: February 29 in a non-leap year is invalid. | ||
| * | ||
| * @throws ReflectionException | ||
| */ | ||
| public function testIsValidTimePartNonLeapYearNegativeFlow(): void | ||
| { | ||
| $reflectionMethod = $this->getIsValidTimePartMethod(); | ||
|
|
||
| // Feb 29, 2023 is invalid (2023 is not a leap year) | ||
| $nonLeapYearParts = [ | ||
| 'year' => 2023, | ||
| 'month' => 2, | ||
| 'day' => 29, | ||
| 'hour' => 12, | ||
| 'minute' => 0, | ||
| 'second' => 0, | ||
| ]; | ||
|
|
||
| self::assertFalse($reflectionMethod->invoke(null, $nonLeapYearParts)); | ||
| } | ||
|
|
||
| /** | ||
| * Test edge case: month 0 is invalid. | ||
| * | ||
| * @throws ReflectionException | ||
| */ | ||
| public function testIsValidTimePartMonthZeroNegativeFlow(): void | ||
| { | ||
| $reflectionMethod = $this->getIsValidTimePartMethod(); | ||
|
|
||
| // Month 0 is invalid | ||
| $invalidMonthParts = [ | ||
| 'year' => 2023, | ||
| 'month' => 0, | ||
| 'day' => 15, | ||
| 'hour' => 12, | ||
| 'minute' => 30, | ||
| 'second' => 0, | ||
| ]; | ||
|
|
||
| self::assertFalse($reflectionMethod->invoke(null, $invalidMonthParts)); | ||
| } | ||
|
|
||
| /** | ||
| * Test edge case: month 13 is invalid. | ||
| * | ||
| * @throws ReflectionException | ||
| */ | ||
| public function testIsValidTimePartMonthThirteenNegativeFlow(): void | ||
| { | ||
| $reflectionMethod = $this->getIsValidTimePartMethod(); | ||
|
|
||
| // Month 13 is invalid | ||
| $invalidMonthParts = [ | ||
| 'year' => 2023, | ||
| 'month' => 13, | ||
| 'day' => 15, | ||
| 'hour' => 12, | ||
| 'minute' => 30, | ||
| 'second' => 0, | ||
| ]; | ||
|
|
||
| self::assertFalse($reflectionMethod->invoke(null, $invalidMonthParts)); | ||
| } | ||
|
|
||
| /** | ||
| * Test boundary: hour -1 is invalid. | ||
| * | ||
| * @throws ReflectionException | ||
| */ | ||
| public function testIsValidTimePartNegativeHourNegativeFlow(): void | ||
| { | ||
| $reflectionMethod = $this->getIsValidTimePartMethod(); | ||
|
|
||
| // Negative hour is invalid | ||
| $invalidHourParts = [ | ||
| 'year' => 2023, | ||
| 'month' => 9, | ||
| 'day' => 6, | ||
| 'hour' => -1, | ||
| 'minute' => 30, | ||
| 'second' => 0, | ||
| ]; | ||
|
|
||
| self::assertFalse($reflectionMethod->invoke(null, $invalidHourParts)); | ||
| } | ||
|
|
||
| /** | ||
| * Test boundary: minute -1 is invalid. | ||
| * | ||
| * @throws ReflectionException | ||
| */ | ||
| public function testIsValidTimePartNegativeMinuteNegativeFlow(): void | ||
| { | ||
| $reflectionMethod = $this->getIsValidTimePartMethod(); | ||
|
|
||
| // Negative minute is invalid | ||
| $invalidMinuteParts = [ | ||
| 'year' => 2023, | ||
| 'month' => 9, | ||
| 'day' => 6, | ||
| 'hour' => 12, | ||
| 'minute' => -1, | ||
| 'second' => 0, | ||
| ]; | ||
|
|
||
| self::assertFalse($reflectionMethod->invoke(null, $invalidMinuteParts)); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Good coverage of isValidTimePart branches; consider adding an invalid-second case
These tests neatly cover invalid dates, hour/minute boundaries, leap-year rules, and various month edge cases, giving solid confidence in isValidTimePart()’s defensive logic. One gap is the scenario where date/hour/minute are valid but second is out of range, which would exercise the false path in isValidSecond() via isValidTimePart().
If you want to close that last hole, you could add something like:
@@
/**
* Test boundary: minute -1 is invalid.
*
* @throws ReflectionException
*/
public function testIsValidTimePartNegativeMinuteNegativeFlow(): void
{
$reflectionMethod = $this->getIsValidTimePartMethod();
@@
self::assertFalse($reflectionMethod->invoke(null, $invalidMinuteParts));
}
+
+ /**
+ * Test that an out-of-range second is invalid while date/hour/minute are valid.
+ *
+ * @throws ReflectionException
+ */
+ public function testIsValidTimePartInvalidSecondNegativeFlow(): void
+ {
+ $reflectionMethod = $this->getIsValidTimePartMethod();
+
+ $invalidSecondParts = [
+ 'year' => 2023,
+ 'month' => 9,
+ 'day' => 6,
+ 'hour' => 12,
+ 'minute' => 30,
+ 'second' => 60,
+ ];
+
+ self::assertFalse($reflectionMethod->invoke(null, $invalidSecondParts));
+ }That would complete the set of boundary tests for all three time components.
🤖 Prompt for AI Agents
In tests/Unit/IsValidTimePartTest.php around lines 22 to 246, add a test that
covers the missing invalid-second case: create a new test method that gets the
private isValidTimePart via reflection, builds a time-parts array where
year/month/day/hour/minute are valid but 'second' is out of range (e.g., 60 or
-1), invokes the reflection method and assertsFalse on the result; keep the
style and annotations consistent with the other tests (ReflectionException
docblock) so the test suite exercises the isValidSecond() failure path through
isValidTimePart().
| /** | ||
| * Get the isValidTimePart method via reflection. | ||
| * | ||
| * @throws ReflectionException | ||
| * | ||
| * @return \ReflectionMethod | ||
| */ | ||
| private function getIsValidTimePartMethod(): \ReflectionMethod | ||
| { | ||
| $reflectionClass = new ReflectionClass(StringManipulation::class); | ||
|
|
||
| return $reflectionClass->getMethod('isValidTimePart'); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Reflection helper is clear; minor consistency tweak possible
getIsValidTimePartMethod() is straightforward and keeps the reflection logic in one place. For consistency with the use imports, you could also import ReflectionMethod at the top and use the short name in the return type and docblock, but this is purely cosmetic and optional.
🤖 Prompt for AI Agents
In tests/Unit/IsValidTimePartTest.php around lines 248 to 260, the reflection
helper uses fully-qualified \ReflectionMethod in the docblock and return type;
for consistency with existing use imports, add a use ReflectionMethod; at the
top of the file and update the docblock and the method signature to reference
ReflectionMethod (short name) instead of the FQCN.
Code Review: Pre-compute character mappings for O(1) lookupSummaryThis PR delivers an excellent performance optimization by replacing runtime array operations with compile-time constants. The refactoring is well-executed with strong test coverage improvements. Overall, this is a high-quality PR that I recommend for approval with minor considerations noted below. ✅ Strengths1. Performance Architecture
2. Code Quality
3. Test Coverage
4. Dependency Management
|
| Aspect | Rating | Notes |
|---|---|---|
| Code Quality | ⭐⭐⭐⭐⭐ | Excellent refactoring, cleaner implementation |
| Performance | ⭐⭐⭐⭐⭐ | Significant measurable improvements |
| Test Coverage | ⭐⭐⭐⭐⭐ | 100% coverage, comprehensive scenarios |
| Security | ⭐⭐⭐⭐⭐ | No concerns identified |
| Documentation | ⭐⭐⭐⭐☆ | Good, minor CLAUDE.md update needed |
| Maintainability | ⭐⭐⭐⭐⭐ | Simpler code, removed technical debt |
Overall: ⭐⭐⭐⭐⭐ (4.8/5)
✅ Approval Recommendation
APPROVED with minor documentation fix for CLAUDE.md.
This PR represents a textbook example of performance optimization:
- Measurable improvements with benchmarks
- Maintains correctness with 100% test coverage
- Simplifies implementation
- Removes technical debt
- Well-documented changes
Excellent work! 🎉
Remove phan/phan from dependencies due to PHP 8.5 incompatibility. Phan crashes when parsing vendor files on PHP 8.5 due to deprecated syntax handling. Changes: - Remove phan/phan from require-dev - Remove @test:phan from tests sequence - Remove @test:phan from analyse:all - Remove phan step from CI workflow - Remove test-phan and test-psalm services from docker - Update test-all command in docker-compose Script definitions retained for manual use on PHP 8.3/8.4. Signed-off-by: Marjo Wenzel van Lier <marjo.vanlier@gmail.com>
|
Changelog updates: 🔄 2025-12-08 *Changed
Added
Removed
|
Pull Request ReviewThis PR implements a significant performance optimization by replacing runtime array operations with compile-time constants. Here is my comprehensive review: ✅ Code Quality & Best PracticesExcellent Work:
Observations:
🐛 Potential IssuesMinor Issue - Polish Ł:The uppercase Polish Ł character mapping may not preserve case as intended: // Current (line 222):
Ł => l // Uppercase -> lowercase
// Should arguably be:
Ł => L // Uppercase -> uppercase (to match pattern)However, checking the old code, this was the original behavior, so this PR does not introduce the issue. If this is intentional for linguistic reasons (Polish Ł has complex rules), then it is fine. No Critical Bugs Detected⚡ Performance ConsiderationsStrengths:
Potential Concerns:
🔒 Security ConcernsNo security issues identified.
🧪 Test CoverageExcellent additions:
Test Plan Verification:✅ 201 tests with 889 assertions 📝 Documentation & ConfigurationPositive changes:
Recommendations:
📊 Overall AssessmentScore: 9.5/10This is an excellent performance optimization PR that: ✅ Delivers substantial performance improvements (66-104%) Minor Recommendation:Verify the Polish Ł mapping ( Final Verdict: ✅ APPROVEThis PR is ready to merge. The performance gains are significant, the implementation is clean, and test coverage is comprehensive. Great work on the optimization! |
User description
Summary
significant performance improvements
SEARCH_WORDS_MAPPINGconstant with lowercase accent mappingsACCENT_MAPPINGconstant for case-preserving accent removalsearchWords()andremoveAccents()to singlestrtr()calls$searchWordsMapping,applyBasicNameFix(),legacy
REMOVE_ACCENTS_FROM/TOarraysIsValidTimePartTestfor 100% code coveragePerformance Improvements
removeAccents()searchWords()nameFix()Test Results
Test Plan
IsValidTimePartTestcovers defensive validation codePR Type
Enhancement, Performance
Description
Replace runtime array operations with compile-time constants for O(1) lookup
Pre-compute SEARCH_WORDS_MAPPING and ACCENT_MAPPING constants eliminating lazy initialization
Simplify searchWords() and removeAccents() to single strtr() calls with constants
Remove unused static properties, applyBasicNameFix() method, and legacy array constants
Add IsValidTimePartTest for 100% code coverage of defensive validation
Remove Psalm and Phan dependencies, add PHP 8.5 to CI test matrix
Diagram Walkthrough
File Walkthrough
2 files
Replace legacy arrays with pre-computed mapping constantsSimplify methods to use pre-computed constants, remove lazy init4 files
Remove static cache reset, simplify test setupRemove cache reset logic, update test commentsAdd comprehensive tests for isValidTimePart methodRemove static cache reset, simplify test setup2 files
Add PHP 8.5 to test matrix, remove Psalm and Phan stepsRemove Psalm and Phan test service definitions1 files
Remove Psalm and Phan dependencies for PHP 8.5 compatibility1 files
Update benchmarks and documentation for pre-computed constantsSummary by CodeRabbit
Bug Fixes
Tests
Refactor
✏️ Tip: You can customize this high-level summary in your review settings.