From 10978f93d6263addf012da4f48fff178a2266a17 Mon Sep 17 00:00:00 2001 From: Al-Mothafar Al-Hasan Date: Tue, 21 Jul 2026 18:50:00 +0300 Subject: [PATCH] Docs + AppPrefs: align with the no-final-on-parameters convention (#200) 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 --- .claude/guidelines.md | 2 +- CODE_REVIEW_GUIDELINES.md | 29 ++++++++++++++----- .../simplebatterynotifier/util/AppPrefs.java | 14 ++++----- 3 files changed, 29 insertions(+), 16 deletions(-) diff --git a/.claude/guidelines.md b/.claude/guidelines.md index 5d115aa..d00cdc5 100644 --- a/.claude/guidelines.md +++ b/.claude/guidelines.md @@ -14,7 +14,7 @@ SimpleBatteryNotifier is an Android application that monitors battery status and - Records for simple data carriers - **Resource Management**: Use try-with-resources for automatic resource cleanup - **Null Safety**: Use `isNull()` and `nonNull()` from `java.util.Objects` -- **Immutability**: Use `final` for all method parameters and local variables where possible +- **Immutability**: Use `final` for local variables where possible. Do **not** put `final` on method/constructor parameters in new or edited code — effectively-final already covers lambda capture, so the keyword is just noise. Legacy code carries `final` params (an older convention) and migrates incrementally as methods are otherwise touched; a full sweep isn't required. - **Static Imports**: Import commonly used static methods (e.g., `isNull`, `nonNull`) ### Naming Conventions diff --git a/CODE_REVIEW_GUIDELINES.md b/CODE_REVIEW_GUIDELINES.md index 3a49668..2b46bd6 100644 --- a/CODE_REVIEW_GUIDELINES.md +++ b/CODE_REVIEW_GUIDELINES.md @@ -242,6 +242,19 @@ final int age = 25; **Why?** Improves readability and prevents accidental reassignment. +> **Parameters are the exception — do not mark method/constructor parameters `final`** in new or edited code. Effectively-final already covers lambda capture, so the keyword only adds noise. Legacy code still carries `final` params (an older convention) and migrates as methods are otherwise touched — a full sweep isn't required. +> +> ```java +> // ❌ BAD - final on parameters +> public int criticalLevel(final Context context) { ... } +> +> // ✅ GOOD - final on locals, not parameters +> public int criticalLevel(Context context) { +> final SharedPreferences prefs = getPrefs(context); +> // ... +> } +> ``` + ### 2. Declare Variables Close to Usage ```java @@ -278,7 +291,7 @@ public void process(String input) { } // ✅ GOOD - Early returns -public void process(final String input) { +public void process(String input) { if (isNull(input)) { return; } @@ -308,12 +321,12 @@ public void processUserDataAndSendEmail(User user) { } // ✅ GOOD - Separate concerns -public void processUser(final User user) { +public void processUser(User user) { validateUser(user); saveToDatabase(user); } -public void sendWelcomeEmail(final User user) { +public void sendWelcomeEmail(User user) { // Email logic only } ``` @@ -331,7 +344,7 @@ final NotificationManager manager2 = (NotificationManager) context.getSystemServ manager2.notify(id2, notification2); // ✅ GOOD - Extracted to method -private void sendNotification(final Context context, final int id, final Notification notification) { +private void sendNotification(Context context, int id, Notification notification) { final NotificationManager manager = (NotificationManager) context.getSystemService(Context.NOTIFICATION_SERVICE); if (nonNull(manager)) { manager.notify(id, notification); @@ -371,7 +384,7 @@ public void unusedMethod() { notificationManager.notify(id, notification); // ✅ GOOD - Check permission first (Android 13+) -private boolean hasNotificationPermission(final Context context) { +private boolean hasNotificationPermission(Context context) { if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.TIRAMISU) { return ContextCompat.checkSelfPermission(context, Manifest.permission.POST_NOTIFICATIONS) == PackageManager.PERMISSION_GRANTED; @@ -534,7 +547,7 @@ public void method2() { } // ✅ GOOD - Extract to helper -private NotificationManager getNotificationManager(final Context context) { +private NotificationManager getNotificationManager(Context context) { return (NotificationManager) context.getSystemService(Context.NOTIFICATION_SERVICE); } ``` @@ -603,7 +616,7 @@ private boolean isInDoNotDisturbMode() { * @param context The application context * @param type Notification type (CRITICAL_TYPE, WARNING_TYPE, or FULL_LEVEL_TYPE) */ -public static void sendNotification(final Context context, final int type) { +public static void sendNotification(Context context, int type) { // Implementation } ``` @@ -646,7 +659,7 @@ When reviewing code, check: - [ ] Uses `isNull()`/`nonNull()` instead of `== null`/`!= null` - [ ] All curly brackets present (even single-line) -- [ ] Variables marked `final` where appropriate +- [ ] Locals marked `final` where appropriate; parameters **not** marked `final` - [ ] No FQNs (uses imports) - [ ] Line width ≤ 160 characters - [ ] Early returns instead of deep nesting diff --git a/app/src/main/java/com/almothafar/simplebatterynotifier/util/AppPrefs.java b/app/src/main/java/com/almothafar/simplebatterynotifier/util/AppPrefs.java index a768e08..382bd67 100644 --- a/app/src/main/java/com/almothafar/simplebatterynotifier/util/AppPrefs.java +++ b/app/src/main/java/com/almothafar/simplebatterynotifier/util/AppPrefs.java @@ -54,7 +54,7 @@ private AppPrefs() { * * @return the configured critical level */ - public static int criticalLevel(final Context context) { + public static int criticalLevel(Context context) { return prefs(context).getInt(context.getString(R.string._pref_key_critical_battery_level), DEFAULT_CRITICAL_LEVEL); } @@ -66,7 +66,7 @@ public static int criticalLevel(final Context context) { * * @return the configured warning level */ - public static int warningLevel(final Context context) { + public static int warningLevel(Context context) { return prefs(context).getInt(context.getString(R.string._pref_key_warn_battery_level), DEFAULT_WARNING_LEVEL); } @@ -78,7 +78,7 @@ public static int warningLevel(final Context context) { * * @return the configured {@code (critical, warning)} thresholds */ - public static LevelThresholds batteryLevels(final Context context) { + public static LevelThresholds batteryLevels(Context context) { return new LevelThresholds(criticalLevel(context), warningLevel(context)); } @@ -89,7 +89,7 @@ public static LevelThresholds batteryLevels(final Context context) { * @param context Application context * @param levels the thresholds to store */ - public static void setBatteryLevels(final Context context, final LevelThresholds levels) { + public static void setBatteryLevels(Context context, LevelThresholds levels) { prefs(context).edit() .putInt(context.getString(R.string._pref_key_critical_battery_level), levels.critical()) .putInt(context.getString(R.string._pref_key_warn_battery_level), levels.warning()) @@ -106,7 +106,7 @@ public static void setBatteryLevels(final Context context, final LevelThresholds * * @return the configured limit in %/h */ - public static int drainLimitPph(final Context context) { + public static int drainLimitPph(Context context) { return clampDrainLimit(prefs(context).getInt( context.getString(R.string._pref_key_fast_drain_limit), DEFAULT_DRAIN_LIMIT_PPH)); } @@ -120,11 +120,11 @@ public static int drainLimitPph(final Context context) { * * @return the limit clamped to {@code [MIN_DRAIN_LIMIT_PPH, MAX_DRAIN_LIMIT_PPH]} */ - public static int clampDrainLimit(final int stored) { + public static int clampDrainLimit(int stored) { return Math.max(MIN_DRAIN_LIMIT_PPH, Math.min(MAX_DRAIN_LIMIT_PPH, stored)); } - private static SharedPreferences prefs(final Context context) { + private static SharedPreferences prefs(Context context) { return PreferenceManager.getDefaultSharedPreferences(context); } }