SkillTools refactor

src/main/java/com/gmail/nossr50/util/skills/SkillTools.java

@@ -1,6 +1,5 @@
 package com.gmail.nossr50.util.skills;
 
-import com.gmail.nossr50.api.exceptions.InvalidSkillException;
 import com.gmail.nossr50.config.experience.ExperienceConfig;
 import com.gmail.nossr50.datatypes.skills.PrimarySkillType;
 import com.gmail.nossr50.datatypes.skills.SubSkillType;
@@ -18,6 +17,7 @@ import java.util.Collections;
 import java.util.EnumMap;
 import java.util.HashSet;
 import java.util.List;
+import java.util.Map;
 import java.util.Set;
 import org.bukkit.entity.Entity;
 import org.bukkit.entity.Player;
@@ -27,15 +27,16 @@ import org.jetbrains.annotations.VisibleForTesting;
 
 public class SkillTools {
     private final mcMMO pluginRef;
+
     // TODO: Java has immutable types now, switch to those
     // TODO: Figure out which ones we don't need, this was copy pasted from a diff branch
     public final @NotNull ImmutableList<String> LOCALIZED_SKILL_NAMES;
     public final @NotNull ImmutableList<String> FORMATTED_SUBSKILL_NAMES;
     public final @NotNull ImmutableSet<String> EXACT_SUBSKILL_NAMES;
     public final @NotNull ImmutableList<PrimarySkillType> CHILD_SKILLS;
-    public final static @NotNull ImmutableList<PrimarySkillType> NON_CHILD_SKILLS;
-    public final static @NotNull ImmutableList<PrimarySkillType> SALVAGE_PARENTS;
-    public final static @NotNull ImmutableList<PrimarySkillType> SMELTING_PARENTS;
+    public static final @NotNull ImmutableList<PrimarySkillType> NON_CHILD_SKILLS;
+    public static final @NotNull ImmutableList<PrimarySkillType> SALVAGE_PARENTS;
+    public static final @NotNull ImmutableList<PrimarySkillType> SMELTING_PARENTS;
     public final @NotNull ImmutableList<PrimarySkillType> COMBAT_SKILLS;
     public final @NotNull ImmutableList<PrimarySkillType> GATHERING_SKILLS;
     public final @NotNull ImmutableList<PrimarySkillType> MISC_SKILLS;
@@ -44,73 +45,141 @@ public class SkillTools {
     private final @NotNull ImmutableMap<SuperAbilityType, PrimarySkillType> superAbilityParentRelationshipMap;
     private final @NotNull ImmutableMap<PrimarySkillType, Set<SubSkillType>> primarySkillChildrenMap;
 
-    // The map below is for the super abilities which require readying a tool, its everything except blast mining
     private final ImmutableMap<PrimarySkillType, SuperAbilityType> mainActivatedAbilityChildMap;
     private final ImmutableMap<PrimarySkillType, ToolType> primarySkillToolMap;
 
     static {
+        // Build NON_CHILD_SKILLS once from the enum values
         ArrayList<PrimarySkillType> tempNonChildSkills = new ArrayList<>();
         for (PrimarySkillType primarySkillType : PrimarySkillType.values()) {
-            if (primarySkillType != PrimarySkillType.SALVAGE
-                    && primarySkillType != PrimarySkillType.SMELTING) {
+            if (!isChildSkill(primarySkillType)) {
                 tempNonChildSkills.add(primarySkillType);
             }
         }
-
         NON_CHILD_SKILLS = ImmutableList.copyOf(tempNonChildSkills);
-        SALVAGE_PARENTS = ImmutableList.of(PrimarySkillType.REPAIR, PrimarySkillType.FISHING);
-        SMELTING_PARENTS = ImmutableList.of(PrimarySkillType.MINING, PrimarySkillType.REPAIR);
+
+        SALVAGE_PARENTS = ImmutableList.of(
+                PrimarySkillType.REPAIR,
+                PrimarySkillType.FISHING
+        );
+        SMELTING_PARENTS = ImmutableList.of(
+                PrimarySkillType.MINING,
+                PrimarySkillType.REPAIR
+        );
     }
 
-    public SkillTools(@NotNull mcMMO pluginRef) throws InvalidSkillException {
+    public SkillTools(@NotNull mcMMO pluginRef) {
         this.pluginRef = pluginRef;
 
         /*
          * Setup subskill -> parent relationship map
          */
-        EnumMap<SubSkillType, PrimarySkillType> tempSubParentMap = new EnumMap<>(
-                SubSkillType.class);
+        this.subSkillParentRelationshipMap = buildSubSkillParentMap();
+
+        /*
+         * Setup primary -> (collection) subskill map
+         */
+        this.primarySkillChildrenMap = buildPrimarySkillChildrenMap(subSkillParentRelationshipMap);
+
+        /*
+         * Setup primary -> tooltype map
+         */
+        this.primarySkillToolMap = buildPrimarySkillToolMap();
+
+        /*
+         * Setup ability -> primary map
+         * Setup primary -> ability map
+         */
+        var abilityMaps = buildSuperAbilityMaps();
+        this.superAbilityParentRelationshipMap = abilityMaps.superAbilityParentRelationshipMap();
+        this.mainActivatedAbilityChildMap = abilityMaps.mainActivatedAbilityChildMap();
+
+        /*
+         * Build child skill list
+         */
+        this.CHILD_SKILLS = buildChildSkills();
+
+        /*
+         * Build categorized skill lists
+         */
+        this.COMBAT_SKILLS = buildCombatSkills();
+        this.GATHERING_SKILLS = ImmutableList.of(
+                PrimarySkillType.EXCAVATION,
+                PrimarySkillType.FISHING,
+                PrimarySkillType.HERBALISM,
+                PrimarySkillType.MINING,
+                PrimarySkillType.WOODCUTTING
+        );
+        this.MISC_SKILLS = ImmutableList.of(
+                PrimarySkillType.ACROBATICS,
+                PrimarySkillType.ALCHEMY,
+                PrimarySkillType.REPAIR,
+                PrimarySkillType.SALVAGE,
+                PrimarySkillType.SMELTING
+        );
+
+        /*
+         * Build formatted/localized/etc string lists
+         */
+        this.LOCALIZED_SKILL_NAMES = ImmutableList.copyOf(buildLocalizedPrimarySkillNames());
+        this.FORMATTED_SUBSKILL_NAMES = ImmutableList.copyOf(buildFormattedSubSkillNameList());
+        this.EXACT_SUBSKILL_NAMES = ImmutableSet.copyOf(buildExactSubSkillNameList());
+    }
 
-        //Super hacky and disgusting
-        for (PrimarySkillType primarySkillType1 : PrimarySkillType.values()) {
-            for (SubSkillType subSkillType : SubSkillType.values()) {
-                String[] splitSubSkillName = subSkillType.toString().split("_");
+    @VisibleForTesting
+    @NotNull
+    ImmutableMap<SubSkillType, PrimarySkillType> buildSubSkillParentMap() {
+        EnumMap<SubSkillType, PrimarySkillType> tempSubParentMap =
+                new EnumMap<>(SubSkillType.class);
 
-                if (primarySkillType1.toString().equalsIgnoreCase(splitSubSkillName[0])) {
-                    //Parent Skill Found
-                    tempSubParentMap.put(subSkillType, primarySkillType1);
+        // SubSkillType names use a convention: <PRIMARY>_SOMETHING
+        for (SubSkillType subSkillType : SubSkillType.values()) {
+            String enumName = subSkillType.name();
+            int underscoreIndex = enumName.indexOf('_');
+            String parentPrefix = underscoreIndex == -1
+                    ? enumName
+                    : enumName.substring(0, underscoreIndex);
+
+            for (PrimarySkillType primarySkillType : PrimarySkillType.values()) {
+                if (primarySkillType.name().equalsIgnoreCase(parentPrefix)) {
+                    tempSubParentMap.put(subSkillType, primarySkillType);
+                    break;
                 }
             }
         }
 
-        subSkillParentRelationshipMap = ImmutableMap.copyOf(tempSubParentMap);
+        return ImmutableMap.copyOf(tempSubParentMap);
+    }
 
-        /*
-         * Setup primary -> (collection) subskill map
-         */
+    @VisibleForTesting
+    @NotNull
+    ImmutableMap<PrimarySkillType, Set<SubSkillType>> buildPrimarySkillChildrenMap(
+            ImmutableMap<SubSkillType, PrimarySkillType> subParentMap) {
 
-        EnumMap<PrimarySkillType, Set<SubSkillType>> tempPrimaryChildMap = new EnumMap<>(
-                PrimarySkillType.class);
+        EnumMap<PrimarySkillType, Set<SubSkillType>> tempPrimaryChildMap =
+                new EnumMap<>(PrimarySkillType.class);
 
-        //Init the empty Hash Sets
-        for (PrimarySkillType primarySkillType1 : PrimarySkillType.values()) {
-            tempPrimaryChildMap.put(primarySkillType1, new HashSet<>());
+        // Initialize empty sets
+        for (PrimarySkillType primarySkillType : PrimarySkillType.values()) {
+            tempPrimaryChildMap.put(primarySkillType, new HashSet<>());
         }
 
-        //Fill in the hash sets
+        // Fill sets
         for (SubSkillType subSkillType : SubSkillType.values()) {
-            PrimarySkillType parentSkill = subSkillParentRelationshipMap.get(subSkillType);
-
-            //Add this subskill as a child
-            tempPrimaryChildMap.get(parentSkill).add(subSkillType);
+            PrimarySkillType parentSkill = subParentMap.get(subSkillType);
+            if (parentSkill != null) {
+                tempPrimaryChildMap.get(parentSkill).add(subSkillType);
+            }
         }
 
-        primarySkillChildrenMap = ImmutableMap.copyOf(tempPrimaryChildMap);
+        return ImmutableMap.copyOf(tempPrimaryChildMap);
+    }
 
-        /*
-         * Setup primary -> tooltype map
-         */
-        EnumMap<PrimarySkillType, ToolType> tempToolMap = new EnumMap<>(PrimarySkillType.class);
+    @VisibleForTesting
+    @NotNull
+    ImmutableMap<PrimarySkillType, ToolType> buildPrimarySkillToolMap() {
+        EnumMap<PrimarySkillType, ToolType> tempToolMap =
+                new EnumMap<>(PrimarySkillType.class);
 
         tempToolMap.put(PrimarySkillType.AXES, ToolType.AXE);
         tempToolMap.put(PrimarySkillType.WOODCUTTING, ToolType.AXE);
@@ -120,55 +189,63 @@ public class SkillTools {
         tempToolMap.put(PrimarySkillType.HERBALISM, ToolType.HOE);
         tempToolMap.put(PrimarySkillType.MINING, ToolType.PICKAXE);
 
-        primarySkillToolMap = ImmutableMap.copyOf(tempToolMap);
+        return ImmutableMap.copyOf(tempToolMap);
+    }
 
-        /*
-         * Setup ability -> primary map
-         * Setup primary -> ability map
-         */
+    /**
+     * Holder for the two super ability maps, so we can build them in one pass.
+     */
+    @VisibleForTesting
+    record SuperAbilityMaps(
+            @NotNull ImmutableMap<SuperAbilityType, PrimarySkillType> superAbilityParentRelationshipMap,
+            @NotNull ImmutableMap<PrimarySkillType, SuperAbilityType> mainActivatedAbilityChildMap) {
+    }
 
-        EnumMap<SuperAbilityType, PrimarySkillType> tempAbilityParentRelationshipMap = new EnumMap<>(
-                SuperAbilityType.class);
-        EnumMap<PrimarySkillType, SuperAbilityType> tempMainActivatedAbilityChildMap = new EnumMap<>(
-                PrimarySkillType.class);
+    @VisibleForTesting
+    @NotNull
+    SuperAbilityMaps buildSuperAbilityMaps() {
+        final Map<SuperAbilityType, PrimarySkillType> tempAbilityParentRelationshipMap =
+                new EnumMap<>(SuperAbilityType.class);
+        final Map<PrimarySkillType, SuperAbilityType> tempMainActivatedAbilityChildMap =
+                new EnumMap<>(PrimarySkillType.class);
 
         for (SuperAbilityType superAbilityType : SuperAbilityType.values()) {
-            try {
-                PrimarySkillType parent = getSuperAbilityParent(superAbilityType);
-                tempAbilityParentRelationshipMap.put(superAbilityType, parent);
+            final PrimarySkillType parent = getSuperAbilityParent(superAbilityType);
+            tempAbilityParentRelationshipMap.put(superAbilityType, parent);
 
-                if (superAbilityType != SuperAbilityType.BLAST_MINING) {
-                    //This map is used only for abilities that have a tool readying phase, so blast mining is ignored
-                    tempMainActivatedAbilityChildMap.put(parent, superAbilityType);
-                }
-            } catch (InvalidSkillException e) {
-                e.printStackTrace();
+            // This map is used only for abilities that have a tool readying phase,
+            // so Blast Mining is ignored.
+            if (superAbilityType != SuperAbilityType.BLAST_MINING) {
+                tempMainActivatedAbilityChildMap.put(parent, superAbilityType);
             }
         }
 
-        superAbilityParentRelationshipMap = ImmutableMap.copyOf(tempAbilityParentRelationshipMap);
-        mainActivatedAbilityChildMap = ImmutableMap.copyOf(tempMainActivatedAbilityChildMap);
-
-        /*
-         * Build child skill and nonchild skill lists
-         */
+        return new SuperAbilityMaps(
+                ImmutableMap.copyOf(tempAbilityParentRelationshipMap),
+                ImmutableMap.copyOf(tempMainActivatedAbilityChildMap)
+        );
+    }
 
+    @VisibleForTesting
+    @NotNull
+    ImmutableList<PrimarySkillType> buildChildSkills() {
         List<PrimarySkillType> childSkills = new ArrayList<>();
-
         for (PrimarySkillType primarySkillType : PrimarySkillType.values()) {
             if (isChildSkill(primarySkillType)) {
                 childSkills.add(primarySkillType);
             }
         }
+        return ImmutableList.copyOf(childSkills);
+    }
 
-        CHILD_SKILLS = ImmutableList.copyOf(childSkills);
+    @VisibleForTesting
+    @NotNull
+    ImmutableList<PrimarySkillType> buildCombatSkills() {
+        var gameVersion = mcMMO.getCompatibilityManager().getMinecraftGameVersion();
 
-        /*
-         * Build categorized skill lists
-         */
-        if (mcMMO.getCompatibilityManager().getMinecraftGameVersion().isAtLeast(1, 21, 11)) {
+        if (gameVersion.isAtLeast(1, 21, 11)) {
             // We are in a game version with Spears and Maces
-            COMBAT_SKILLS = ImmutableList.of(
+            return ImmutableList.of(
                     PrimarySkillType.ARCHERY,
                     PrimarySkillType.AXES,
                     PrimarySkillType.CROSSBOWS,
@@ -177,10 +254,11 @@ public class SkillTools {
                     PrimarySkillType.SPEARS,
                     PrimarySkillType.TAMING,
                     PrimarySkillType.TRIDENTS,
-                    PrimarySkillType.UNARMED);
-        } else if (mcMMO.getCompatibilityManager().getMinecraftGameVersion().isAtLeast(1, 21, 0)) {
+                    PrimarySkillType.UNARMED
+            );
+        } else if (gameVersion.isAtLeast(1, 21, 0)) {
             // We are in a game version with Maces
-            COMBAT_SKILLS = ImmutableList.of(
+            return ImmutableList.of(
                     PrimarySkillType.ARCHERY,
                     PrimarySkillType.AXES,
                     PrimarySkillType.CROSSBOWS,
@@ -188,42 +266,23 @@ public class SkillTools {
                     PrimarySkillType.SWORDS,
                     PrimarySkillType.TAMING,
                     PrimarySkillType.TRIDENTS,
-                    PrimarySkillType.UNARMED);
+                    PrimarySkillType.UNARMED
+            );
         } else {
             // No Maces in this version
-            COMBAT_SKILLS = ImmutableList.of(
+            return ImmutableList.of(
                     PrimarySkillType.ARCHERY,
                     PrimarySkillType.AXES,
                     PrimarySkillType.CROSSBOWS,
                     PrimarySkillType.SWORDS,
                     PrimarySkillType.TAMING,
                     PrimarySkillType.TRIDENTS,
-                    PrimarySkillType.UNARMED);
+                    PrimarySkillType.UNARMED
+            );
         }
-        GATHERING_SKILLS = ImmutableList.of(
-                PrimarySkillType.EXCAVATION,
-                PrimarySkillType.FISHING,
-                PrimarySkillType.HERBALISM,
-                PrimarySkillType.MINING,
-                PrimarySkillType.WOODCUTTING);
-        MISC_SKILLS = ImmutableList.of(
-                PrimarySkillType.ACROBATICS,
-                PrimarySkillType.ALCHEMY,
-                PrimarySkillType.REPAIR,
-                PrimarySkillType.SALVAGE,
-                PrimarySkillType.SMELTING);
-
-        /*
-         * Build formatted/localized/etc string lists
-         */
-
-        LOCALIZED_SKILL_NAMES = ImmutableList.copyOf(buildLocalizedPrimarySkillNames());
-        FORMATTED_SUBSKILL_NAMES = ImmutableList.copyOf(buildFormattedSubSkillNameList());
-        EXACT_SUBSKILL_NAMES = ImmutableSet.copyOf(buildExactSubSkillNameList());
     }
 
-    private @NotNull PrimarySkillType getSuperAbilityParent(SuperAbilityType superAbilityType)
-            throws InvalidSkillException {
+    private @NotNull PrimarySkillType getSuperAbilityParent(SuperAbilityType superAbilityType) {
         return switch (superAbilityType) {
             case BERSERK -> PrimarySkillType.UNARMED;
             case GREEN_TERRA -> PrimarySkillType.HERBALISM;
@@ -241,7 +300,7 @@ public class SkillTools {
     }
 
     /**
-     * Makes a list of the "nice" version of sub skill names Used in tab completion mostly
+     * Makes a list of the "nice" version of sub skill names. Used in tab completion mostly.
      *
      * @return a list of formatted sub skill names
      */
@@ -284,9 +343,12 @@ public class SkillTools {
     }
 
     /**
-     * Matches a string of a skill to a skill This is NOT case sensitive First it checks the locale
-     * file and tries to match by the localized name of the skill Then if nothing is found it checks
-     * against the hard coded "name" of the skill, which is just its name in English
+     * Matches a string of a skill to a skill.
+     * This is NOT case-sensitive.
+     * <p>
+     * First it checks the locale file and tries to match by the localized name of the skill.
+     * Then if nothing is found it checks against the hard coded "name" of the skill,
+     * which is just its name in English.
      *
      * @param skillName target skill name
      * @return the matching PrimarySkillType if one is found, otherwise null
@@ -294,8 +356,9 @@ public class SkillTools {
     public PrimarySkillType matchSkill(String skillName) {
         if (!pluginRef.getGeneralConfig().getLocale().equalsIgnoreCase("en_US")) {
             for (PrimarySkillType type : PrimarySkillType.values()) {
-                if (skillName.equalsIgnoreCase(LocaleLoader.getString(
-                        StringUtils.getCapitalized(type.name()) + ".SkillName"))) {
+                String localized = LocaleLoader.getString(
+                        StringUtils.getCapitalized(type.name()) + ".SkillName");
+                if (skillName.equalsIgnoreCase(localized)) {
                     return type;
                 }
             }
@@ -309,15 +372,15 @@ public class SkillTools {
 
         if (!skillName.equalsIgnoreCase("all")) {
             pluginRef.getLogger()
-                    .warning("Invalid mcMMO skill (" + skillName + ")"); //TODO: Localize
+                    .warning("Invalid mcMMO skill (" + skillName + ")"); // TODO: Localize
         }
 
         return null;
     }
 
     /**
-     * Gets the PrimarySkillStype to which a SubSkillType belongs Return null if it does not belong
-     * to one.. which should be impossible in most circumstances
+     * Gets the PrimarySkillType to which a SubSkillType belongs.
+     * Returns null if it does not belong to one (which should be impossible in most circumstances).
      *
      * @param subSkillType target subskill
      * @return the PrimarySkillType of this SubSkill, null if it doesn't exist
@@ -327,8 +390,8 @@ public class SkillTools {
     }
 
     /**
-     * Gets the PrimarySkillStype to which a SuperAbilityType belongs Return null if it does not
-     * belong to one.. which should be impossible in most circumstances
+     * Gets the PrimarySkillType to which a SuperAbilityType belongs.
+     * Returns null if it does not belong to one (which should be impossible in most circumstances).
      *
      * @param superAbilityType target super ability
      * @return the PrimarySkillType of this SuperAbilityType, null if it doesn't exist
@@ -338,16 +401,15 @@ public class SkillTools {
     }
 
     public SuperAbilityType getSuperAbility(PrimarySkillType primarySkillType) {
-        if (mainActivatedAbilityChildMap.get(primarySkillType) == null) {
-            return null;
-        }
-
         return mainActivatedAbilityChildMap.get(primarySkillType);
     }
 
     public boolean isSuperAbilityUnlocked(PrimarySkillType primarySkillType, Player player) {
-        SuperAbilityType superAbilityType = mcMMO.p.getSkillTools()
-                .getSuperAbility(primarySkillType);
+        SuperAbilityType superAbilityType = getSuperAbility(primarySkillType);
+        if (superAbilityType == null) {
+            return false;
+        }
+
         SubSkillType subSkillType = superAbilityType.getSubSkillTypeDefinition();
         return RankUtils.hasUnlockedSubskill(player, subSkillType);
     }
@@ -380,7 +442,6 @@ public class SkillTools {
         return ExperienceConfig.getInstance().getFormulaSkillModifier(primarySkillType);
     }
 
-    // TODO: This is a little "hacky", we probably need to add something to distinguish child skills in the enum, or to use another enum for them
     public static boolean isChildSkill(PrimarySkillType primarySkillType) {
         return switch (primarySkillType) {
             case SALVAGE, SMELTING -> true;
@@ -404,8 +465,10 @@ public class SkillTools {
     }
 
     public boolean canCombatSkillsTrigger(PrimarySkillType primarySkillType, Entity target) {
-        return (target instanceof Player || (target instanceof Tameable
-                && ((Tameable) target).isTamed())) ? getPVPEnabled(primarySkillType)
+        boolean isPlayerOrTamed = (target instanceof Player)
+                || (target instanceof Tameable && ((Tameable) target).isTamed());
+        return isPlayerOrTamed
+                ? getPVPEnabled(primarySkillType)
                 : getPVEEnabled(primarySkillType);
     }
 
@@ -422,7 +485,7 @@ public class SkillTools {
     }
 
     public int getLevelCap(@NotNull PrimarySkillType primarySkillType) {
-        return mcMMO.p.getGeneralConfig().getLevelCap(primarySkillType);
+        return pluginRef.getGeneralConfig().getLevelCap(primarySkillType);
     }
 
     /**
@@ -457,17 +520,12 @@ public class SkillTools {
     }
 
     public @NotNull ImmutableList<PrimarySkillType> getChildSkillParents(
-            PrimarySkillType childSkill)
-            throws IllegalArgumentException {
-        switch (childSkill) {
-            case SALVAGE -> {
-                return SALVAGE_PARENTS;
-            }
-            case SMELTING -> {
-                return SMELTING_PARENTS;
-            }
+            PrimarySkillType childSkill) throws IllegalArgumentException {
+        return switch (childSkill) {
+            case SALVAGE -> SALVAGE_PARENTS;
+            case SMELTING -> SMELTING_PARENTS;
             default -> throw new IllegalArgumentException(
                     "Skill " + childSkill + " is not a child skill");
-        }
+        };
     }
 }

src/test/java/com/gmail/nossr50/util/skills/SkillToolsTest.java

@@ -0,0 +1,371 @@
+package com.gmail.nossr50.util.skills;
+
+import static org.assertj.core.api.Assertions.assertThat;
+import static org.assertj.core.api.Assertions.assertThatThrownBy;
+import static org.mockito.ArgumentMatchers.anyString;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.when;
+
+import com.gmail.nossr50.config.GeneralConfig;
+import com.gmail.nossr50.datatypes.skills.PrimarySkillType;
+import com.gmail.nossr50.datatypes.skills.SubSkillType;
+import com.gmail.nossr50.datatypes.skills.SuperAbilityType;
+import com.gmail.nossr50.datatypes.skills.ToolType;
+import com.gmail.nossr50.locale.LocaleLoader;
+import com.gmail.nossr50.mcMMO;
+import com.gmail.nossr50.util.compat.CompatibilityManager;
+import com.gmail.nossr50.util.platform.MinecraftGameVersion;
+import java.util.ArrayList;
+import java.util.Arrays;
+import java.util.Collections;
+import java.util.HashSet;
+import java.util.List;
+import java.util.Locale;
+import java.util.logging.Logger;
+import java.util.stream.Collectors;
+import org.jetbrains.annotations.NotNull;
+import org.junit.jupiter.api.AfterAll;
+import org.junit.jupiter.api.BeforeAll;
+import org.junit.jupiter.api.Test;
+import org.junit.jupiter.api.TestInstance;
+import org.junit.jupiter.api.TestInstance.Lifecycle;
+import org.mockito.MockedStatic;
+import org.mockito.Mockito;
+
+@TestInstance(Lifecycle.PER_CLASS)
+class SkillToolsTest {
+
+    private static final @NotNull Logger logger = Logger.getLogger(Logger.GLOBAL_LOGGER_NAME);
+
+    private static MockedStatic<mcMMO> mockedMcMMO;
+    private static MockedStatic<LocaleLoader> mockedLocaleLoader;
+
+    private GeneralConfig generalConfig;
+    private CompatibilityManager compatibilityManager;
+
+    @BeforeAll
+    void setUpAll() {
+        // Static mcMMO + LocaleLoader mocks
+        mockedMcMMO = Mockito.mockStatic(mcMMO.class);
+        mockedLocaleLoader = Mockito.mockStatic(LocaleLoader.class);
+
+        // Plugin instance
+        mcMMO.p = mock(mcMMO.class);
+        when(mcMMO.p.getLogger()).thenReturn(logger);
+
+        // General config
+        generalConfig = mock(GeneralConfig.class);
+        when(mcMMO.p.getGeneralConfig()).thenReturn(generalConfig);
+        when(generalConfig.getLocale()).thenReturn("en_US");
+
+        // Compatibility manager + game version
+        compatibilityManager = mock(CompatibilityManager.class);
+        when(mcMMO.getCompatibilityManager()).thenReturn(compatibilityManager);
+
+        // LocaleLoader – just echo key back to keep things simple/deterministic
+        mockedLocaleLoader.when(() -> LocaleLoader.getString(anyString()))
+                .thenAnswer(invocation -> invocation.getArgument(0));
+    }
+
+    @AfterAll
+    void tearDownAll() {
+        mockedLocaleLoader.close();
+        mockedMcMMO.close();
+    }
+
+    private SkillTools newSkillToolsForVersion(int major, int minor, int patch) throws Exception {
+        when(compatibilityManager.getMinecraftGameVersion())
+                .thenReturn(new MinecraftGameVersion(major, minor, patch));
+        return new SkillTools(mcMMO.p);
+    }
+
+    // ------------------------------------------------------------------------
+    // NON_CHILD_SKILLS / isChildSkill / CHILD_SKILLS
+    // ------------------------------------------------------------------------
+
+    @Test
+    void nonChildSkillsShouldContainAllPrimarySkillsExceptSalvageAndSmelting() {
+        List<PrimarySkillType> expected = Arrays.stream(PrimarySkillType.values())
+                .filter(t -> t != PrimarySkillType.SALVAGE && t != PrimarySkillType.SMELTING)
+                .collect(Collectors.toList());
+
+        assertThat(SkillTools.NON_CHILD_SKILLS)
+                .containsExactlyInAnyOrderElementsOf(expected);
+    }
+
+    @Test
+    void isChildSkillShouldReturnTrueOnlyForSalvageAndSmelting() {
+        for (PrimarySkillType type : PrimarySkillType.values()) {
+            boolean isChild = SkillTools.isChildSkill(type);
+
+            if (type == PrimarySkillType.SALVAGE || type == PrimarySkillType.SMELTING) {
+                assertThat(isChild)
+                        .as("%s should be considered a child skill", type)
+                        .isTrue();
+            } else {
+                assertThat(isChild)
+                        .as("%s should NOT be considered a child skill", type)
+                        .isFalse();
+            }
+        }
+    }
+
+    @Test
+    void childSkillsListShouldMatchIsChildSkillClassification() throws Exception {
+        SkillTools skillTools = newSkillToolsForVersion(1, 21, 11);
+
+        List<PrimarySkillType> expectedChildren = Arrays.stream(PrimarySkillType.values())
+                .filter(SkillTools::isChildSkill)
+                .collect(Collectors.toList());
+
+        assertThat(skillTools.getChildSkills())
+                .containsExactlyInAnyOrderElementsOf(expectedChildren);
+    }
+
+    // ------------------------------------------------------------------------
+    // Child skill parents (SALVAGE_PARENTS / SMELTING_PARENTS / getChildSkillParents)
+    // ------------------------------------------------------------------------
+
+    @Test
+    void childSkillParentsShouldMatchStaticParentLists() throws Exception {
+        SkillTools skillTools = newSkillToolsForVersion(1, 21, 11);
+
+        assertThat(skillTools.getChildSkillParents(PrimarySkillType.SALVAGE))
+                .as("SALVAGE parents")
+                .containsExactlyElementsOf(SkillTools.SALVAGE_PARENTS);
+
+        assertThat(skillTools.getChildSkillParents(PrimarySkillType.SMELTING))
+                .as("SMELTING parents")
+                .containsExactlyElementsOf(SkillTools.SMELTING_PARENTS);
+    }
+
+    @Test
+    void getChildSkillParentsShouldThrowForNonChildSkill() throws Exception {
+        SkillTools skillTools = newSkillToolsForVersion(1, 21, 11);
+
+        assertThatThrownBy(() -> skillTools.getChildSkillParents(PrimarySkillType.MINING))
+                .isInstanceOf(IllegalArgumentException.class)
+                .hasMessageContaining("is not a child skill");
+    }
+
+    // ------------------------------------------------------------------------
+    // Super ability ↔ primary skill relationships
+    // ------------------------------------------------------------------------
+
+    @Test
+    void superAbilityParentMappingShouldMatchDefinedSwitch() throws Exception {
+        SkillTools skillTools = newSkillToolsForVersion(1, 21, 11);
+
+        assertThat(skillTools.getPrimarySkillBySuperAbility(SuperAbilityType.BERSERK))
+                .isEqualTo(PrimarySkillType.UNARMED);
+        assertThat(skillTools.getPrimarySkillBySuperAbility(SuperAbilityType.GREEN_TERRA))
+                .isEqualTo(PrimarySkillType.HERBALISM);
+        assertThat(skillTools.getPrimarySkillBySuperAbility(SuperAbilityType.TREE_FELLER))
+                .isEqualTo(PrimarySkillType.WOODCUTTING);
+        assertThat(skillTools.getPrimarySkillBySuperAbility(SuperAbilityType.SUPER_BREAKER))
+                .isEqualTo(PrimarySkillType.MINING);
+        assertThat(skillTools.getPrimarySkillBySuperAbility(SuperAbilityType.BLAST_MINING))
+                .isEqualTo(PrimarySkillType.MINING);
+        assertThat(skillTools.getPrimarySkillBySuperAbility(SuperAbilityType.SKULL_SPLITTER))
+                .isEqualTo(PrimarySkillType.AXES);
+        assertThat(skillTools.getPrimarySkillBySuperAbility(SuperAbilityType.SERRATED_STRIKES))
+                .isEqualTo(PrimarySkillType.SWORDS);
+        assertThat(skillTools.getPrimarySkillBySuperAbility(SuperAbilityType.GIGA_DRILL_BREAKER))
+                .isEqualTo(PrimarySkillType.EXCAVATION);
+        assertThat(skillTools.getPrimarySkillBySuperAbility(SuperAbilityType.SUPER_SHOTGUN))
+                .isEqualTo(PrimarySkillType.CROSSBOWS);
+        assertThat(skillTools.getPrimarySkillBySuperAbility(SuperAbilityType.TRIDENTS_SUPER_ABILITY))
+                .isEqualTo(PrimarySkillType.TRIDENTS);
+        assertThat(skillTools.getPrimarySkillBySuperAbility(SuperAbilityType.EXPLOSIVE_SHOT))
+                .isEqualTo(PrimarySkillType.ARCHERY);
+        assertThat(skillTools.getPrimarySkillBySuperAbility(SuperAbilityType.MACES_SUPER_ABILITY))
+                .isEqualTo(PrimarySkillType.MACES);
+        assertThat(skillTools.getPrimarySkillBySuperAbility(SuperAbilityType.SPEARS_SUPER_ABILITY))
+                .isEqualTo(PrimarySkillType.SPEARS);
+    }
+
+    @Test
+    void mainActivatedAbilityChildMapShouldOmitBlastMiningAndMapOthersBackToAbility() throws Exception {
+        SkillTools skillTools = newSkillToolsForVersion(1, 21, 11);
+
+        // All super abilities EXCEPT BLAST_MINING should be discoverable via getSuperAbility()
+        assertThat(skillTools.getSuperAbility(PrimarySkillType.MINING))
+                .as("MINING should not expose BLAST_MINING as the 'main' tool-readied ability")
+                .isEqualTo(SuperAbilityType.SUPER_BREAKER);
+
+        assertThat(skillTools.getSuperAbility(PrimarySkillType.UNARMED))
+                .isEqualTo(SuperAbilityType.BERSERK);
+        assertThat(skillTools.getSuperAbility(PrimarySkillType.HERBALISM))
+                .isEqualTo(SuperAbilityType.GREEN_TERRA);
+        assertThat(skillTools.getSuperAbility(PrimarySkillType.WOODCUTTING))
+                .isEqualTo(SuperAbilityType.TREE_FELLER);
+        assertThat(skillTools.getSuperAbility(PrimarySkillType.AXES))
+                .isEqualTo(SuperAbilityType.SKULL_SPLITTER);
+        assertThat(skillTools.getSuperAbility(PrimarySkillType.SWORDS))
+                .isEqualTo(SuperAbilityType.SERRATED_STRIKES);
+        assertThat(skillTools.getSuperAbility(PrimarySkillType.EXCAVATION))
+                .isEqualTo(SuperAbilityType.GIGA_DRILL_BREAKER);
+        assertThat(skillTools.getSuperAbility(PrimarySkillType.CROSSBOWS))
+                .isEqualTo(SuperAbilityType.SUPER_SHOTGUN);
+        assertThat(skillTools.getSuperAbility(PrimarySkillType.TRIDENTS))
+                .isEqualTo(SuperAbilityType.TRIDENTS_SUPER_ABILITY);
+        assertThat(skillTools.getSuperAbility(PrimarySkillType.ARCHERY))
+                .isEqualTo(SuperAbilityType.EXPLOSIVE_SHOT);
+        assertThat(skillTools.getSuperAbility(PrimarySkillType.MACES))
+                .isEqualTo(SuperAbilityType.MACES_SUPER_ABILITY);
+        assertThat(skillTools.getSuperAbility(PrimarySkillType.SPEARS))
+                .isEqualTo(SuperAbilityType.SPEARS_SUPER_ABILITY);
+
+        // Skills without a main activated ability should return null
+        assertThat(skillTools.getSuperAbility(PrimarySkillType.REPAIR)).isNull();
+        assertThat(skillTools.getSuperAbility(PrimarySkillType.FISHING)).isNull();
+    }
+
+    // ------------------------------------------------------------------------
+    // Sub-skill → primary-skill mapping (name prefix convention)
+    // ------------------------------------------------------------------------
+
+    @Test
+    void primarySkillBySubSkillShouldFollowNamePrefixConvention() throws Exception {
+        SkillTools skillTools = newSkillToolsForVersion(1, 21, 11);
+
+        for (SubSkillType sub : SubSkillType.values()) {
+            PrimarySkillType parent = skillTools.getPrimarySkillBySubSkill(sub);
+
+            assertThat(parent)
+                    .as("SubSkill %s should have a parent PrimarySkillType", sub)
+                    .isNotNull();
+
+            String subName = sub.name().toUpperCase(Locale.ENGLISH);
+            String parentPrefix = parent.name().toUpperCase(Locale.ENGLISH);
+
+            assertThat(subName.startsWith(parentPrefix))
+                    .as("SubSkill %s should start with its parent skill name %s", subName, parentPrefix)
+                    .isTrue();
+        }
+    }
+
+    // ------------------------------------------------------------------------
+    // primarySkillToolMap
+    // ------------------------------------------------------------------------
+
+    @Test
+    void primarySkillToolTypeMappingShouldMatchDefinition() throws Exception {
+        SkillTools skillTools = newSkillToolsForVersion(1, 21, 11);
+
+        assertThat(skillTools.getPrimarySkillToolType(PrimarySkillType.AXES))
+                .isEqualTo(ToolType.AXE);
+        assertThat(skillTools.getPrimarySkillToolType(PrimarySkillType.WOODCUTTING))
+                .isEqualTo(ToolType.AXE);
+        assertThat(skillTools.getPrimarySkillToolType(PrimarySkillType.UNARMED))
+                .isEqualTo(ToolType.FISTS);
+        assertThat(skillTools.getPrimarySkillToolType(PrimarySkillType.SWORDS))
+                .isEqualTo(ToolType.SWORD);
+        assertThat(skillTools.getPrimarySkillToolType(PrimarySkillType.EXCAVATION))
+                .isEqualTo(ToolType.SHOVEL);
+        assertThat(skillTools.getPrimarySkillToolType(PrimarySkillType.HERBALISM))
+                .isEqualTo(ToolType.HOE);
+        assertThat(skillTools.getPrimarySkillToolType(PrimarySkillType.MINING))
+                .isEqualTo(ToolType.PICKAXE);
+
+        // And any skill not explicitly mapped should currently return null
+        assertThat(skillTools.getPrimarySkillToolType(PrimarySkillType.FISHING)).isNull();
+        assertThat(skillTools.getPrimarySkillToolType(PrimarySkillType.TAMING)).isNull();
+    }
+
+    // ------------------------------------------------------------------------
+    // Combat / Gathering / Misc groupings by Minecraft version
+    // ------------------------------------------------------------------------
+
+    @Test
+    void combatGatheringMiscGroupingsShouldMatchDefinitionForModernSpearsAndMacesVersion()
+            throws Exception {
+        SkillTools skillTools = newSkillToolsForVersion(1, 21, 11);
+
+        assertThat(skillTools.getCombatSkills())
+                .containsExactly(
+                        PrimarySkillType.ARCHERY,
+                        PrimarySkillType.AXES,
+                        PrimarySkillType.CROSSBOWS,
+                        PrimarySkillType.MACES,
+                        PrimarySkillType.SWORDS,
+                        PrimarySkillType.SPEARS,
+                        PrimarySkillType.TAMING,
+                        PrimarySkillType.TRIDENTS,
+                        PrimarySkillType.UNARMED
+                );
+
+        assertThat(skillTools.getGatheringSkills())
+                .containsExactly(
+                        PrimarySkillType.EXCAVATION,
+                        PrimarySkillType.FISHING,
+                        PrimarySkillType.HERBALISM,
+                        PrimarySkillType.MINING,
+                        PrimarySkillType.WOODCUTTING
+                );
+
+        assertThat(skillTools.getMiscSkills())
+                .containsExactly(
+                        PrimarySkillType.ACROBATICS,
+                        PrimarySkillType.ALCHEMY,
+                        PrimarySkillType.REPAIR,
+                        PrimarySkillType.SALVAGE,
+                        PrimarySkillType.SMELTING
+                );
+    }
+
+    @Test
+    void combatSkillsShouldMatchDefinitionForVersionWithMacesButWithoutSpears() throws Exception {
+        SkillTools skillTools = newSkillToolsForVersion(1, 21, 0);
+
+        assertThat(skillTools.getCombatSkills())
+                .containsExactly(
+                        PrimarySkillType.ARCHERY,
+                        PrimarySkillType.AXES,
+                        PrimarySkillType.CROSSBOWS,
+                        PrimarySkillType.MACES,
+                        PrimarySkillType.SWORDS,
+                        PrimarySkillType.TAMING,
+                        PrimarySkillType.TRIDENTS,
+                        PrimarySkillType.UNARMED
+                );
+    }
+
+    @Test
+    void combatSkillsShouldMatchDefinitionForVersionWithoutMacesOrSpears() throws Exception {
+        SkillTools skillTools = newSkillToolsForVersion(1, 20, 4);
+
+        assertThat(skillTools.getCombatSkills())
+                .containsExactly(
+                        PrimarySkillType.ARCHERY,
+                        PrimarySkillType.AXES,
+                        PrimarySkillType.CROSSBOWS,
+                        PrimarySkillType.SWORDS,
+                        PrimarySkillType.TAMING,
+                        PrimarySkillType.TRIDENTS,
+                        PrimarySkillType.UNARMED
+                );
+    }
+
+    // ------------------------------------------------------------------------
+    // LOCALIZED_SKILL_NAMES basic sanity (size + uniqueness, not content)
+    // ------------------------------------------------------------------------
+
+    @Test
+    void localizedSkillNamesShouldContainOneEntryPerPrimarySkillAndBeSorted() throws Exception {
+        SkillTools skillTools = newSkillToolsForVersion(1, 21, 11);
+
+        List<String> names = new ArrayList<>(skillTools.LOCALIZED_SKILL_NAMES);
+
+        // One per PrimarySkillType
+        assertThat(names).hasSize(PrimarySkillType.values().length);
+
+        // No duplicates
+        assertThat(new HashSet<>(names)).hasSize(names.size());
+
+        // Sorted ascending
+        List<String> sorted = new ArrayList<>(names);
+        Collections.sort(sorted);
+        assertThat(names).isEqualTo(sorted);
+    }
+}