GH-74: Split the 164-line mapSingleObject() along its phases - #120
Merged
Conversation
The method ran a full pipeline in one body, its phases already marked by comment blocks. It is now an orchestration that reads as the pipeline it is - collect converted values, apply the unknown collector, report missing properties, hydrate - with each phase in its own method. The state the phases hand along - the converted values, the names actually mapped, the diverted unknown keys - travels by return rather than by shared locals, so each method's inputs and outputs are its signature. That is also why collectConvertedValues() returns the unknown keys separately for applyUnknownCollector() rather than merging inline: the merge is a distinct phase, not a tail of the collection loop. 180 lines of body became a 31-line orchestration plus four named methods. Behaviour-neutral: the whole suite was green before this commit's own formatting pass. #72 and #73 had already removed the configuration round trip and the per-element reflection this method used to carry, so what remained split cleanly along the existing seams.
The simplicity review pushed back on applyUnknownCollector() as its own method, and the argument held: it re-read metadata->collectorProperty that collectConvertedValues() already has as a local, and it forced that method to return a third tuple element whose only purpose was to be handed straight back for merging. The divert and the merge are two halves of one concern - a key with no declared property is diverted during the loop and the gathered keys merged after it - so they belong in one method. collectConvertedValues() now does both and returns a two-tuple. The orchestration drops to three phases, and the double read is gone. The issue suggested applyUnknownCollector() as a phase name, but 'e.g.' - and a single-caller method re-reading its own input is what KISS ranks below keeping the concern whole. Also trimmed per the review: the orchestration's lead comment no longer narrates the refactor's history, and a docblock sentence that restated an inline comment three lines down is gone.
|
Warning You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again! |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #74.
mapSingleObject()ran a full pipeline in one body, its phases already marked by comment blocks. It is now a 22-line orchestration that reads as the pipeline it is:Each phase is its own method, and the state they hand along — the converted values, the names actually mapped — travels by return rather than by shared locals, so each method's inputs and outputs are its signature.
Three phases, not four
The issue suggested
applyUnknownCollector()as a fourth method. The simplicity review argued against it and I agreed: a 12-line single caller that re-readcollectorProperty(which the collect loop already holds) and forced a third tuple element whose only job was to be handed straight back for merging. The divert and the merge are two halves of one concern — a key with no declared property is diverted during the loop, the gathered keys merged after it — so both live incollectConvertedValues()now.e.g.in the issue, and a single-caller method re-reading its own input is what KISS ranks below keeping the concern whole.Verification
Behaviour-neutral:
mainand this branch produce identical results, confirmed statement-by-statement by review and by the full suite. The collector merge still runs at the same logical point — appending its property to the mapped list before the strict missing-check, so it is never falsely reported missing.Rests on #72 and #73 having already removed the configuration round trip and the per-element reflection this method used to carry, so what remained split cleanly along the existing seams.
composer ci:testgreen, verified by exit code: 379 tests, 1225 assertions; PHPStan max, Rector, CGL and CPD clean.