Implement type-safe registry keys for Skills and Abilities (#129)#194
Implement type-safe registry keys for Skills and Abilities (#129)#194dnocturne wants to merge 1 commit into
Conversation
…afety - Updated various components in the mining, swords, and woodcutting abilities to replace direct ability retrieval with the new McRPGAbilityKey interface. - Introduced McRPGAbilityKey and McRPGSkillKey interfaces for better organization and access to abilities and skills. - Enhanced SkillRegistry to support type-safe access to skills using SkillKey. - Ensured all instances of ability retrieval in components and commands are now using the new key-based approach for consistency and maintainability.
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/main/java/us/eunoians/mcrpg/skill/SkillRegistry.java (1)
44-50:⚠️ Potential issue | 🟠 MajorPrevent class-key collisions in
SkillRegistry
register(...)only guardsNamespacedKey. If two skills of the same class are registered under different keys, Line 49 overwritesskillsByClass, and later unregister can remove the class mapping for the still-registered entry.🛠️ Suggested guard + safer remove
public void register(`@NotNull` Skill skill) { if (skills.containsKey(skill.getSkillKey())) { throw new IllegalArgumentException("Skill " + skill.getSkillKey() + " already registered"); } + if (skillsByClass.containsKey(skill.getClass())) { + throw new IllegalArgumentException("Skill class " + skill.getClass().getName() + " already registered"); + } skills.put(skill.getSkillKey(), skill); skillsByClass.put(skill.getClass(), skill); Bukkit.getPluginManager().callEvent(new SkillRegisterEvent(skill)); } @@ public void unregisterSkill(`@NotNull` NamespacedKey skillKey) { Skill skill = skills.remove(skillKey); if (skill != null) { - skillsByClass.remove(skill.getClass()); + skillsByClass.remove(skill.getClass(), skill); Bukkit.getPluginManager().callEvent(new SkillUnregisterEvent(skill)); } }Also applies to: 121-125
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/us/eunoians/mcrpg/skill/SkillRegistry.java` around lines 44 - 50, The register behavior currently only guards by NamespacedKey and writes to skillsByClass using the skill class, which allows a later registration to overwrite an existing class entry; update register(Skill) to prevent class-key collisions by either (a) checking skillsByClass.containsKey(skill.getClass()) and throwing an IllegalArgumentException with a clear message, or (b) changing skillsByClass from Map<Class<? extends Skill>, Skill> to Map<Class<? extends Skill>, Collection<Skill>> and adding the skill to the collection (pick one approach and apply consistently). Also update the corresponding unregister(...) logic so it only removes the class mapping if the value equals the skill being removed (or remove the skill from the collection if you choose the collection approach) to avoid deleting a still-registered sibling; refer to the skills, skillsByClass fields and the register(...) and unregister(...) methods when making the changes.
🧹 Nitpick comments (5)
src/main/java/us/eunoians/mcrpg/ability/AbilityKeyImpl.java (1)
12-24: Enforce non-null invariant in the record constructor.
@NotNullis not runtime enforcement; direct construction (new AbilityKeyImpl<>(null)) can violate the contract and cause downstream failures.Suggested fix
import org.jetbrains.annotations.NotNull; +import java.util.Objects; @@ public record AbilityKeyImpl<A extends Ability>(`@NotNull` Class<A> ability) implements AbilityKey<A> { + public AbilityKeyImpl { + Objects.requireNonNull(ability, "ability"); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/us/eunoians/mcrpg/ability/AbilityKeyImpl.java` around lines 12 - 24, The record AbilityKeyImpl currently accepts a null Class at runtime because `@NotNull` is only a static annotation; add a null-check in the record's canonical constructor (AbilityKeyImpl) to enforce the invariant (e.g., Objects.requireNonNull(ability, "ability")) so direct construction fails fast, and also apply the same null-check in the static factory method create(`@NotNull` Class<A> clazz) (or delegate create to the record constructor) to prevent new AbilityKeyImpl<>(null) or AbilityKeyImpl.create(null) from producing an invalid instance.src/main/java/us/eunoians/mcrpg/ability/impl/woodcutting/NymphsVitalityComponents.java (1)
29-29: Consider centralizing repeatedNymphsVitalitylookup.Line 29 and Line 44 perform the same registry lookup. Extracting a helper keeps both paths consistent and reduces repeated registry traversal on hot events.
♻️ Proposed refactor
public class NymphsVitalityComponents { + `@NotNull` + private static NymphsVitality nymphsVitality() { + return McRPG.getInstance().registryAccess().registry(McRPGRegistryKey.ABILITY).ability(McRPGAbilityKey.NYMPHS_VITALITY); + } private static class NymphsVitalityActivateOnHungerDropComponent implements EventActivatableComponent { @@ - NymphsVitality nymphsVitality = McRPG.getInstance().registryAccess().registry(McRPGRegistryKey.ABILITY).ability(McRPGAbilityKey.NYMPHS_VITALITY); + NymphsVitality nymphsVitality = nymphsVitality(); @@ private static class NymphsVitalityActivateOnMoveDropComponent implements EventActivatableComponent { @@ - NymphsVitality nymphsVitality = McRPG.getInstance().registryAccess().registry(McRPGRegistryKey.ABILITY).ability(McRPGAbilityKey.NYMPHS_VITALITY); + NymphsVitality nymphsVitality = nymphsVitality();Also applies to: 44-44
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/us/eunoians/mcrpg/ability/impl/woodcutting/NymphsVitalityComponents.java` at line 29, The NymphsVitality lookup is duplicated (calls to McRPG.getInstance().registryAccess().registry(McRPGRegistryKey.ABILITY).ability(McRPGAbilityKey.NYMPHS_VITALITY)) — create a single helper method (e.g., a private static getNymphsVitality() in NymphsVitalityComponents or a shared utility) that returns the NymphsVitality instance and replace both direct lookups (the uses on line 29 and line 44) with calls to that helper to centralize the retrieval and avoid repeated registry traversal.src/main/java/us/eunoians/mcrpg/gui/ability/slot/remotetransfer/RemoteTransferToggleSlot.java (1)
88-88: DeduplicateRemoteTransferlookup across slot operations.Line 88, Line 101, and Line 121 repeat the same registry lookup. A small helper improves readability and reduces repetitive registry access code.
♻️ Proposed refactor
public class RemoteTransferToggleSlot implements McRPGSlot { + `@NotNull` + private RemoteTransfer remoteTransfer() { + return McRPG.getInstance().registryAccess().registry(McRPGRegistryKey.ABILITY).ability(McRPGAbilityKey.REMOTE_TRANSFER); + } @@ public boolean isItemDisallowed() { - RemoteTransfer remoteTransfer = McRPG.getInstance().registryAccess().registry(McRPGRegistryKey.ABILITY).ability(McRPGAbilityKey.REMOTE_TRANSFER); + RemoteTransfer remoteTransfer = remoteTransfer(); @@ public void toggleItemStack() { - RemoteTransfer remoteTransfer = McRPG.getInstance().registryAccess().registry(McRPGRegistryKey.ABILITY).ability(McRPGAbilityKey.REMOTE_TRANSFER); + RemoteTransfer remoteTransfer = remoteTransfer(); @@ public void toggleItemStack(boolean enableItem) { - RemoteTransfer remoteTransfer = McRPG.getInstance().registryAccess().registry(McRPGRegistryKey.ABILITY).ability(McRPGAbilityKey.REMOTE_TRANSFER); + RemoteTransfer remoteTransfer = remoteTransfer();Also applies to: 101-101, 121-121
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/us/eunoians/mcrpg/gui/ability/slot/remotetransfer/RemoteTransferToggleSlot.java` at line 88, The RemoteTransfer registry lookup is repeated in RemoteTransferToggleSlot; create a single helper to deduplicate it—either add a private method like getRemoteTransfer() that returns McRPG.getInstance().registryAccess().registry(McRPGRegistryKey.ABILITY).ability(McRPGAbilityKey.REMOTE_TRANSFER) or cache the RemoteTransfer in a private field initialized lazily, then replace the repeated lookups in the methods (the spots currently calling McRPG.getInstance().registryAccess()...ability(McRPGAbilityKey.REMOTE_TRANSFER)) to call the new helper/field (e.g., getRemoteTransfer() or this.remoteTransfer) to improve readability and avoid repeated registry access.src/main/java/us/eunoians/mcrpg/command/link/UnlinkChestCommand.java (1)
65-65: Keep placeholder lookup fully type-safe here too.Line 65 assigns a typed-key lookup to base
Ability; usingRemoteTransferpreserves the PR’s type-safety goal end-to-end.♻️ Proposed refactor
- Ability ability = RegistryAccess.registryAccess().registry(McRPGRegistryKey.ABILITY).ability(McRPGAbilityKey.REMOTE_TRANSFER); - placeholders.put(SKILL.getPlaceholder(), mcRPGPlayer == null ? ability.getName() : ability.getName(mcRPGPlayer)); + RemoteTransfer remoteTransfer = RegistryAccess.registryAccess().registry(McRPGRegistryKey.ABILITY).ability(McRPGAbilityKey.REMOTE_TRANSFER); + placeholders.put(SKILL.getPlaceholder(), mcRPGPlayer == null ? remoteTransfer.getName() : remoteTransfer.getName(mcRPGPlayer));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/us/eunoians/mcrpg/command/link/UnlinkChestCommand.java` at line 65, Replace the raw Ability assignment with a fully typed RemoteTransfer lookup: change the variable declared at the call to RegistryAccess.registryAccess().registry(McRPGRegistryKey.ABILITY).ability(McRPGAbilityKey.REMOTE_TRANSFER) to the concrete RemoteTransfer type (and rename the variable accordingly), add the RemoteTransfer import if missing, and update any subsequent uses in UnlinkChestCommand to work with the RemoteTransfer API instead of the base Ability type.src/main/java/us/eunoians/mcrpg/ability/impl/mining/RemoteTransfer.java (1)
249-250: Preferthisover registry self-lookup in hot pathInside
RemoteTransfer, fetchingRemoteTransferback from the registry is unnecessary; usethiswhen callingabilityHolder.getAbilityData(...)to avoid repeated registry round-trips.♻️ Suggested simplification
- RemoteTransfer remoteTransfer = getPlugin().registryAccess().registry(McRPGRegistryKey.ABILITY).ability(McRPGAbilityKey.REMOTE_TRANSFER); - var abilityDataOptional = abilityHolder.getAbilityData(remoteTransfer); + var abilityDataOptional = abilityHolder.getAbilityData(this);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/us/eunoians/mcrpg/ability/impl/mining/RemoteTransfer.java` around lines 249 - 250, The code does an unnecessary registry lookup for RemoteTransfer before calling abilityHolder.getAbilityData; replace the registry round‑trip (getPlugin().registryAccess().registry(McRPGRegistryKey.ABILITY).ability(McRPGAbilityKey.REMOTE_TRANSFER)) with this and call abilityHolder.getAbilityData(this) inside the RemoteTransfer class to avoid repeated registry lookups and improve hot‑path performance.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/main/java/us/eunoians/mcrpg/ability/AbilityRegistry.java`:
- Around line 69-73: In register(Ability) in AbilityRegistry ensure the two maps
(abilities and abilitiesByClass) are updated atomically and prevent silent
overwrites by first checking existing mappings: if abilities contains the
ability.getAbilityKey() mapped to a different instance or abilitiesByClass
contains ability.getClass() mapped to a different instance, throw an exception
(fail-fast) instead of blindly putting; only insert into both maps if both
checks pass (or synchronize the method to make the check+put atomic). Likewise
update the unregister logic (the code around lines handling removal at 149-150)
to only remove entries when the current map value matches the expected ability
(avoid removing a class mapping that was replaced), ensuring coherence between
abilities and abilitiesByClass.
---
Outside diff comments:
In `@src/main/java/us/eunoians/mcrpg/skill/SkillRegistry.java`:
- Around line 44-50: The register behavior currently only guards by
NamespacedKey and writes to skillsByClass using the skill class, which allows a
later registration to overwrite an existing class entry; update register(Skill)
to prevent class-key collisions by either (a) checking
skillsByClass.containsKey(skill.getClass()) and throwing an
IllegalArgumentException with a clear message, or (b) changing skillsByClass
from Map<Class<? extends Skill>, Skill> to Map<Class<? extends Skill>,
Collection<Skill>> and adding the skill to the collection (pick one approach and
apply consistently). Also update the corresponding unregister(...) logic so it
only removes the class mapping if the value equals the skill being removed (or
remove the skill from the collection if you choose the collection approach) to
avoid deleting a still-registered sibling; refer to the skills, skillsByClass
fields and the register(...) and unregister(...) methods when making the
changes.
---
Nitpick comments:
In `@src/main/java/us/eunoians/mcrpg/ability/AbilityKeyImpl.java`:
- Around line 12-24: The record AbilityKeyImpl currently accepts a null Class at
runtime because `@NotNull` is only a static annotation; add a null-check in the
record's canonical constructor (AbilityKeyImpl) to enforce the invariant (e.g.,
Objects.requireNonNull(ability, "ability")) so direct construction fails fast,
and also apply the same null-check in the static factory method create(`@NotNull`
Class<A> clazz) (or delegate create to the record constructor) to prevent new
AbilityKeyImpl<>(null) or AbilityKeyImpl.create(null) from producing an invalid
instance.
In `@src/main/java/us/eunoians/mcrpg/ability/impl/mining/RemoteTransfer.java`:
- Around line 249-250: The code does an unnecessary registry lookup for
RemoteTransfer before calling abilityHolder.getAbilityData; replace the registry
round‑trip
(getPlugin().registryAccess().registry(McRPGRegistryKey.ABILITY).ability(McRPGAbilityKey.REMOTE_TRANSFER))
with this and call abilityHolder.getAbilityData(this) inside the RemoteTransfer
class to avoid repeated registry lookups and improve hot‑path performance.
In
`@src/main/java/us/eunoians/mcrpg/ability/impl/woodcutting/NymphsVitalityComponents.java`:
- Line 29: The NymphsVitality lookup is duplicated (calls to
McRPG.getInstance().registryAccess().registry(McRPGRegistryKey.ABILITY).ability(McRPGAbilityKey.NYMPHS_VITALITY))
— create a single helper method (e.g., a private static getNymphsVitality() in
NymphsVitalityComponents or a shared utility) that returns the NymphsVitality
instance and replace both direct lookups (the uses on line 29 and line 44) with
calls to that helper to centralize the retrieval and avoid repeated registry
traversal.
In `@src/main/java/us/eunoians/mcrpg/command/link/UnlinkChestCommand.java`:
- Line 65: Replace the raw Ability assignment with a fully typed RemoteTransfer
lookup: change the variable declared at the call to
RegistryAccess.registryAccess().registry(McRPGRegistryKey.ABILITY).ability(McRPGAbilityKey.REMOTE_TRANSFER)
to the concrete RemoteTransfer type (and rename the variable accordingly), add
the RemoteTransfer import if missing, and update any subsequent uses in
UnlinkChestCommand to work with the RemoteTransfer API instead of the base
Ability type.
In
`@src/main/java/us/eunoians/mcrpg/gui/ability/slot/remotetransfer/RemoteTransferToggleSlot.java`:
- Line 88: The RemoteTransfer registry lookup is repeated in
RemoteTransferToggleSlot; create a single helper to deduplicate it—either add a
private method like getRemoteTransfer() that returns
McRPG.getInstance().registryAccess().registry(McRPGRegistryKey.ABILITY).ability(McRPGAbilityKey.REMOTE_TRANSFER)
or cache the RemoteTransfer in a private field initialized lazily, then replace
the repeated lookups in the methods (the spots currently calling
McRPG.getInstance().registryAccess()...ability(McRPGAbilityKey.REMOTE_TRANSFER))
to call the new helper/field (e.g., getRemoteTransfer() or this.remoteTransfer)
to improve readability and avoid repeated registry access.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (24)
src/main/java/us/eunoians/mcrpg/ability/AbilityKey.javasrc/main/java/us/eunoians/mcrpg/ability/AbilityKeyImpl.javasrc/main/java/us/eunoians/mcrpg/ability/AbilityRegistry.javasrc/main/java/us/eunoians/mcrpg/ability/impl/herbalism/TooManyPlantsComponents.javasrc/main/java/us/eunoians/mcrpg/ability/impl/mining/ExtraOreComponents.javasrc/main/java/us/eunoians/mcrpg/ability/impl/mining/ItsATripleComponents.javasrc/main/java/us/eunoians/mcrpg/ability/impl/mining/RemoteTransfer.javasrc/main/java/us/eunoians/mcrpg/ability/impl/mining/RemoteTransferComponents.javasrc/main/java/us/eunoians/mcrpg/ability/impl/swords/DeeperWoundComponents.javasrc/main/java/us/eunoians/mcrpg/ability/impl/swords/VampireComponents.javasrc/main/java/us/eunoians/mcrpg/ability/impl/swords/bleed/BleedComponents.javasrc/main/java/us/eunoians/mcrpg/ability/impl/woodcutting/DryadsGiftComponents.javasrc/main/java/us/eunoians/mcrpg/ability/impl/woodcutting/ExtraLumberComponents.javasrc/main/java/us/eunoians/mcrpg/ability/impl/woodcutting/HeavySwingComponents.javasrc/main/java/us/eunoians/mcrpg/ability/impl/woodcutting/NymphsVitalityComponents.javasrc/main/java/us/eunoians/mcrpg/command/link/LinkChestCommand.javasrc/main/java/us/eunoians/mcrpg/command/link/UnlinkChestCommand.javasrc/main/java/us/eunoians/mcrpg/gui/ability/slot/remotetransfer/RemoteTransferToggleSlot.javasrc/main/java/us/eunoians/mcrpg/listener/ability/OnBlockDropItemListener.javasrc/main/java/us/eunoians/mcrpg/registry/McRPGAbilityKey.javasrc/main/java/us/eunoians/mcrpg/registry/McRPGSkillKey.javasrc/main/java/us/eunoians/mcrpg/skill/SkillKey.javasrc/main/java/us/eunoians/mcrpg/skill/SkillKeyImpl.javasrc/main/java/us/eunoians/mcrpg/skill/SkillRegistry.java
| public void register(@NotNull Ability ability) { | ||
| NamespacedKey abilityKey = ability.getAbilityKey(); | ||
| abilities.put(abilityKey, ability); | ||
| abilitiesByClass.put(ability.getClass(), ability); | ||
|
|
There was a problem hiding this comment.
Prevent abilitiesByClass/abilities desynchronization during registration.
Current put(...) calls allow silent overwrites. Re-registering an existing key/class can leave stale class mappings or remove the wrong class mapping during later unregister, breaking ability(AbilityKey<?>) correctness.
Suggested fail-fast guard (keeps both maps coherent)
public void register(`@NotNull` Ability ability) {
NamespacedKey abilityKey = ability.getAbilityKey();
+ if (abilities.containsKey(abilityKey)) {
+ throw new IllegalStateException("Ability key already registered: " + abilityKey);
+ }
+ if (abilitiesByClass.containsKey(ability.getClass())) {
+ throw new IllegalStateException("Ability class already registered: " + ability.getClass().getName());
+ }
abilities.put(abilityKey, ability);
abilitiesByClass.put(ability.getClass(), ability);Also applies to: 149-150
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/main/java/us/eunoians/mcrpg/ability/AbilityRegistry.java` around lines 69
- 73, In register(Ability) in AbilityRegistry ensure the two maps (abilities and
abilitiesByClass) are updated atomically and prevent silent overwrites by first
checking existing mappings: if abilities contains the ability.getAbilityKey()
mapped to a different instance or abilitiesByClass contains ability.getClass()
mapped to a different instance, throw an exception (fail-fast) instead of
blindly putting; only insert into both maps if both checks pass (or synchronize
the method to make the check+put atomic). Likewise update the unregister logic
(the code around lines handling removal at 149-150) to only remove entries when
the current map value matches the expected ability (avoid removing a class
mapping that was replaced), ensuring coherence between abilities and
abilitiesByClass.
GUI/UX ReviewNo GUI/UX concerns found. This diff introduces |
Extensibility ReviewBreaking change risk: LOW — New additive API surface with no removals, but one behavioral trap exists that could silently break addon code relying on the class-keyed lookup when ability subclassing is used. CONCERN: CONCERN: CONCERN: The class-based lookup in CONCERN: CONCERN: CONCERN: |
Testing ReviewTesting Audit ReportProduction files changed:
Test files present: (none identified in the diff) Coverage gaps: All items listed below. FindingsCONCERN: CONCERN: CONCERN: CONCERN: CONCERN: CONCERN: CONCERN: CONCERN: CONCERN: Edge case — CONCERN: No test validates that CONCERN: Same uniqueness gap applies to CONCERN: No test method in the diff follows the CONCERN: No test method carries at least one assertion — because no tests exist. Summary of coverage gaps:
|
1 participant
Summary
AbilityKey<A>andSkillKey<S>for type-safe registry accessMcRPGAbilityKeyandMcRPGSkillKeywith all key constantsCloses #129
Changes
New Files
AbilityKey.java- Generic interface for typed ability keysAbilityKeyImpl.java- Record implementationSkillKey.java- Generic interface for typed skill keysSkillKeyImpl.java- Record implementationMcRPGAbilityKey.java- Constants for all McRPG abilitiesMcRPGSkillKey.java- Constants for all McRPG skillsModified Files
AbilityRegistry.java- Addedability(AbilityKey<T>)method and class-based lookupSkillRegistry.java- Addedskill(SkillKey<T>)method and class-based lookupUsage
Before (casting required):
This PR will no doubt require changes, as I made this off my own assumptions reading the issue.
Summary by CodeRabbit
Release Notes
New Features
Refactor