PersonnelRole Enum #2352
PersonnelRole Enum #2352
Conversation
Windchild292
added
Bug
(RFE) Enhancement
| @@ -2536,16 +2270,15 @@ public int hashCode() { | |||
| } | |||
|
|
|||
| public int getExperienceLevel(boolean secondary) { | |||
There was a problem hiding this comment.
I'm not certain the logic for this method should stay here or be moved to the PersonnelRole Enum given how much it relies on the latter, but the heavy reliance on Personnel is why I've kept it here.
| */ | ||
| public static boolean isSupportRole(int role) { | ||
| return (role >= T_MECH_TECH) && (role < T_LAM_PILOT); | ||
| public boolean hasSupportRole(boolean excludeUnmarketable) { |
There was a problem hiding this comment.
Key note for review: the boolean has been flipped, so anyone using the former value will be doing so incorrectly.
Windchild292
added
the
Hydra
Codecov Report
@@ Coverage Diff @@
## master #2352 +/- ##
============================================
+ Coverage 10.65% 10.71% +0.06%
- Complexity 3740 3763 +23
============================================
Files 692 693 +1
Lines 97250 97239 -11
Branches 16287 16276 -11
============================================
+ Hits 10364 10424 +60
+ Misses 85514 85456 -58
+ Partials 1372 1359 -13
Continue to review full report at Codecov.
|
…tion of newer salaries if one does not already have them
| totalCombatPersonnel++; | ||
| } else if (p.hasPrimarySupportRole(false)) { | ||
| } else { |
There was a problem hiding this comment.
This seems to imply that "dependents" and "no role" count as support personnel.
There was a problem hiding this comment.
In the vast majority of cases they are. I've not, as per the overall description, finalized the dependent role, but am planning on them being largely considered as support personnel.
There was a problem hiding this comment.
This is also consistent with their current implementation in MekHQ.
| } else { | ||
| return OTHER_RANSOM_VALUES.get(getExperienceLevel(false)); | ||
| } | ||
| return (getPrimaryRole().isMechWarriorGrouping() || getPrimaryRole().isAerospacePilot() |
There was a problem hiding this comment.
Again, not to be flippant, but do you really think that this ternary operator statement is more readable than the slightly longer but cleaner if/else block?
There was a problem hiding this comment.
I do for now, but will look at this specific example in the near future as I want to make the ransom values customizable.
|
Ready for re-review @NickAragua. |
This PR implements PersonnelRole as an Enum, a grouping of role-related bugfixes, and adds the basis of the Dependent Role. There will be more changes in future for this role, and there may be bugs for it. However, I believe the bugs are minimal as part of the larger implementation and they will be fixed in the near future as I move to a full rollout of the dependent role alongside #1886.
To review:
0.47.X: Do not.
Post-0.48.0: The goal is for this to be merged in for 0.49.0.
State: