KellyPoolApp — Code Review Report

Last updated: 2026-03-21 Legend: - [ ] = pending, - [x] = completed

Project Overview

1. No null checks on Intent extras All Activities blindly call getIntent().getStringArrayListExtra(...) without null checks. If an Activity is launched without expected data (e.g., process death, deep link, test), it will crash with a NullPointerException. Files: GameActivity.java:46-48, ShowAssignedBallsActivity.java:43
2. Unsafe cast from Serializable
```
ballAssignments = (HashMap<Integer, String>) getIntent().getSerializableExtra("ballAssignments");
```
No instanceof check before casting. If the data is missing or a different type, this throws a ClassCastException at runtime. The getSerializableExtra API is also deprecated on API 33+. File: GameActivity.java:47
3. No state preservation on configuration changes Zero use of onSaveInstanceState() or ViewModel. Rotating the device mid-game destroys all state — player names, ball assignments, and scores are lost. Files: All Activities

4. Memory leak — TextView references stored in HashMap
```
private Map<String, TextView> playerTextViews = new HashMap<>();
```
This HashMap holds strong references to Views. On configuration changes, the old Activity context is retained, preventing garbage collection. File: GameActivity.java:30
5. Race condition in elimination logic Player state (score tracking, totalPlayers, elimination flags) is mutated inside AlertDialog callbacks. Rapid ball taps can interleave these callbacks, corrupting game state. No synchronisation or state locking exists. File: GameActivity.java:184-210
6. No minification/obfuscation in release builds
```
isMinifyEnabled = false
```
The release APK can be trivially decompiled, exposing all logic and hardcoded strings. ProGuard/R8 rules should be set up. File: app/build.gradle.kts:21
7. Image loading with no error handling
```
int imageResId = getResources().getIdentifier("billard_" + i, "drawable", getPackageName());
ballIcon.setImageResource(imageResId);
```
If a drawable is missing, getIdentifier returns 0 and the image silently shows nothing. No feedback to the user. Files: GameActivity.java:126-127, ShowAssignedBallsActivity.java:98-104

8. Hardcoded magic numbers and strings throughout

Value	Where	Should Be
`15` (ball count)	`GameActivity`, `ShowAssignedBallsActivity`	Constant
`2`, `9` (player range)	`SetUpGameActivity`	Constants
`"Free Ball"`	`GameActivity:84`, `GameActivity:147`, `ShowAssignedBallsActivity:142`	String resource
`"billard_"`	`GameActivity:126`, `ShowAssignedBallsActivity:98`	String resource
`150, 150` (px dimensions)	`GameActivity:131-132`	`dimens.xml` value in `dp`

9. Dialogs are non-dismissible
```
builder.setCancelable(false);
```
Users cannot back out of an elimination dialog if accidentally triggered. Poor UX and no way to undo an accidental tap. File: GameActivity.java:182
10. Default player name numbering is fragile
```
playerNames.add("Player " + (playerNames.size() + 1));
```
If a user leaves the 2nd field blank but fills the 3rd, the default name will be "Player 2" but assigned to the 3rd slot. The number reflects list size at that moment, not the actual player position. File: PlayerEntryActivity.java:63
11. Dead code — showEliminationPopup() is never called This method is defined but unreachable. It should either be wired up or removed. File: GameActivity.java:238-244
12. Accessibility — contentDescription="TODO" left in production File: activity_main.xml:14

13. No constants file — magic values scattered across multiple classes with no single source of truth
14. No input validation on player name length or characters
15. Anonymous OnClickListener classes where lambdas would be cleaner (MainActivity.java)
16. Unused string resources in strings.xml
17. Hardcoded text "I am " in layout XML instead of strings.xml (activity_show_assigned_balls.xml:22)
18. getResources().getIdentifier() called in a loop — slow reflection-based lookup, should be cached
19. No logging framework — scattered Log.e calls with no consistent tagging strategy
20. Single git commit with a typo ("Uploding") and no meaningful commit history

The test suite currently consists only of the two auto-generated placeholder tests. Estimated coverage: <5%.

25. ViewModel / LiveData — no separation of UI and business logic
26. Room database — no game history or persistence between sessions
27. Coroutines / async handling — all work done on main thread
28. onSaveInstanceState — state lost on rotation
29. Hilt / dependency injection — no DI framework
30. Kotlin migration — project is still Java (decision: staying in Java, not worth the rewrite)
31. ProGuard rules — no shrinking/obfuscation config

Item 1 — Null checks on all getIntent() extras
Item 2 — Replace deprecated getSerializableExtra with typed API + instanceof guard
Item 3 — Add onSaveInstanceState / restore in GameActivity
Item 6 — Enable isMinifyEnabled = true and set up ProGuard rules
Item 4 — Fix the memory leak in playerTextViews

Item 8 — Extract all magic numbers and strings to Constants.java and strings.xml
Item 8 — Replace hardcoded pixel dimensions with dp values in dimens.xml
Item 7 — Add error handling around image loading
Item 9 — Make elimination dialog dismissible (or add an undo mechanic)
Item 11 — Remove dead code (showEliminationPopup)
Item 10 — Fix the player name numbering logic
Items 21-24 — Write unit tests for core game logic