Improvement: Convert Skill Type Storage from Array to Enum #7633
Conversation
… Level is Important
Co-authored-by: Copilot <[email protected]>
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
There was a problem hiding this comment.
CodeQL found more than 20 potential problems in the proposed changes. Check the Files changed tab for more details.
| public boolean isRightTechType(String skillType) { | ||
| if (getEngine().hasFlag(Engine.TANK_ENGINE)) { | ||
| return skillType.equals(SkillType.S_TECH_MECHANIC); | ||
| return skillType.equals(SkillTypeNew.S_TECH_MECHANIC.name()); |
There was a problem hiding this comment.
You'll see a lot of SkillTypeNew.SKILL.name() in this PR. The original SkillType format had all skill references using a static String. That means our codebase is set up to accept Strings and not SkillTypes.
One of my goals with this PR was to make as few changes as possible, to reduce the risk of introducing new bugs with this overhaul. While converting the codebase to use SkillTypes wherever possible would be the correct move, it would balloon the size of this PR beyond reasonable levels. It is better, then, to use legacy architecture that can be replaced ad hoc in future PRs.
# Conflicts: # MekHQ/src/mekhq/campaign/personnel/Person.java # MekHQ/src/mekhq/campaign/personnel/generator/AbstractSkillGenerator.java # MekHQ/src/mekhq/campaign/personnel/skills/Skill.java # MekHQ/src/mekhq/campaign/personnel/skills/SkillType.java
There was a problem hiding this comment.
Pull Request Overview
This pull request performs a major refactoring to migrate from the legacy SkillType class to a new enum-based SkillTypeNew system. The changes modernize skill type handling by converting from a string-based initialization system to a strongly-typed enum approach.
Key changes:
- Introduced
SkillTypeNewenum with all skill types as enum constants - Created
SkillUtilitieshelper class consolidating skill-related static methods - Updated all references from
SkillType.S_*constants toSkillTypeNew.S_*.name() - Removed
SkillType.initializeTypes()calls throughout test files - Changed from
SkillType.getType(String)toSkillTypeNew.getType(String)
Reviewed Changes
Copilot reviewed 140 out of 140 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| SkillTypeNew.java | New enum containing all skill type definitions with their properties |
| SkillUtilities.java | New utility class for experience level names, colors, and skill-related helper methods |
| SkillSubType.java | Added getRoleplaySkillSubTypes() helper method |
| Multiple test files | Removed SkillType.initializeTypes() calls and updated skill constant references |
| Multiple source files | Updated skill type references to use new enum system and .name() method |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| public static SkillTypeNew getType(String name) { | ||
| return SkillTypeNew.valueOf(name); |
There was a problem hiding this comment.
The getType() method lacks null handling and will throw IllegalArgumentException for invalid enum names. Consider adding null checks and graceful error handling with a meaningful return value or logging, especially since this is a public API used throughout the codebase.
| import mekhq.campaign.personnel.skills.SkillType; | ||
| import mekhq.campaign.personnel.skills.SkillUtilities; | ||
| import mekhq.campaign.personnel.skills.enums.SkillTypeNew; | ||
| import mekhq.campaign.personnel.skills.SkillUtilities; |
There was a problem hiding this comment.
Duplicate import of SkillUtilities on lines 46 and 48. Remove the duplicate import statement.
| SkillType skillType = SkillType.getSkillHash().get(skillName); | ||
| if (skillType == null) { | ||
| try { | ||
| SkillTypeNew skillType = SkillTypeNew.valueOf(skillName); |
There was a problem hiding this comment.
Variable 'SkillTypeNew skillType' is never read.
1 participant
This PR relates to the conversion of our current SkillType class from a static String plus hash hybrid and into an enum.
This makes it far easier for developers to add new skills and make changes to existing skills. It makes skill storage more reliable and removes the risk that a skill change will result in damage to a players' save.