Sort battles cleanup

game-app/game-core/src/main/java/games/strategy/engine/data/GameSequence.java

@@ -21,9 +21,9 @@ public GameSequence(final GameData data) {
   }
 
   /**
-   * Only used when we are trying to export the data to a savegame, and we need to change the round
-   * and step to something other than the current round and step (because we are creating a savegame
-   * at a certain point in history, for example).
+   * Only used when we are trying to export the data to a save game, and we need to change the round
+   * and step to something other than the current round and step (because we are creating a save
+   * game at a certain point in history, for example).
    */
   public synchronized void setRoundAndStep(
       final int currentRound, final String stepDisplayName, final GamePlayer player) {

game-app/game-core/src/main/java/games/strategy/engine/framework/startup/launcher/ServerLauncher.java

@@ -227,7 +227,7 @@ private void launchInternal() {
         } else {
           final String errorMessage =
               "Unrecognized error occurred. If this is a repeatable error, "
-                  + "please make a copy of this savegame and report to:\n"
+                  + "please make a copy of this save game and report to:\n"
                   + UrlConstants.GITHUB_ISSUES;
           log.error(errorMessage, e);
           stopGame();
@@ -310,7 +310,7 @@ private void stopGame() {
   }
 
   private void saveAndEndGame(final INode node) {
-    // a hack, if headless save to the autosave to avoid polluting our savegames folder with a
+    // a hack, if headless save to the auto save to avoid polluting our save games folder with a
     // million saves
     final Path f = launchAction.getAutoSaveFile();
     try {

game-app/game-core/src/main/java/games/strategy/engine/posted/game/pbem/IEmailSender.java

@@ -23,7 +23,7 @@ public interface IEmailSender {
    *
    * @param subject the subject of the email
    * @param htmlMessage the html email body
-   * @param saveGame the savegame or null
+   * @param saveGame the save game or null
    * @throws IOException if an error occurs
    */
   void sendEmail(String subject, String htmlMessage, Path saveGame, String saveGameName)

game-app/game-core/src/main/java/games/strategy/engine/posted/game/pbf/NodeBbForumPoster.java

@@ -162,7 +162,7 @@ private String uploadSaveGame(
         return parseSaveGameUrlFromJsonResponse(json);
       }
       throw new IllegalStateException(
-          "Failed to upload savegame, server returned Error Code "
+          "Failed to upload save game, server returned Error Code "
               + status
               + "\nMessage:\n"
               + EntityUtils.toString(response.getEntity()));

game-app/game-core/src/main/java/games/strategy/triplea/ai/AbstractAi.java

@@ -601,11 +601,12 @@ protected void battle(final IBattleDelegate battleDelegate) {
     // as in the case of a naval battle preceding an amphibious attack, keep trying to fight every
     // battle
     while (true) {
-      final BattleListing listing = battleDelegate.getBattles();
+      final BattleListing listing = battleDelegate.getBattleListing();
       if (listing.isEmpty()) {
         return;
       }
-      for (final Entry<BattleType, Collection<Territory>> entry : listing.getBattles().entrySet()) {
+      for (final Entry<BattleType, Collection<Territory>> entry :
+          listing.getBattlesMap().entrySet()) {
         for (final Territory current : entry.getValue()) {
           final String error =
               battleDelegate.fightBattle(current, entry.getKey().isBombingRun(), entry.getKey());

game-app/game-core/src/main/java/games/strategy/triplea/ai/pro/simulate/ProSimulateTurnUtils.java

@@ -50,7 +50,7 @@ public static void simulateBattles(
 
     final BattleDelegate battleDelegate = data.getBattleDelegate();
     final Map<BattleType, Collection<Territory>> battleTerritories =
-        battleDelegate.getBattles().getBattles();
+        battleDelegate.getBattleListing().getBattlesMap();
     for (final Entry<BattleType, Collection<Territory>> entry : battleTerritories.entrySet()) {
       for (final Territory t : entry.getValue()) {
         final IBattle battle =

game-app/game-core/src/main/java/games/strategy/triplea/delegate/battle/BattleDelegate.java

@@ -193,7 +193,7 @@ public void loadState(final Serializable state) {
 
   @Override
   public boolean delegateCurrentlyRequiresUserInput() {
-    final BattleListing battles = getBattles();
+    final BattleListing battles = getBattleListing();
     if (battles.isEmpty()) {
       final IBattle battle = getCurrentBattle();
       return battle != null;
@@ -252,8 +252,8 @@ private static String getFightingWord(final IBattle battle) {
   }
 
   @Override
-  public BattleListing getBattles() {
-    return battleTracker.getPendingBattleSites();
+  public BattleListing getBattleListing() {
+    return battleTracker.getBattleListingFromPendingBattles();
   }
 
   /** Add bombardment units to battles. */
@@ -688,7 +688,7 @@ private void doScrambling() {
     if (!Properties.getScrambleRulesInEffect(data.getProperties())) {
       return;
     }
-    final BattleListing pendingBattleSites = battleTracker.getPendingBattleSites();
+    final BattleListing pendingBattleSites = battleTracker.getBattleListingFromPendingBattles();
     final Set<Territory> territoriesWithBattles =
         pendingBattleSites.getNormalBattlesIncludingAirBattles();
     if (Properties.getCanScrambleIntoAirBattles(data.getProperties())) {
@@ -1210,7 +1210,8 @@ private void doKamikazeSuicideAttacks() {
             .and(Matches.unitCanEvade().negate());
     final boolean onlyWhereThereAreBattlesOrAmphibious =
         Properties.getKamikazeSuicideAttacksOnlyWhereBattlesAre(data.getProperties());
-    final Collection<Territory> pendingBattles = battleTracker.getPendingBattleSites(false);
+    final Collection<Territory> pendingBattles =
+        battleTracker.getPendingBattleSitesWithoutBombing();
     // create a list of all kamikaze zones, listed by enemy
     final Map<GamePlayer, Collection<Territory>> kamikazeZonesByEnemy = new HashMap<>();
     for (final Territory t : data.getMap().getTerritories()) {
@@ -1478,7 +1479,7 @@ private static Collection<Territory> whereCanAirLand(
         (Properties.getNeutralFlyoverAllowed(data.getProperties())
             && !Properties.getNeutralsImpassable(data.getProperties()));
     final Set<Territory> canNotLand = new HashSet<>();
-    canNotLand.addAll(battleTracker.getPendingBattleSites(false));
+    canNotLand.addAll(battleTracker.getPendingBattleSitesWithoutBombing());
     canNotLand.addAll(
         CollectionUtils.getMatches(
             data.getMap().getTerritories(), Matches.territoryHasEnemyUnits(alliedPlayer)));
@@ -1519,7 +1520,7 @@ private static Collection<Territory> whereCanAirLand(
                   possibleTerrs,
                   Matches.territoryHasUnitsThatMatch(Matches.unitIsAlliedCarrier(alliedPlayer))
                       .and(Matches.territoryIsWater())));
-      availableWater.removeAll(battleTracker.getPendingBattleSites(false));
+      availableWater.removeAll(battleTracker.getPendingBattleSitesWithoutBombing());
       // simple calculation, either we can take all the air, or we can't, nothing in the middle
       final int carrierCost = AirMovementValidator.carrierCost(strandedAir);
       final Iterator<Territory> waterIter = availableWater.iterator();

game-app/game-core/src/main/java/games/strategy/triplea/delegate/battle/BattleTracker.java

@@ -38,6 +38,7 @@
 import java.io.Serializable;
 import java.util.ArrayList;
 import java.util.Collection;
+import java.util.Collections;
 import java.util.HashMap;
 import java.util.HashSet;
 import java.util.List;
@@ -433,22 +434,23 @@ private void addEmptyBattle(
             .or(Matches.isTerritoryEnemyAndNotUnownedWaterOrImpassableOrRestricted(gamePlayer));
     final Predicate<Territory> conquerable =
         Matches.territoryIsEmptyOfCombatUnits(gamePlayer).and(passableLandAndNotRestricted);
-    final Collection<Territory> conquered = new ArrayList<>();
+    final Collection<Territory> conqueredTerritories = new ArrayList<>();
     if (canConquerMiddleSteps) {
-      conquered.addAll(route.getMatches(conquerable));
+      conqueredTerritories.addAll(route.getMatches(conquerable));
       // in case we begin in enemy territory, and blitz out of it, check the first territory
       if (!route.getStart().equals(route.getEnd()) && conquerable.test(route.getStart())) {
-        conquered.add(route.getStart());
+        conqueredTerritories.add(route.getStart());
       }
     }
     // we handle the end of the route later
-    conquered.remove(route.getEnd());
-    final Collection<Territory> blitzed =
-        CollectionUtils.getMatches(conquered, Matches.territoryIsBlitzable(gamePlayer));
-    this.blitzed.addAll(CollectionUtils.getMatches(blitzed, Matches.isTerritoryEnemy(gamePlayer)));
+    conqueredTerritories.remove(route.getEnd());
+    final Collection<Territory> blitzedTerritories =
+        CollectionUtils.getMatches(conqueredTerritories, Matches.territoryIsBlitzable(gamePlayer));
+    this.blitzed.addAll(
+        CollectionUtils.getMatches(blitzedTerritories, Matches.isTerritoryEnemy(gamePlayer)));
     this.conquered.addAll(
-        CollectionUtils.getMatches(conquered, Matches.isTerritoryEnemy(gamePlayer)));
-    for (final Territory current : conquered) {
+        CollectionUtils.getMatches(conqueredTerritories, Matches.isTerritoryEnemy(gamePlayer)));
+    for (final Territory current : conqueredTerritories) {
       IBattle nonFight = getPendingBattle(current, BattleType.NORMAL);
       // TODO: if we ever want to scramble to a blitzed territory, then we need to fix this
       if (nonFight == null) {
@@ -885,8 +887,10 @@ public static void captureOrDestroyUnits(
         final Map<String, Tuple<String, IntegerMap<UnitType>>> map =
             u.getUnitAttachment().getWhenCapturedChangesInto();
         final GamePlayer currentOwner = u.getOwner();
-        for (final String value : map.keySet()) {
-          final String[] s = Iterables.toArray(Splitter.on(':').split(value), String.class);
+        for (Map.Entry<String, Tuple<String, IntegerMap<UnitType>>> mapEntry :
+            Collections.unmodifiableSet(map.entrySet())) {
+          final String[] s =
+              Iterables.toArray(Splitter.on(':').split(mapEntry.getKey()), String.class);
           if (!(s[0].equals("any")
               || data.getPlayerList().getPlayerId(s[0]).equals(currentOwner))) {
             continue;
@@ -897,7 +901,7 @@ public static void captureOrDestroyUnits(
           }
           final CompositeChange changes = new CompositeChange();
           final Collection<Unit> toAdd = new ArrayList<>();
-          final Tuple<String, IntegerMap<UnitType>> toCreate = map.get(value);
+          final Tuple<String, IntegerMap<UnitType>> toCreate = mapEntry.getValue();
           final boolean translateAttributes = toCreate.getFirst().equalsIgnoreCase("true");
           for (final UnitType ut : toCreate.getSecond().keySet()) {
             toAdd.addAll(ut.create(toCreate.getSecond().getInt(ut), newOwner));
@@ -1036,17 +1040,15 @@ public Collection<IBattle> getPendingBattles(BattleType type) {
   }
 
   public Collection<IBattle> getPendingBattles(final Territory t) {
-    return pendingBattles.stream()
-        .filter(b -> b.getTerritory().equals(t))
-        .collect(Collectors.toSet());
+    return CollectionUtils.getMatches(pendingBattles, b -> b.getTerritory().equals(t));
   }
 
   public boolean hasPendingNonBombingBattle(final Territory t) {
-    return getPendingBattle(t) != null;
+    return getPendingNonBombingBattle(t) != null;
   }
 
   @Nullable
-  public IBattle getPendingBattle(final Territory t) {
+  public IBattle getPendingNonBombingBattle(final Territory t) {
     return BattleType.nonBombingBattleTypes().stream()
         .map(type -> getPendingBattle(t, type))
         .filter(Objects::nonNull)
@@ -1056,50 +1058,43 @@ public IBattle getPendingBattle(final Territory t) {
 
   @Nullable
   public IBattle getPendingBattle(final Territory t, final BattleType type) {
-    for (final IBattle battle : pendingBattles) {
-      if (battle.getTerritory().equals(t) && type == battle.getBattleType()) {
-        return battle;
-      }
-    }
-    return null;
+    return pendingBattles.stream()
+        .filter(b -> b.getBattleType().equals(type) && b.getTerritory().equals(t))
+        .findFirst()
+        .orElse(null);
   }
 
   public IBattle getPendingBattle(final UUID uuid) {
     if (uuid == null) {
       return null;
     }
-
     return pendingBattles.stream().filter(b -> b.getBattleId().equals(uuid)).findAny().orElse(null);
   }
 
+  /** Returns a collection of territories where no bombing battles are pending. */
+  public Collection<Territory> getPendingBattleSitesWithoutBombing() {
+    return getPendingBattleSites(false);
+  }
+
+  /** Returns a collection of territories where bombing battles are pending. */
+  public Collection<Territory> getPendingBattleSitesWithBombing() {
+    return getPendingBattleSites(true);
+  }
+
   /**
    * Returns a collection of territories where battles are pending.
    *
-   * @param bombing whether only battles where there is bombing.
+   * @param bombing whether only battles where there is bombing or where there is no bombing.
    */
-  public Collection<Territory> getPendingBattleSites(final boolean bombing) {
-    final Collection<Territory> battles = new ArrayList<>();
-    for (final IBattle battle : pendingBattles) {
-      if (!battle.isEmpty() && battle.getBattleType().isBombingRun() == bombing) {
-        battles.add(battle.getTerritory());
-      }
-    }
-    return battles;
+  private Collection<Territory> getPendingBattleSites(final boolean bombing) {
+    return pendingBattles.stream()
+        .filter(b -> !b.isEmpty() && b.getBattleType().isBombingRun() == bombing)
+        .map(IBattle::getTerritory)
+        .collect(Collectors.toSet());
   }
 
-  public BattleListing getPendingBattleSites() {
-    final Map<BattleType, Collection<Territory>> battles = new HashMap<>();
-    for (final IBattle battle : pendingBattles) {
-      if (!battle.isEmpty()) {
-        Collection<Territory> territories = battles.get(battle.getBattleType());
-        if (territories == null) {
-          territories = new HashSet<>();
-        }
-        territories.add(battle.getTerritory());
-        battles.put(battle.getBattleType(), territories);
-      }
-    }
-    return new BattleListing(battles);
+  public BattleListing getBattleListingFromPendingBattles() {
+    return new BattleListing(pendingBattles);
   }
 
   /**
@@ -1131,9 +1126,9 @@ public void addDependency(final IBattle blocked, final IBattle blocking) {
   }
 
   private void removeDependency(final IBattle blocked, final IBattle blocking) {
-    final Collection<IBattle> dependencies = this.dependencies.get(blocked);
-    dependencies.remove(blocking);
-    if (dependencies.isEmpty()) {
+    final Collection<IBattle> dependenciesOfBlocked = this.dependencies.get(blocked);
+    dependenciesOfBlocked.remove(blocking);
+    if (dependenciesOfBlocked.isEmpty()) {
       this.dependencies.remove(blocked);
     }
   }
@@ -1204,7 +1199,7 @@ void sendBattleRecordsToGameData(final IDelegateBridge bridge) {
    */
   void fightAirRaidsAndStrategicBombing(final IDelegateBridge delegateBridge) {
     fightAirRaidsAndStrategicBombing(
-        delegateBridge, () -> getPendingBattleSites(true), this::getPendingBattle);
+        delegateBridge, this::getPendingBattleSitesWithBombing, this::getPendingBattle);
   }
 
   @VisibleForTesting
@@ -1226,7 +1221,7 @@ void fightAirRaidsAndStrategicBombing(
       }
     }
 
-    // now that we've done all the air battles, do all the SBR's as a second wave.
+    // now that we've done all the air battles, do all the SBRs as a second wave.
     for (final Territory t : pendingBattleSiteSupplier.get()) {
       final IBattle bombingRaid = pendingBattleFunction.apply(t, BattleType.BOMBING_RAID);
       if (bombingRaid != null) {