refactor(service): extract pure logic into a Bukkit-free service layer wired via Guice - #17
Merged
Merged
Conversation
added 2 commits
July 22, 2026 17:37
Introduce PlayerArmorWearerRegistry (model.player) holding the PlayerId-to-ArmorSetWearer state and ArmorSetRegistry (model.set) holding the ArmorSetId-to-set mapping. Both are Bukkit-free, self-contained, and unit-tested to 100% line and branch. SetPlayerManager and SetManager keep their existing static public API but delegate to a held registry instance; getSetPlayer reconstructs the Bukkit SetPlayer on demand from the wearer record, and getSet guards null/empty names before ArmorSetId rejects them. The characterization tests are unchanged, proving observable behavior is preserved.
nbdSteve
commented
Jul 22, 2026
| public class SetManager { | ||
| private static Map<String, YamlFileUtil> setConfigs; | ||
| private static Map<String, Set> sets; | ||
| private static final ArmorSetRegistry<Set> registry = new ArmorSetRegistry<>(); |
Owner
Author
There was a problem hiding this comment.
Rather than having everything static can be pass around instances? We shouldnt need to rely on static maps, pass around the registry class instance - might need to use DI
| public static Map<String, Set> getSets() { | ||
| Map<String, Set> sets = new HashMap<>(); | ||
| registry.getAll().forEach((id, set) -> sets.put(id.toString(), set)); | ||
| return sets; |
Owner
Author
There was a problem hiding this comment.
We should just return the registry object
added 2 commits
July 23, 2026 10:34
SetManager now keeps its ArmorSetRegistry on a shared instance rather than in a static field, and getSets() returns the registry's own view instead of copying it into a fresh HashMap. The static entry points delegate to the shared instance so every caller and the characterization tests are unchanged; getSets() exposes ArmorSetId keys, so the one keySet() caller iterates those ids.
nbdSteve
changed the title
added 3 commits
July 23, 2026 12:06
Introduce Google Guice as the plugin's dependency-injection framework and use it to replace the static SetManager/SetPlayerManager singletons with ordinary injected instances. - Add Guice 5.1.0 (JSR-330 javax.inject). Chosen over Dagger to avoid a second annotation processor alongside Lombok and for lower runtime ceremony on Spigot. Shade + relocate guice/guava/javax.inject/aopalliance under gg.steve.mc.ap.lib.* (mirrors the commons-lang3 relocation) and exclude annotation-only transitives from the shaded jar. - Stand up a composition root in ArmorPlus.onEnable: build one Injector from ArmorPlusModule and resolve the shared collaborators once. - Replace SetManager with ArmorSetCatalog and SetPlayerManager with PlayerArmorSetService - constructor-injected singletons holding the pure ArmorSetRegistry / PlayerArmorWearerRegistry. Drop the static maps and get() entry points; thread the instances to listeners, commands, the GUI, and the placeholder expansion. - SetPlayer takes a resolved Set directly instead of resolving a name through the old static manager. Behavior is preserved: the characterization suite is unchanged except for the manager-coupled tests, which now construct the injected instances instead of mocking static state (no behavioral assertions changed). mvn clean verify green on JDK 25 (Java 8 bytecode); check-core coverage gate still 100%.
nbdSteve
changed the title
added 2 commits
July 23, 2026 21:17
Purge bloated javadoc and comments that merely restate the code (one-line method narration, class summaries derivable from the name). Keep only the few genuinely non-obvious WHYs: why the model registries are bound rather than annotated, why ArmorSetRegistry is generic, and the DI-wiring invariant the Guice composition-root test turns on. No behavior change; characterization suite untouched.
nbdSteve
changed the title
…ayer model/ should hold data only (value objects, typed IDs, enums, port interfaces). The registries and damage calculators were behavior-bearing, so relocate them into a new concept-grouped gg.steve.mc.ap.service layer: - service.set.ArmorSetRegistry (from model.set) - service.player.PlayerArmorWearerRegistry (from model.player) - service.combat.BasicDamageCalculator (from model.ability) - service.combat.ArmorHandItem (from model.ability) Update all callers, the Guice bindings, and the JaCoCo core-gate paths. Add architecture.LayeringRulesTest (ArchUnit) enforcing that neither model.. nor service.. depends on org.bukkit.., net.minecraft.., or de.tr7zw.., so the layering can't silently regress. Bukkit stays at the adapter edges only. Characterization tests are unchanged apart from package/import lines.
|
nbdSteve
changed the title
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
PR #17 continues a behavior-preserving refactor of ArmorPlus (a Spigot/Paper plugin) toward a layered/hexagonal architecture, with a strict rule that the model layer is pure data and no behavior lives in it. This branch's earlier commits introduced pure registries (ArmorSetRegistry, PlayerArmorWearerRegistry), a pure damage calculator (BasicDamageCalculator, ArmorHandItem), Guice DI wiring (composition root in ArmorPlus.onEnable, shaded/relocated under gg.steve.mc.ap.lib.*), typed-string IDs, and a purged terse-comment convention (comments document only the non-obvious WHY).
The newest commit (5dd4ae2) fixes a layering defect the reviewer flagged: behavior-bearing classes were wrongly nested under model/. It MOVES those pure-logic classes out of model into a new concept-grouped gg.steve.mc.ap.service layer - service.set.ArmorSetRegistry, service.player.PlayerArmorWearerRegistry, service.combat.BasicDamageCalculator, service.combat.ArmorHandItem - leaving model as data-only (value objects, typed IDs, enums, port interfaces). All callers, the Guice bindings, and the JaCoCo core-gate class paths are updated to the new packages. A new ArchUnit test (architecture.LayeringRulesTest, via the archunit-junit5 test dependency) mechanically enforces that neither model.. nor service.. depends on org.bukkit.., net.minecraft.., or de.tr7zw.. - Bukkit stays only at the adapter edges (listeners, commands, GUI, papi, bootstrap).
This is deliberately behavior-preserving: the characterization test suite is byte-identical except for package/import lines (the moved classes and their tests changed package only). No product behavior changes. The full flow/command/converter/Request-Response use-case rearchitecture is intentionally OUT OF SCOPE for this PR and deferred to a later task - do not flag its absence. Constraints honored: JDK 25 build emitting Java 8 bytecode, core coverage gate still 100% line+branch, no Manager-suffixed names, fully-specific class names.
What Changed
servicelayer (service.set.ArmorSetRegistry,service.player.PlayerArmorWearerRegistry,service.combat.BasicDamageCalculator,service.combat.ArmorHandItem); renamed the formerSetManager/SetPlayerManagersingletons toArmorSetCatalog/PlayerArmorSetServiceand dropped their static state so the adapters are thin delegators over the registries.ArmorPlusModulebinds the registries, catalog, and player service as singletons with the composition root inArmorPlus.onEnable, threading collaborators into listeners/commands/GUI/PAPI expansion; Guice, guava, andjavax.injectare shaded and relocated undergg.steve.mc.ap.lib.*.LayeringRulesTestthat fails the build if anymodel..orservice..class importsorg.bukkit/net.minecraft/de.tr7zw, relocated the characterization and unit tests to match the new packages, and repointed the JaCoCo core-coverage gate at the new class paths (still 100% line+branch).Risk Assessment
✅ Low: A well-bounded, fully-tested, behavior-preserving package move with all call sites, DI bindings, coverage gate, and an ArchUnit enforcement rule consistently updated; no material new bugs found.
Testing
Ran the full JDK-25
mvn clean verify(all 237 tests green, JaCoCo check-core gate met), then produced targeted evidence for the intent: rename-aware diffs proving the moved tests are byte-identical apart from package/import lines, a negative test that injected a real Bukkit dependency into a service class and confirmed the new ArchUnit LayeringRulesTest catches it (then reverted), and per-class JaCoCo numbers showing 100% line+branch on the four relocated classes. No UI/visual artifact applies - this is an internal package/layering refactor with no end-user-visible surface, so evidence is the architecture-enforcement proof and coverage data rather than screenshots. Working tree cleaned (build output removed) and verified clean.Evidence: ArchUnit negative test: guard rejects an injected Bukkit dep in the service layer
LayeringRulesTest.service_stays_platform_free FAILED after injecting org.bukkit.Material into service.set.ArmorSetRegistry: Architecture Violation [Priority: MEDIUM] - Rule 'no classes that reside in a package '..service..' should depend on classes that reside in any package ['org.bukkit..', 'net.minecraft..', 'de.tr7zw..']' was violated (2 times): Method <...ArmorSetRegistry.leakPlatformType()> gets field <org.bukkit.Material.AIR> Method <...ArmorSetRegistry.leakPlatformType()> has return type <org.bukkit.Material> Tests run: 2, Failures: 1 -> BUILD FAILURE. (Injection reverted; working tree clean.)Evidence: JaCoCo core-gate: 100% line+branch on relocated service classes
PACKAGE,CLASS,INSTR_MISSED,INSTR_COVERED,BRANCH_MISSED,BRANCH_COVERED,LINE_MISSED,LINE_COVERED service.combat,BasicDamageCalculator,0,26,0,4,0,5 service.combat,ArmorHandItem,0,20,0,2,0,4 service.player,PlayerArmorWearerRegistry,0,50,0,0,0,13 service.set,ArmorSetRegistry,0,33,0,0,0,9 (all zero missed -> check-core: 'All coverage checks have been met')Pipeline
Updates from git push no-mistakes
✅ **intent** - passed
✅ No issues found.
✅ **Rebase** - passed
✅ No issues found.
src/main/java/gg/steve/mc/ap/player/PlayerArmorSetService.java:32- PlayerArmorSetService.init() returns after applying a set to the first matching online player, so on plugin enable/reload only one player's armor-set effects are re-applied even if many are wearing sets. This is a verbatim copy of the pre-existing SetPlayerManager.init() bug - the refactor is intentionally behavior-preserving, and AGENTS.md documents that surprising pinned behavior should be deferred to a separate bug-fix PR. Flagging only so a reviewer scanning the new file doesn't mistake the nested-loopreturnfor a fresh defect.✅ **Test** - passed
✅ No issues found.
JAVA_HOME=<corretto-25> mvn -B clean verify— full build + all 237 tests pass, including relocated characterization suites at service.combat/service.set/service.playermvn -B test -Dtest=LayeringRulesTestafter injectingimport org.bukkit.Material+ a Material-returning method into service.set.ArmorSetRegistry — confirmed the guard FAILS (service_stays_platform_free architecture violation), then reverted the injection (working tree clean)git diff -M(rename-aware) on the four moved test files — verified they differ only by package line + one required model.player.ArmorSetWearer import (97-98% similarity)Parsed target/site/jacoco/jacoco.csv — confirmed 0 missed instructions/branches/lines for service.combat.ArmorHandItem, service.combat.BasicDamageCalculator, service.player.PlayerArmorWearerRegistry, service.set.ArmorSetRegistry (JaCoCo check-core: 'All coverage checks have been met')✅ **Document** - passed
✅ No issues found.
✅ **Lint** - passed
✅ No issues found.
✅ **Push** - passed
✅ No issues found.