Merge pull request #3148 from Windchild292/dev_Windchild_3146

3146: Fixing NPEs in Campaign::getForceFor to handle NPE raised from unit assignment.
MegaMek · Mar 9, 2022 · 48f8d58 · 48f8d58
2 parents 40497dd + 88ce57b
commit 48f8d58
Show file tree

Hide file tree

Showing 3 changed files with 43 additions and 36 deletions.
diff --git a/MekHQ/src/mekhq/campaign/Campaign.java b/MekHQ/src/mekhq/campaign/Campaign.java
@@ -1,7 +1,8 @@
 /*
  * Campaign.java
  *
- * Copyright (c) 2009 Jay Lawson (jaylawson39 at yahoo.com). All rights reserved.
+ * Copyright (c) 2009 - Jay Lawson (jaylawson39 at yahoo.com). All Rights Reserved.
+ * Copyright (c) 2022 - The MegaMek Team. All Rights Reserved.
  *
  * This file is part of MekHQ.
  *
@@ -3755,26 +3756,24 @@ public void removeUnitFromForce(Unit u) {
             }
 
 
-            if (campaignOptions.getUseAtB() && force.getUnits().size() == 0) {
+            if (campaignOptions.getUseAtB() && force.getUnits().isEmpty()) {
                 lances.remove(force.getId());
             }
         }
     }
 
-    public Force getForceFor(Unit u) {
-        return getForce(u.getForceId());
+    public @Nullable Force getForceFor(final @Nullable Unit unit) {
+        return (unit == null) ? null : getForce(unit.getForceId());
     }
 
-    public Force getForceFor(Person p) {
-        Unit u = p.getUnit();
-        if (u != null) {
-            return getForceFor(u);
-        } else if (p.isTech()) {
-            for (Force force : forceIds.values()) {
-                if (p.getId().equals(force.getTechID())) {
-                    return force;
-                }
-            }
+    public @Nullable Force getForceFor(final Person person) {
+        final Unit unit = person.getUnit();
+        if (unit != null) {
+            return getForceFor(unit);
+        } else if (person.isTech()) {
+            return forceIds.values().stream()
+                    .filter(force -> person.getId().equals(force.getTechID()))
+                    .findFirst().orElse(null);
         }
 
         return null;
@@ -3793,6 +3792,7 @@ public void restore() {
                     partsToRemove.add(part);
                 }
             }
+
             if (part instanceof MissingEquipmentPart) {
                 ((MissingEquipmentPart) part).restore();
                 if (null == ((MissingEquipmentPart) part).getType()) {

diff --git a/MekHQ/src/mekhq/campaign/log/ServiceLogger.java b/MekHQ/src/mekhq/campaign/log/ServiceLogger.java
@@ -18,13 +18,15 @@
  */
 package mekhq.campaign.log;
 
+import megamek.common.annotations.Nullable;
 import megamek.common.util.EncodeControl;
 import mekhq.MekHQ;
 import mekhq.campaign.Campaign;
 import mekhq.campaign.force.Force;
 import mekhq.campaign.personnel.Person;
 import mekhq.campaign.personnel.enums.GenderDescriptors;
 import mekhq.campaign.personnel.enums.PersonnelStatus;
+import org.apache.logging.log4j.LogManager;
 
 import java.text.MessageFormat;
 import java.time.LocalDate;
@@ -163,8 +165,12 @@ public static void addedToTOEForce(Campaign campaign, Person person, LocalDate d
         }
     }
 
-    public static void reassignedTOEForce(Campaign campaign, Person person, LocalDate date, Force oldForce, Force newForce) {
+    public static void reassignedTOEForce(final Campaign campaign, final Person person,
+                                          final LocalDate date, final @Nullable Force oldForce,
+                                          final @Nullable Force newForce) {
         if ((oldForce == null) && (newForce == null)) {
+            LogManager.getLogger().error(String.format("Cannot reassign %s on %s because both specified forces are null",
+                    person.getFullTitle(), date));
             return;
         }
 

diff --git a/MekHQ/src/mekhq/campaign/unit/Unit.java b/MekHQ/src/mekhq/campaign/unit/Unit.java
@@ -4273,43 +4273,44 @@ public void removeTech() {
         }
     }
 
-    private void ensurePersonIsRegistered(Person p) {
-        Objects.requireNonNull(p);
-        if (null == getCampaign().getPerson(p.getId())) {
-            getCampaign().recruitPerson(p, p.getPrisonerStatus(), true,  false);
-            LogManager.getLogger().warn(String.format("The person %s added this unit %s, was not in the campaign.", p.getFullName(), getName()));
+    private void ensurePersonIsRegistered(final Person person) {
+        Objects.requireNonNull(person);
+        if (getCampaign().getPerson(person.getId()) == null) {
+            getCampaign().recruitPerson(person, person.getPrisonerStatus(), true,  false);
+            LogManager.getLogger().warn(String.format("The person %s added this unit %s, was not in the campaign.", person.getFullName(), getName()));
         }
     }
 
-    public void addPilotOrSoldier(Person p) {
-        addPilotOrSoldier(p, false);
+    public void addPilotOrSoldier(final Person person) {
+        addPilotOrSoldier(person, false);
     }
 
-    public void addPilotOrSoldier(Person p, boolean useTransfers) {
-        addPilotOrSoldier(p, useTransfers, null);
+    public void addPilotOrSoldier(final Person person, final boolean useTransfers) {
+        addPilotOrSoldier(person, null, useTransfers);
     }
 
-    public void addPilotOrSoldier(Person p, boolean useTransfers, Unit oldUnit) {
-        Objects.requireNonNull(p);
+    public void addPilotOrSoldier(final Person person, final @Nullable Unit oldUnit,
+                                  final boolean useTransfers) {
+        Objects.requireNonNull(person);
 
-        ensurePersonIsRegistered(p);
-        drivers.add(p);
-        //Multi-crew cockpits should not set the pilot to the gunner position
+        ensurePersonIsRegistered(person);
+        drivers.add(person);
+        // Multi-crew cockpits should not set the pilot to the gunner position
         if (entity.getCrew().getCrewType().getPilotPos() == entity.getCrew().getCrewType().getGunnerPos()) {
-            gunners.add(p);
+            gunners.add(person);
         }
-        p.setUnit(this);
+        person.setUnit(this);
         resetPilotAndEntity();
         if (useTransfers) {
-            ServiceLogger.reassignedTo(p, getCampaign().getLocalDate(), getName());
-            ServiceLogger.reassignedTOEForce(getCampaign(), p, getCampaign().getLocalDate(),
+            ServiceLogger.reassignedTo(person, getCampaign().getLocalDate(), getName());
+            ServiceLogger.reassignedTOEForce(getCampaign(), person, getCampaign().getLocalDate(),
                     getCampaign().getForceFor(oldUnit), getCampaign().getForceFor(this));
         } else {
-            ServiceLogger.assignedTo(p, getCampaign().getLocalDate(), getName());
-            ServiceLogger.addedToTOEForce(getCampaign(), p, getCampaign().getLocalDate(),
+            ServiceLogger.assignedTo(person, getCampaign().getLocalDate(), getName());
+            ServiceLogger.addedToTOEForce(getCampaign(), person, getCampaign().getLocalDate(),
                     getCampaign().getForceFor(this));
         }
-        MekHQ.triggerEvent(new PersonCrewAssignmentEvent(p, this));
+        MekHQ.triggerEvent(new PersonCrewAssignmentEvent(person, this));
     }
 
     /**