diff --git a/.plans/issue-16-fix-diceroll-bug.md b/.plans/issue-16-fix-diceroll-bug.md new file mode 100644 index 0000000..ea1c75d --- /dev/null +++ b/.plans/issue-16-fix-diceroll-bug.md @@ -0,0 +1,27 @@ +# Plan: Fix DiceRoll Success Counting Bug (Issue #16) + +## Summary +The `DiceRoll.roll()` method computes `numberOfSuccesses` from the stale `this.result` list instead of the freshly rolled values. The fix is to capture the new roll in a local variable before passing it to `copy()`, so both `result` and `numberOfSuccesses` reference the same fresh data. + +## Steps + +### Step 1: Fix `Dice.kt` +**File**: `sharedUI/src/commonMain/kotlin/org/shahondin1624/lib/functions/Dice.kt` + +Change the `roll()` method to: +1. Roll dice into a local `val newResult` +2. Count successes from `newResult` +3. Pass both to `copy()` + +### Step 2: Write unit test +**File**: `sharedUI/src/commonTest/kotlin/org/shahondin1624/DiceTest.kt` + +Create a new test file with: +- Test that after `roll()`, `numberOfSuccesses` equals the count of values >= threshold in `result` +- Test with default threshold (5) and custom threshold +- Test that `result` list has correct size (matches `numberOfDice`) +- Test the `countSuccesses` extension function directly + +## AC Verification Checklist +1. [ ] `numberOfSuccesses` is computed from newly rolled values (not stale result list) +2. [ ] Unit test verifying correct success count after `.roll()` diff --git a/sharedUI/src/commonMain/kotlin/org/shahondin1624/lib/functions/Dice.kt b/sharedUI/src/commonMain/kotlin/org/shahondin1624/lib/functions/Dice.kt old mode 100644 new mode 100755 index 149d2f6..45722e5 --- a/sharedUI/src/commonMain/kotlin/org/shahondin1624/lib/functions/Dice.kt +++ b/sharedUI/src/commonMain/kotlin/org/shahondin1624/lib/functions/Dice.kt @@ -15,8 +15,11 @@ data class DiceRoll( val result: List = listOf(), val numberOfSuccesses: Int = -1 ) { - fun roll(numberForSuccessOrHigher: Int = 5): DiceRoll = this.copy( - result = rollXDice(sides = this.numberOfSides, numberOfDice = this.numberOfDice), - numberOfSuccesses = result.countSuccesses(numberForSuccessOrHigher) - ) + fun roll(numberForSuccessOrHigher: Int = 5): DiceRoll { + val newResult = rollXDice(sides = this.numberOfSides, numberOfDice = this.numberOfDice) + return this.copy( + result = newResult, + numberOfSuccesses = newResult.countSuccesses(numberForSuccessOrHigher) + ) + } } \ No newline at end of file diff --git a/sharedUI/src/commonTest/kotlin/org/shahondin1624/DiceTest.kt b/sharedUI/src/commonTest/kotlin/org/shahondin1624/DiceTest.kt new file mode 100644 index 0000000..4a76ee1 --- /dev/null +++ b/sharedUI/src/commonTest/kotlin/org/shahondin1624/DiceTest.kt @@ -0,0 +1,89 @@ +package org.shahondin1624 + +import org.shahondin1624.lib.functions.DiceRoll +import org.shahondin1624.lib.functions.countSuccesses +import kotlin.test.Test +import kotlin.test.assertEquals + +class DiceTest { + + @Test + fun rollProducesCorrectSuccessCount() { + val diceRoll = DiceRoll(numberOfDice = 10).roll() + val expectedSuccesses = diceRoll.result.count { it >= 5 } + assertEquals( + expectedSuccesses, + diceRoll.numberOfSuccesses, + "numberOfSuccesses should match the count of dice >= 5 in result" + ) + } + + @Test + fun rollWithCustomThresholdProducesCorrectSuccessCount() { + val diceRoll = DiceRoll(numberOfDice = 10).roll(numberForSuccessOrHigher = 4) + val expectedSuccesses = diceRoll.result.count { it >= 4 } + assertEquals( + expectedSuccesses, + diceRoll.numberOfSuccesses, + "numberOfSuccesses should match the count of dice >= 4 in result" + ) + } + + @Test + fun rollResultHasCorrectNumberOfDice() { + val numberOfDice = 6 + val diceRoll = DiceRoll(numberOfDice = numberOfDice).roll() + assertEquals( + numberOfDice, + diceRoll.result.size, + "result list size should equal numberOfDice" + ) + } + + @Test + fun countSuccessesExtensionFunction() { + val rolls = listOf(1, 2, 3, 4, 5, 6) + assertEquals(2, rolls.countSuccesses(5), "Only 5 and 6 should be successes with threshold 5") + assertEquals(3, rolls.countSuccesses(4), "4, 5, and 6 should be successes with threshold 4") + assertEquals(1, rolls.countSuccesses(6), "Only 6 should be a success with threshold 6") + } + + @Test + fun freshRollDoesNotUseStaleResult() { + // Create a DiceRoll with an empty result (default), then roll. + // Before the fix, numberOfSuccesses would always be 0 because it counted + // successes from the stale empty result list. + val initial = DiceRoll(numberOfDice = 20) + assertEquals(listOf(), initial.result, "Initial result should be empty") + assertEquals(-1, initial.numberOfSuccesses, "Initial numberOfSuccesses should be -1") + + val rolled = initial.roll() + assertEquals(20, rolled.result.size, "Rolled result should have 20 dice") + + val manualCount = rolled.result.count { it >= 5 } + assertEquals( + manualCount, + rolled.numberOfSuccesses, + "numberOfSuccesses must reflect the new roll, not the stale empty list" + ) + } + + @Test + fun consecutiveRollsProduceIndependentResults() { + val diceRoll = DiceRoll(numberOfDice = 10) + val firstRoll = diceRoll.roll() + val secondRoll = diceRoll.roll() + + // Both rolls should independently have correct success counts + assertEquals( + firstRoll.result.count { it >= 5 }, + firstRoll.numberOfSuccesses, + "First roll success count should be correct" + ) + assertEquals( + secondRoll.result.count { it >= 5 }, + secondRoll.numberOfSuccesses, + "Second roll success count should be correct" + ) + } +}