refactor(model): extract damage calculation into the pure domain model#16
Merged
Conversation
added 3 commits
July 10, 2026 11:23
…Data/HandSetData now delegate BasicDamageCalculator in model.ability holds the pure damage math: - calculateAttackDamage: basic damage multiplier (increase != -1 guard) - calculateDefense: reduction multiplier + knockback sentinel HandSetData.calculateFinalDamage() now delegates to ArmorHandItem in the domain layer. BasicSetData.onHit/onDamage delegate to BasicDamageCalculator for the non-hand-item paths and defense calculations. The existing characterization tests (BasicSetDataTest, HandSetDataTest) pass unchanged, proving behavior preservation. New BasicDamageCalculatorTest provides 100% line+branch coverage, enforced by the JaCoCo core gate.
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Intent
Extract the pure damage-calculation logic from BasicSetData and HandSetData into the domain model layer (model.ability.BasicDamageCalculator and ArmorHandItem.calculateFinalDamage), using the strangler-fig pattern. The existing classes become thin delegators so the plugin behaves identically. Behavior preservation is the top priority - the characterization tests (BasicSetDataTest, HandSetDataTest) must pass unchanged. The new pure domain logic gets 100% coverage enforced by the JaCoCo core gate. BasicSetData.onHit delegates the basic-increase non-hand-item path to BasicDamageCalculator.calculateAttackDamage; the hand-item path still calls through HandSetData.calculateFinalDamage (which itself now delegates to ArmorHandItem). BasicSetData.onDamage delegates to BasicDamageCalculator.calculateDefense for the reduction math, then applies the returned knockback multiplier to the Bukkit vector. The design intentionally does NOT wire through port interfaces yet - this is pure math extraction only.
What Changed
model.ability.BasicDamageCalculatorhandles the attack-damage multiplier and defense reduction plus knockback multiplier, andArmorHandItem.calculateFinalDamage()handles the hand-item combined multiplier.BasicSetData.onHit/onDamageandHandSetData.calculateFinalDamageare now thin delegators that translate Bukkit events into domain calls and apply the results back, with the-1"disabled" sentinel checked once per path (a review pass deduplicated the redundant checks). Characterization tests (BasicSetDataTest,HandSetDataTest) pass unchanged, and a differential harness comparing the old formulas against the new domain code across 1088 input combinations found no mismatches.BasicDamageCalculatorTestand includedBasicDamageCalculatorin the JaCoCocheck-coregate enforcing 100% line and branch coverage;AGENTS.md/README.mddocs updated to describe the new domain home for damage logic.Risk Assessment
✅ Low: Straightforward strangler-fig extraction of pure math into a new domain class with complete test coverage, verified behavioral equivalence to the original code, and no new logic introduced in the delegators.
Testing
Ran the full mvn clean verify build (212 tests plus the JaCoCo check-core 100% gate) on JDK 25, re-ran the unchanged BasicSetData/HandSetData characterization tests plus the new BasicDamageCalculatorTest in isolation, and executed a custom differential sweep comparing the base commit's original damage formulas against the new domain classes across 1088 input combinations - everything passes with zero numerical mismatches, and the shaded plugin jar ships the new Bukkit-free domain classes as Java 8 bytecode. No screenshot evidence because this is a headless Minecraft server plugin change with no rendered UI; the differential-sweep transcript is the end-user-behavior evidence.
Evidence: Differential sweep: base-commit formulas vs new domain classes (1088 combos, 0 mismatches)
Sample rows (old formula -> new domain result): attack: damage=7.00 increase=1.30 old=9.1000 new=9.1000 attack: damage=7.00 increase=-1.00 old=7.0000 new=7.0000 defense: damage=10.00 reduction=0.50 kb=2.00 old=5.0000 new=5.0000 (kb passthrough=2.00) defense sentinel: damage=10.00 reduction=-1 kb=-1 old=10.0000 new=10.0000 hand: damage=8.00 handIncrease=1.50 setIncrease=1.30 old=14.4000 new=14.4000 hand sentinel: damage=8.00 handIncrease=1.50 setIncrease=-1 old=12.0000 new=12.0000 Checked 1088 input combinations, 0 mismatches. RESULT: new domain logic is numerically identical to the base-commit formulas.Evidence: Characterization + domain test run (unchanged tests, all green)
Running gg.steve.mc.ap.model.ability.BasicDamageCalculatorTest Tests run: 9, Failures: 0, Errors: 0, Skipped: 0 Running gg.steve.mc.ap.data.types.HandSetDataTest Tests run: 15, Failures: 0, Errors: 0, Skipped: 0 Running gg.steve.mc.ap.data.BasicSetDataTest Tests run: 12, Failures: 0, Errors: 0, Skipped: 0 Tests run: 36, Failures: 0, Errors: 0, Skipped: 0Evidence: JaCoCo core-gate coverage for extracted classes (0 missed lines/branches)
Evidence: JaCoCo HTML coverage report for BasicDamageCalculator
Pipeline
Updates from git push no-mistakes
✅ **intent** - passed
✅ No issues found.
✅ **Rebase** - passed
✅ No issues found.
🔧 **Review** - 3 issues found → auto-fixed ✅
src/main/java/gg/steve/mc/ap/data/BasicSetData.java:37- The '-1 means disabled' sentinel decision is duplicated across both layers: onHit calls BasicDamageCalculator.calculateAttackDamage unconditionally (which internally checks increase != -1) and then re-checks this.increase != -1 before applying the result. The same computation is done and discarded when disabled. Guarding once - 'if (this.increase != -1) event.setDamage(BasicDamageCalculator.calculateAttackDamage(event.getDamage(), this.increase))' - preserves behavior (the characterization test only requires that setDamage is never called when disabled) and keeps the disabled-check in a single place. Same pattern applies to onDamage/calculateDefense.src/main/java/gg/steve/mc/ap/model/ability/BasicDamageCalculator.java:19- The ternary 'double kb = (knockback != -1) ? knockback : -1;' is an identity no-op - both branches yield knockback, so it is equivalent to 'double kb = knockback;'. It exists only so the -1 sentinel flows through CombatDamageModification for the delegator to re-check, which also conflicts with CombatDamageModification's javadoc convention that absence-of-modification should be modeled with Optional rather than magic values. Consider dropping the ternary now and modeling 'knockback disabled' explicitly (e.g. Optional or a boolean) in a follow-up.src/main/java/gg/steve/mc/ap/data/types/HandSetData.java:41- calculateFinalDamage builds a fresh ArmorHandItem (including a DamageCauseId with commons-lang3 validation) on every damage event, and the requireSet/damageCause fields it populates are unused by the calculation. Per-hit allocation is negligible for the JVM and caching would be complicated by HandSetData's mutable setters (setIncrease/setActiveCause), so per-call construction is a reasonable tradeoff - noting it as an accepted cost of the strangler-fig step.🔧 Fix: deduplicate -1 sentinel checks in damage delegators
✅ Re-checked - no issues remain.
✅ **Test** - passed
✅ No issues found.
mvn -B clean verifywith JDK 25 - full build, all 212 tests, and the JaCoCo check-core 100% line+branch gate (now including BasicDamageCalculator) passmvn -B test -Dtest='BasicSetDataTest,HandSetDataTest,BasicDamageCalculatorTest'- 36 tests pass, including the 27 characterization tests pinning onHit/onDamage/calculateFinalDamage behaviorgit diff 9553adf..be0e8f9 -- BasicSetDataTest.java HandSetDataTest.java- empty, proving characterization tests were not modified to accommodate the refactorDifferential harness (DiffSweep.java) executing the base commit's verbatim formulas vs BasicDamageCalculator.calculateAttackDamage/calculateDefense and ArmorHandItem.calculateFinalDamage across 1088 input combinations including -1 sentinels - 0 mismatchesVerified delegation wiring in the diff: onHit non-hand path -> calculateAttackDamage, hand path -> HandSetData -> ArmorHandItem, onDamage -> calculateDefense + Bukkit knockback vectorgrep -rE 'import (org.bukkit|net.minecraft|de.tr7zw)' src/main/java/gg/steve/mc/ap/model/- zero forbidden imports in the domain modelunzip -l target/ArmorPlus-v2.3.6.jar+javap -v- new domain classes present in the shaded plugin jar as Java 8 bytecode (major version 52)Inspected target/site/jacoco/jacoco.csv - 0 missed instructions/branches/lines for BasicDamageCalculator and ArmorHandItem✅ **Document** - passed
✅ No issues found.
✅ **Lint** - passed
✅ No issues found.
✅ **Push** - passed
✅ No issues found.