Speed up canal validation code.

game-app/game-core/src/main/java/games/strategy/triplea/attachments/CanalAttachment.java

@@ -12,7 +12,9 @@
 import games.strategy.triplea.Constants;
 import games.strategy.triplea.delegate.Matches;
 import java.util.HashSet;
+import java.util.List;
 import java.util.Set;
+import java.util.function.Predicate;
 import java.util.stream.Collectors;
 import javax.annotation.Nullable;
 import org.triplea.java.collections.CollectionUtils;
@@ -34,23 +36,34 @@ public CanalAttachment(final String name, final Attachable attachable, final Gam
     super(name, attachable, gameData);
   }
 
+  private static boolean hasCanal(final Territory t, final String canalName) {
+    return !get(t, canalAttachment -> canalAttachment.getCanalName().equals(canalName)).isEmpty();
+  }
+
+  public static List<CanalAttachment> get(final Territory t, final Route onRoute) {
+    return get(t, attachment -> isCanalOnRoute(attachment.getCanalName(), onRoute));
+  }
+
+  private static List<CanalAttachment> get(final Territory t, Predicate<CanalAttachment> cond) {
+    return t.getAttachments().values().stream()
+        .filter(attachment -> attachment.getName().startsWith(Constants.CANAL_ATTACHMENT_PREFIX))
+        .map(CanalAttachment.class::cast)
+        .filter(cond)
+        .collect(Collectors.toList());
+  }
+
+  static CanalAttachment get(final Territory t, final String nameOfAttachment) {
+    return getAttachment(t, nameOfAttachment, CanalAttachment.class);
+  }
+
   /**
    * Checks if the route contains both territories to pass through the given canal. If route is null
    * returns true.
    */
-  public static boolean isCanalOnRoute(final String canalName, final Route route) {
-    if (route == null) {
-      return true;
-    }
+  private static boolean isCanalOnRoute(final String canalName, final Route route) {
     boolean previousTerritoryHasCanal = false;
     for (final Territory t : route) {
-      boolean currentTerritoryHasCanal = false;
-      for (final CanalAttachment canalAttachment : get(t)) {
-        if (canalAttachment.getCanalName().equals(canalName)) {
-          currentTerritoryHasCanal = true;
-          break;
-        }
-      }
+      boolean currentTerritoryHasCanal = hasCanal(t, canalName);
       if (previousTerritoryHasCanal && currentTerritoryHasCanal) {
         return true;
       }
@@ -59,17 +72,6 @@ public static boolean isCanalOnRoute(final String canalName, final Route route)
     return false;
   }
 
-  public static Set<CanalAttachment> get(final Territory t) {
-    return t.getAttachments().values().stream()
-        .filter(attachment -> attachment.getName().startsWith(Constants.CANAL_ATTACHMENT_PREFIX))
-        .map(CanalAttachment.class::cast)
-        .collect(Collectors.toSet());
-  }
-
-  static CanalAttachment get(final Territory t, final String nameOfAttachment) {
-    return getAttachment(t, nameOfAttachment, CanalAttachment.class);
-  }
-
   private void setCanalName(final String name) {
     canalName = name.intern();
   }
@@ -162,11 +164,8 @@ public void validate(final GameState data) throws GameParseException {
     }
     final Set<Territory> territories = new HashSet<>();
     for (final Territory t : data.getMap()) {
-      final Set<CanalAttachment> canalAttachments = get(t);
-      for (final CanalAttachment canalAttachment : canalAttachments) {
-        if (canalAttachment.getCanalName().equals(canalName)) {
-          territories.add(t);
-        }
+      if (hasCanal(t, canalName)) {
+        territories.add(t);
       }
     }
     if (territories.size() != 2) {

game-app/game-core/src/main/java/games/strategy/triplea/delegate/move/validation/MoveValidator.java

@@ -193,10 +193,9 @@ private MoveValidationResult validateFirst(
                   .negate());
       if (matches.isEmpty() || !matches.stream().allMatch(Matches.unitIsOwnedBy(player))) {
         result.setError(
-            "Player, "
-                + player.getName()
-                + ", is not owner of all the units: "
-                + MyFormatter.unitsToTextNoOwner(units));
+            String.format(
+                "Player, %s, is not owner of all the units: %s",
+                player.getName(), MyFormatter.unitsToTextNoOwner(units)));
         return result;
       }
     }
@@ -216,9 +215,8 @@ private MoveValidationResult validateFirst(
     final Collection<Territory> landOnRoute = route.getMatches(Matches.territoryIsLand());
     if (!landOnRoute.isEmpty()) {
       // TODO: if this ever changes, we need to also update getBestRoute(), because getBestRoute is
-      // also checking to
-      // make sure we avoid land territories owned by nations with these 2 relationship type
-      // attachment options
+      // also checking to make sure we avoid land territories owned by nations with these 2
+      // relationship type attachment options
       for (final Territory t : landOnRoute) {
         if (units.stream().anyMatch(Matches.unitIsLand())
             && !data.getRelationshipTracker().canMoveLandUnitsOverOwnedLand(player, t.getOwner())) {
@@ -291,7 +289,7 @@ private MoveValidationResult validateCanal(
    */
   public @Nullable String validateCanal(
       final Route route, @Nullable final Collection<Unit> units, final GamePlayer player) {
-    return validateCanal(route, units, new HashMap<>(), player);
+    return validateCanal(route, units, Map.of(), player);
   }
 
   @Nullable
@@ -300,6 +298,20 @@ String validateCanal(
       @Nullable final Collection<Unit> units,
       final Map<Unit, Collection<Unit>> airTransportDependents,
       final GamePlayer player) {
+    Map<Territory, Collection<CanalAttachment>> territoryCanals = new HashMap<>();
+    int numCanals = 0;
+    for (Territory t : route.getAllTerritories()) {
+      // Only check canals that are on the route
+      var canals = CanalAttachment.get(t, route);
+      territoryCanals.put(t, canals);
+      numCanals += canals.size();
+    }
+    if (numCanals == 0) {
+      return null;
+    }
+    final boolean mustControlAllCanals =
+        Properties.getControlAllCanalsBetweenTerritoriesToPass(data.getProperties());
+
     // Check each unit 1 by 1 to see if they can move through necessary canals on route
     String result = null;
     final Set<Unit> unitsThatFailCanal = new HashSet<>();
@@ -310,14 +322,9 @@ String validateCanal(
     for (final Unit unit : unitsWithoutDependents) {
       for (final Territory t : route.getAllTerritories()) {
         Optional<String> failureMessage = Optional.empty();
-        for (final CanalAttachment canalAttachment : CanalAttachment.get(t)) {
-          if (!CanalAttachment.isCanalOnRoute(canalAttachment.getCanalName(), route)) {
-            continue; // Only check canals that are on the route
-          }
+        for (CanalAttachment canalAttachment : territoryCanals.get(t)) {
           failureMessage = canPassThroughCanal(canalAttachment, unit, player);
           final boolean canPass = failureMessage.isEmpty();
-          final boolean mustControlAllCanals =
-              Properties.getControlAllCanalsBetweenTerritoriesToPass(data.getProperties());
           if (mustControlAllCanals != canPass) {
             // If need to control any canal and can pass OR need to control all and can't pass.
             break;
@@ -365,10 +372,7 @@ public boolean canAnyUnitsPassCanal(
       final GamePlayer player) {
     boolean canPass = true;
     final Route route = new Route(start, end);
-    for (final CanalAttachment canalAttachment : CanalAttachment.get(start)) {
-      if (!CanalAttachment.isCanalOnRoute(canalAttachment.getCanalName(), route)) {
-        continue; // Only check canals that are on the route
-      }
+    for (final CanalAttachment canalAttachment : CanalAttachment.get(start, route)) {
       final Collection<Unit> unitsWithoutDependents = findNonDependentUnits(units, route, Map.of());
       canPass = canAnyPassThroughCanal(canalAttachment, unitsWithoutDependents, player).isEmpty();
       final boolean mustControlAllCanals =