From c26e4bab9f71a01e2f1544f05d64479b30e264af Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Oliver=20R=C3=BCmpelein?= <oli_r@fg4f.de>
Date: Sun, 3 Jan 2021 15:08:38 +0100
Subject: [PATCH] Changes as per review.

Missing:
 - Open discussion on "test value" renames
 - docs
 - open discussion on last-die-handling
---
 spec/support/ds4rolls/executor.spec.ts | 18 +++++++++++++++++-
 src/module/rolls/roll-utils.ts         |  6 +++---
 2 files changed, 20 insertions(+), 4 deletions(-)

diff --git a/spec/support/ds4rolls/executor.spec.ts b/spec/support/ds4rolls/executor.spec.ts
index f70d874a..e141b215 100644
--- a/spec/support/ds4rolls/executor.spec.ts
+++ b/spec/support/ds4rolls/executor.spec.ts
@@ -232,6 +232,14 @@ describe("DS4 Rolls with multiple dice and no modifiers.", () => {
             new RollResult(37, RollResultStatus.SUCCESS, [19, 1]),
         );
     });
+
+    it("Should maximize correctly when swapping with more than one crit success", () => {
+        const rollProvider = mockMultipleThrows([1, 1, 15]);
+
+        expect(rollCheckMultipleDice(48, {}, rollProvider)).toEqual(
+            new RollResult(43, RollResultStatus.SUCCESS, [1, 15, 1]),
+        );
+    });
 });
 
 describe("DS4 Rolls with multiple dice and min/max modifiers.", () => {
@@ -308,10 +316,18 @@ describe("DS4 Rolls with multiple and slaying dice, first throw", () => {
     it("Should fail with the first roll being a `20`", () => {
         const rollProvider = mockMultipleThrows([20, 2, 19]);
 
-        expect(rollCheckMultipleDice(48, {}, rollProvider)).toEqual(
+        expect(rollCheckMultipleDice(48, { useSlayingDice: true }, rollProvider)).toEqual(
             new RollResult(0, RollResultStatus.CRITICAL_FAILURE, [20, 2, 19]),
         );
     });
+
+    it("Should issue a critical success, even with resorting dice", () => {
+        const rollProvider = mockMultipleThrows([2, 19, 15]);
+
+        expect(rollCheckMultipleDice(48, { useSlayingDice: true, maxCritSucc: 2 }, rollProvider)).toEqual(
+            new RollResult(42, RollResultStatus.CRITICAL_SUCCESS, [19, 15, 2]),
+        );
+    });
 });
 
 describe("DS4 Rolls with multiple and slaying dice, recurrent throw", () => {
diff --git a/src/module/rolls/roll-utils.ts b/src/module/rolls/roll-utils.ts
index 7db8da27..14e95f01 100644
--- a/src/module/rolls/roll-utils.ts
+++ b/src/module/rolls/roll-utils.ts
@@ -3,18 +3,18 @@ import { RollOptions } from "./roll-data";
 export function isDiceSwapNecessary(
     critSuccesses: Array<number>,
     otherRolls: Array<number>,
-    finalRollValue: number,
+    lastTestValue: number,
 ): boolean {
     if (critSuccesses.length == 0 || otherRolls.length == 0) {
         return false;
     }
     const amountOfOtherRolls = otherRolls.length;
     const lastDice = otherRolls[amountOfOtherRolls - 1];
-    if (lastDice <= finalRollValue) {
+    if (lastDice <= lastTestValue) {
         return false;
     }
 
-    return lastDice + finalRollValue > 20;
+    return lastDice + lastTestValue > 20;
 }
 
 export function isSlayingDiceRepetition(opts: RollOptions): boolean {