Align coding-standards docs + AppPrefs with the no-final-on-parameters convention (#200)#201
Merged
Merged
Conversation
The guideline docs said "use final for all method parameters" while the actual convention is final on locals, NOT on parameters (new/edited code; legacy migrates incrementally). Surfaced by the #166 / PR #199 review, where AppPrefs.vibrateEnabled's final param was a hard violation the docs endorsed. - .claude/guidelines.md + CODE_REVIEW_GUIDELINES.md: state the convention (final on locals, not parameters; legacy migrates as touched), add a bad/good example, drop `final` from every parameter in the doc examples, and update the review checklist item. - AppPrefs: drop `final` from all remaining legacy method parameters so the file is internally consistent (it went mixed in #199). `final` on locals unchanged; behaviour unchanged (final on a param is a no-op modifier). testDebugUnitTest + lintDebug + assembleDebug green. Closes #200 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…ms-convention # Conflicts: # app/src/main/java/com/almothafar/simplebatterynotifier/util/AppPrefs.java
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.
Closes #200.
The guideline docs said to use
finalon method parameters, but the actual convention isfinalon locals, not on parameters (new/edited code; legacy migrates incrementally). This surfaced in the two-axis review of #166 / PR #199, whereAppPrefs.vibrateEnabled'sfinalparam was flagged as a hard violation the docs technically endorsed.Changes
Docs — both kept in sync:
.claude/guidelines.md(AI rulebook): the Immutability line now saysfinalon locals, not parameters; notes legacy migrates as touched.CODE_REVIEW_GUIDELINES.md(human-facing): added a bad/good callout in the Variables section, droppedfinalfrom every parameter in the code examples, and updated the review-checklist item (parameters **not** marked final).Code:
AppPrefs: droppedfinalfrom the parameters of all remaining legacy methods (criticalLevel,warningLevel,batteryLevels,setBatteryLevels,drainLimitPph,clampDrainLimit, privateprefs), so the file is internally consistent — a concrete reference application. (Refactor: split NotificationService (channels / quiet-hours / sound / dispatch) + dedupe (#166) #199 had already dropped it on the one new method, leaving the file mixed.)finalon locals is unchanged.Not in scope
A codebase-wide
final-param sweep. Legacy files migrate incrementally as they're touched, per the convention now documented.Verification
testDebugUnitTest+lintDebug+assembleDebuggreen. Removingfinalfrom a parameter is a no-op modifier change — no behaviour change.Acceptance criteria
finalparameter (except the deliberate ❌ BAD sample).AppPrefshas nofinalon parameters;finalon locals unchanged.🤖 Generated with Claude Code