refactor(api): clear residual duplication to 0% (#243) - #406
Merged
Conversation
Drives orbit-api SonarCloud duplicated_lines_density 0.1% -> 0.0% (#404 deferred these files to avoid a concurrent-edit conflict). Real extractions (genuinely-shared logic): - HabitScheduleService: the identical active-days filter + frequency-unit switch tail of IsHabitDueOnDate and IsHabitHistoricallyDueOnDate is extracted into one private MatchesFrequency helper (self-dup ~24 lines). - LevelDefinitions.SyncLevel(User): the GetLevelForXp + SetLevel level-sync idiom, previously copy-pasted in 6 places (both accept commands, SendCheer, ChallengeProgressService, JoinChallenge, and GamificationService.UpdateLevel), is unified into one canonical helper; UpdateLevel is deleted. Justified exclusion (irreducible parallel mirror): - AcceptAccountabilityPairCommand / AcceptFriendRequestCommand: after the shared level-sync is extracted, the only residual is the BuildRequesterNotification parallel mirror -- a null-guard + isPortuguese ternary skeleton emitting DISTINCT localized copy for two independent accept events in separate bounded contexts. CPD normalizes the differing string literals so the skeletons match; a shared builder would couple the two contexts without improving readability. Added to sonar.cpd.exclusions with rationale, consistent with the existing #404 exclusions. Behavior-preserving: build 0 errors, all tests green, no EF model changes, openapi.json unchanged. Refs thomasluizon/orbit-ui-mobile#243 (duplication -> 0) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
There was a problem hiding this comment.
PR Review: #406 — refactor(api): clear residual duplication to 0% (#243)
Recommendation: APPROVE
| Severity | Count |
|---|---|
| Critical (incl. old-client breaks) | 0 |
| High | 0 |
| Medium | 0 |
| Low / Info | 2 |
What was reviewed
A pure, behavior-preserving DRY refactor across 9 files:
src/Orbit.Application/Habits/Services/HabitScheduleService.cs— extracts the identical active-days + frequency-unit tail shared byIsHabitDueOnDate/IsHabitHistoricallyDueOnDateinto a privateMatchesFrequencyhelper.src/Orbit.Application/Gamification/LevelDefinitions.cs+ 6 call sites (AcceptAccountabilityPairCommand.cs,JoinChallengeCommand.cs,ChallengeProgressService.cs,GamificationService.csx4,AcceptFriendRequestCommand.cs,SendCheerCommand.cs) — unifies the copy-pasted "recompute level after XP" block intoLevelDefinitions.SyncLevel(User), deletes the now-redundant privateGamificationService.UpdateLevel..github/workflows/sonarcloud.yml— adds a documentedsonar.cpd.exclusionsentry for the one irreducible residual (BuildRequesterNotificationin the accountability/friend-request accept commands — same skeleton, genuinely distinct localized copy).
Verification performed
- Diffed every extraction byte-for-byte against the pre-diff inline code — confirmed identical logic, no altered condition.
- Verified all 6
LevelDefinitions.SyncLevelcall sites operate on the correctuser/senderreference (no cross-user XP/level bleed) — cross-checked independently by thesecurity-reviewersubagent (verdict: PASS). contract-alignercorrectly gated N/A — no DTO/Controller/packages/sharedsurface touched.- Repo-wide grep confirmed zero remaining references to deleted
UpdateLeveland zero test references to the refactored private symbols — no test breakage from the code motion. - Read the actual
BuildRequesterNotificationbodies in both commands and confirmed the CPD-exclusion rationale in the PR body/workflow comment is accurate, not metric-gamed.
Findings (Info only, no action required)
- Faithful Extract Method, well past the "third use" DRY bar (
LevelDefinitions.cs:80-85,HabitScheduleService.cs:500-513). - CPD exclusion is honestly justified (
.github/workflows/sonarcloud.yml:94-100), not a workaround — verified against actual code.
Deferred (N/A dimensions)
DESIGN.md/AI-slop, Parity, i18n, Contract drift, FEATURES.md parity — all N/A, no frontend/contract/user-facing-feature surface in this diff. dotnet build/dotnet test skipped per CI-wrapper convention (Build / Unit Tests / SonarCloud run as separate required checks on this PR).
Subagents
| Agent | Verdict |
|---|---|
| security-reviewer | PASS |
| contract-aligner | N/A (no contract surface touched) |
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
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.



What & why
Drives orbit-api SonarCloud
duplicated_lines_densityfrom 0.1% → 0.0% (the last ~54 duplicated lines), satisfying #243's "Duplicated code → 0%". #404 deferred these files to avoid a concurrent-edit conflict; they are now free.Each block was verified against SonarCloud's authoritative
api/duplications/showbefore touching it, and handled per the honest extract-vs-exclude rule.Real extractions (genuinely-shared logic)
HabitScheduleService self-duplication (~24 lines).
IsHabitDueOnDateandIsHabitHistoricallyDueOnDatehad an identical 12-line tail (active-days filter + frequency-unitswitch). Extracted into one privateMatchesFrequency(habit, target, anchor, unit, qty)helper, called from both. The methods still differ in their own earliest-date gate; only the shared tail is de-duped.Level-sync idiom (6 copies → 1 helper). The
GetLevelForXp+SetLevel"recompute level after awarding XP" block was copy-pasted in 6 places: both accept commands,SendCheerCommand,ChallengeProgressService,JoinChallengeCommand, andGamificationService.UpdateLevel. Unified into one canonicalLevelDefinitions.SyncLevel(User)(co-located withGetLevelForXp); the now-redundant privateUpdateLevelis deleted. Well past the third-use bar — real DRY, not metric-gaming.Justified exclusion (irreducible parallel mirror)
AcceptAccountabilityPairCommand ↔ AcceptFriendRequestCommand. After the shared level-sync tail is extracted, the only residual is the
BuildRequesterNotificationparallel mirror: anull-guard +isPortugueseternary skeleton that emits distinct localized copy for two independent accept events (accountability invite vs friend request) in two separate bounded contexts. CPD normalizes the differing string literals so the skeletons match; a shared builder would couple Accountability↔Social and leave CPD-flagged per-event title/body ternaries at each call site without improving readability. Added tosonar.cpd.exclusionswith a documented rationale, consistent with the existing #404 exclusions (which are reserved for irreducible mirrors where the shared logic is already extracted).Before / after
duplicated_lines_densityVerification (all foreground)
dotnet build Orbit.slnx— 0 errorsdotnet test Orbit.slnx— 6048 passed, 0 faileddotnet ef migrations has-pending-model-changes— false ("No changes have been made to the model")src/Orbit.Api/openapi.json— unchanged//narration in the diff (ORBIT0001-safe)Behavior-preserving throughout — every change is a pure extraction or a config-level exclusion.
Refs thomasluizon/orbit-ui-mobile#243 (duplication → 0)