Fixed bug where skunking was showing always

[madebybye]

No description provided.

[claude]

Pull Request Review: Fixed bug where skunking was showing always

Summary

This PR fixes a bug in the skunk detection logic and bumps the version from 1.0 to 1.1. The changes correctly implement the skunk condition to only trigger when the winner beats the loser by more than 30 points.

---

Code Quality & Best Practices

✅ Positive Changes

1. Correct Logic Fix - The operator change from >= to > at ContentView.swift:81 is correct. In cribbage, a skunk occurs when the loser has 30+ points LESS than the winner (i.e., difference > 30), not when the difference equals exactly 30.

2. Mutual Exclusivity - The explicit setting of showConfetti = false in the skunk branch and showSkunk = false in the confetti branch improves state clarity and prevents both animations from showing simultaneously.

---

Potential Issues & Concerns

🔴 Critical: Version Number Regression

Location: Cribbage Scorer.xcodeproj/project.pbxproj:250, 270, 296, 319

- CURRENT_PROJECT_VERSION = 3;
+ CURRENT_PROJECT_VERSION = 1;

Problem: The CURRENT_PROJECT_VERSION was decreased from 3 to 1, which is likely a mistake. This is the build number that should monotonically increase with each release.

Impact:

• App Store/TestFlight may reject the build if a lower version number has already been submitted
• Version tracking becomes confusing
• May cause issues with automatic updates

Recommendation: Change CURRENT_PROJECT_VERSION to 4 (or higher) instead of reverting to 1.

---

⚠️ Logic Verification Needed

Location: ContentView.swift:77-87

The comment states "loser has 90 or less when winner hits 121", but the code checks winnerScore - loserScore > 30. Let's verify this is correct:

• If winner = 121 and loser = 91: difference = 30 (NOT skunked) ✓
• If winner = 121 and loser = 90: difference = 31 (SKUNKED) ✓

The logic appears correct, but the comment could be more precise:

// Check if opponent got SKUNKED! (loser is more than 30 points behind)
if winnerScore - loserScore > skunkDifference {

---

🟡 Minor: Redundant State Management

Location: ContentView.swift:82-86

While the explicit setting of both boolean flags improves clarity, these flags are mutually exclusive by design. Consider whether both need to exist as separate state variables, or if a single enum might be cleaner:

enum GameEndState {
    case none
    case normalWin
    case skunk
}

@Published var gameEndState: GameEndState = .none

This would prevent impossible states and reduce the need for manual mutual exclusion. However, this is a minor optimization and not required for the current fix.

---

Performance Considerations

✅ No performance concerns - The changes are minimal and only affect the game-end logic path, which is infrequently executed.

---

Security Considerations

✅ No security concerns - This is a local scoring app with no network operations or data exposure in the changed code.

---

Test Coverage

⚠️ No automated tests found in the repository.

Recommendation: Consider adding unit tests for the GameViewModel, especially for the checkForWinner method:

// Test cases to cover:
- Winner at 121, loser at 91 → normal win (difference = 30)
- Winner at 121, loser at 90 → skunk (difference = 31)
- Winner at 121, loser at 0 → skunk (difference = 121)
- Winner at 120, loser at 90 → no winner yet

Manual testing should verify:

• Regular wins show confetti (no skunk)
• Wins with 31+ point difference show skunk animation
• Only one animation shows at a time

---

Action Items

Must Fix Before Merge:

1. Fix CURRENT_PROJECT_VERSION: Change from 1 to 4 (or appropriate next build number)

Nice to Have:

1. Update comment at line 80 to be more precise about the skunk condition
2. Consider adding basic unit tests for score logic
3. Verify the version bump to 1.1 is intentional for this bug fix

---

Overall Assessment

The core bug fix is correct and well-implemented. The logic change from >= to > and the mutual exclusion of animations are both appropriate. However, the version number regression must be addressed before merging.

Recommendation: Request changes to fix the CURRENT_PROJECT_VERSION before merging.

---

🤖 Generated with Claude Code