Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Infantry refactor #3944

Merged
merged 8 commits into from
Oct 28, 2022
Merged

Infantry refactor #3944

merged 8 commits into from
Oct 28, 2022

Conversation

SJuliez
Copy link
Member

@SJuliez SJuliez commented Oct 21, 2022

When there, why not improve.

Removes unnecessary comments (for overrides mostly), improves a few method and field names, makes use of methods and improves the way some things are represented in the code.

The real changes are in Infantry, the changes in the other files adapt to changed method names.
This should be merged together with the adaptations for MML and MHQ.

@@ -827,7 +827,7 @@

String infSquadNum = entityTag.getAttribute(INF_SQUAD_NUM);
if (!infSquadNum.isBlank()) {
inf.setSquadN(Integer.parseInt(infSquadNum));
inf.setSquadCount(Integer.parseInt(infSquadNum));

Check notice

Code scanning / CodeQL

Missing catch of NumberFormatException

Potential uncaught 'java.lang.NumberFormatException'.
Copy link
Member

@NickAragua NickAragua left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I'm ok with this for the most part. We'll definitely want to give infantry a good beating in terms of testing after we merge it.

/**
* Infantry only have critical slots for field gun ammo
*/
// Infantry only have critical slots for field gun ammo
private static final int[] NUM_OF_SLOTS = { 20, 20 };
private static final String[] LOCATION_ABBRS = { "MEN", "FGUN" };
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since we're polishing this, can we change the "MEN" location to "troopers" or "soldiers" or something?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like troopers myself. But agree with the men part.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, done. I was afraid this would break things but at least loading a damaged CI from a MUL is compatible across this change.

Copy link
Member

@NickAragua NickAragua left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I think locations might be determined by index rather than name. Although I could be wrong, it's been more than five minutes since I last looked at a MUL file.

@SJuliez SJuliez merged commit 8062c0b into MegaMek:master Oct 28, 2022
@SJuliez SJuliez deleted the Infantry_Refac branch January 6, 2023 09:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants