Use BFS class for GameMap.getDistance(). (#11914)

* Use BFS class for GameMap.getDistance().

* Fix test.

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

@@ -4,6 +4,7 @@
 import static com.google.common.base.Preconditions.checkNotNull;
 
 import com.google.common.annotations.VisibleForTesting;
+import games.strategy.engine.data.util.BreadthFirstSearch;
 import games.strategy.triplea.delegate.Matches;
 import java.math.BigDecimal;
 import java.util.ArrayList;
@@ -375,37 +376,9 @@ public int getDistance(
     if (t1.equals(t2)) {
       return 0;
     }
-    return getDistance(0, new HashSet<>(), Set.of(t1), t2, routeCond);
-  }
-
-  /**
-   * Guaranteed that frontier doesn't contain target. Territories on the frontier are not target.
-   * They represent the extent of paths already searched. Territories in searched have already been
-   * on the frontier.
-   */
-  private int getDistance(
-      final int distance,
-      final Set<Territory> searched,
-      final Set<Territory> frontier,
-      final Territory target,
-      final BiPredicate<Territory, Territory> routeCond) {
-
-    // add the frontier to the searched
-    searched.addAll(frontier);
-    // find the new frontier
-
-    final Set<Territory> newFrontier =
-        frontier.stream()
-            .flatMap(f -> getNeighbors(f, routeCond).stream())
-            .collect(Collectors.toSet());
-    if (newFrontier.contains(target)) {
-      return distance + 1;
-    }
-    newFrontier.removeAll(searched);
-    if (newFrontier.isEmpty()) {
-      return -1;
-    }
-    return getDistance(distance + 1, searched, newFrontier, target, routeCond);
+    var territoryFinder = new BreadthFirstSearch.TerritoryFinder(t2);
+    new BreadthFirstSearch(List.of(t1), routeCond).traverse(territoryFinder);
+    return territoryFinder.getDistanceFound();
   }
 
   public IntegerMap<Territory> getDistance(

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

@@ -7,7 +7,9 @@
 import java.util.HashSet;
 import java.util.List;
 import java.util.Set;
+import java.util.function.BiPredicate;
 import java.util.function.Predicate;
+import lombok.Getter;
 import org.triplea.java.ObjectUtils;
 import org.triplea.java.collections.CollectionUtils;
 
@@ -20,7 +22,7 @@ public final class BreadthFirstSearch {
   private final GameMap map;
   private final Set<Territory> visited;
   private final ArrayDeque<Territory> territoriesToCheck;
-  private final Predicate<Territory> neighborCondition;
+  private final BiPredicate<Territory, Territory> neighborCondition;
 
   @FunctionalInterface
   public interface Visitor {
@@ -34,19 +36,50 @@ public interface Visitor {
     boolean visit(Territory territory, int distance);
   }
 
+  /**
+   * Visitor implementation for finding a specific territory. The resulting distance can be accessed
+   * using the getDistanceFound() getter, which will return -1 when not found.
+   *
+   * <p>Note: The Visitor does not get called with any of the starting territories, so -1 will be
+   * returned when passing one of them. If handling such a case is needed, it should be done by the
+   * caller before running the BreadthFirstSearch.
+   */
+  public static class TerritoryFinder implements Visitor {
+    final Territory destination;
+    @Getter int distanceFound = -1;
+
+    public TerritoryFinder(Territory destination) {
+      this.destination = destination;
+    }
+
+    @Override
+    public boolean visit(Territory territory, int distance) {
+      if (destination.equals(territory)) {
+        distanceFound = distance;
+        return false;
+      }
+      return true;
+    }
+  }
+
   /**
    * @param startTerritories The territories from where to start the search.
    * @param neighborCondition Condition that neighboring territories must match to be considered
    *     neighbors.
    */
   public BreadthFirstSearch(
-      Collection<Territory> startTerritories, Predicate<Territory> neighborCondition) {
+      Collection<Territory> startTerritories, BiPredicate<Territory, Territory> neighborCondition) {
     this.map = CollectionUtils.getAny(startTerritories).getData().getMap();
     this.visited = new HashSet<>(startTerritories);
     this.territoriesToCheck = new ArrayDeque<>(startTerritories);
     this.neighborCondition = neighborCondition;
   }
 
+  public BreadthFirstSearch(
+      Collection<Territory> startTerritories, Predicate<Territory> neighborCondition) {
+    this(startTerritories, (it, it2) -> neighborCondition.test(it2));
+  }
+
   public BreadthFirstSearch(Territory startTerritory, Predicate<Territory> neighborCondition) {
     this(List.of(startTerritory), neighborCondition);
   }
@@ -82,10 +115,10 @@ private Territory checkNextTerritory(Visitor visitor, int currentDistance) {
     final Territory territory = territoriesToCheck.removeFirst();
     // Note: The condition isn't passed to getNeighbors() because that implementation is very slow.
     for (final Territory neighbor : map.getNeighbors(territory)) {
-      if (!visited.contains(neighbor) && neighborCondition.test(neighbor)) {
+      if (!visited.contains(neighbor) && neighborCondition.test(territory, neighbor)) {
         visited.add(neighbor);
 
-        final boolean shouldContinueSearch = visitor.visit(neighbor, currentDistance);
+        final boolean shouldContinueSearch = visitor.visit(neighbor, currentDistance + 1);
         if (!shouldContinueSearch) {
           territoriesToCheck.clear();
           break;

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

@@ -2405,24 +2405,21 @@ private Optional<Territory> findDestinationOrSafeTerritoryOnTheWay(
     MutableObject<Territory> destination = new MutableObject<>();
     BreadthFirstSearch bfs = new BreadthFirstSearch(from, canMoveThrough);
     bfs.traverse(
-        new BreadthFirstSearch.Visitor() {
-          @Override
-          public boolean visit(Territory t, int distance) {
-            // If it's a desired final destination, see if we can move towards it.
-            if (finalDestinationTest.test(t)) {
-              Route r = data.getMap().getRouteForUnit(from, t, canMoveThrough, unit, player);
-              while (r != null && r.hasSteps()) {
-                final ProTerritory proDestination = proData.getProTerritory(moveMap, r.getEnd());
-                if (proDestination.isCanHold() && validateMove.test(r)) {
-                  destination.setValue(r.getEnd());
-                  // End the search.
-                  return false;
-                }
-                r = new Route(from, r.getMiddleSteps());
+        (t, distance) -> {
+          // If it's a desired final destination, see if we can move towards it.
+          if (finalDestinationTest.test(t)) {
+            Route r = data.getMap().getRouteForUnit(from, t, canMoveThrough, unit, player);
+            while (r != null && r.hasSteps()) {
+              final ProTerritory proDestination = proData.getProTerritory(moveMap, r.getEnd());
+              if (proDestination.isCanHold() && validateMove.test(r)) {
+                destination.setValue(r.getEnd());
+                // End the search.
+                return false;
               }
+              r = new Route(from, r.getMiddleSteps());
             }
-            return true;
           }
+          return true;
         });
     // If nothing chosen and we can't hold the current territory, try to move somewhere safe.
     if (destination.getValue() == null && !moveMap.get(from).isCanHold()) {

game-app/game-core/src/main/java/games/strategy/triplea/ai/pro/util/ProTerritoryValueUtils.java

@@ -187,13 +187,10 @@ static int findMaxLandMassSize(final GamePlayer player) {
         final int[] landMassSize = new int[1];
         new BreadthFirstSearch(t, cond)
             .traverse(
-                new BreadthFirstSearch.Visitor() {
-                  @Override
-                  public boolean visit(Territory territory, int distance) {
-                    visited.add(territory);
-                    landMassSize[0]++;
-                    return true;
-                  }
+                (territory, distance) -> {
+                  visited.add(territory);
+                  landMassSize[0]++;
+                  return true;
                 });
         if (landMassSize[0] > maxLandMassSize) {
           maxLandMassSize = landMassSize[0];
@@ -462,7 +459,7 @@ public boolean visit(Territory territory, int distance) {
               }
 
               public boolean shouldContinueSearch() {
-                return currentDistance < MIN_FACTORY_CHECK_DISTANCE || found.isEmpty();
+                return currentDistance <= MIN_FACTORY_CHECK_DISTANCE || found.isEmpty();
               }
             });
     return found;

game-app/game-core/src/test/java/games/strategy/engine/data/GameMapTest.java

@@ -0,0 +1,35 @@
+package games.strategy.engine.data;
+
+import static org.hamcrest.MatcherAssert.assertThat;
+import static org.hamcrest.Matchers.is;
+
+import games.strategy.triplea.xml.TestMapGameData;
+import org.junit.jupiter.api.Test;
+
+public class GameMapTest {
+  private final GameData gameData = TestMapGameData.REVISED.getGameData();
+  private final Territory caucasus = gameData.getMap().getTerritory("Caucasus");
+  private final Territory germany = gameData.getMap().getTerritory("Germany");
+  private final Territory russia = gameData.getMap().getTerritory("Russia");
+  private final Territory uk = gameData.getMap().getTerritory("United Kingdom");
+
+  private int getLandDistance(Territory from, Territory to) {
+    return gameData.getMap().getLandDistance(from, to);
+  }
+
+  @Test
+  void testLandDistance() {
+    assertThat(getLandDistance(caucasus, russia), is(1));
+    assertThat(getLandDistance(caucasus, germany), is(3));
+  }
+
+  @Test
+  void testLandDistanceNotFound() {
+    assertThat(getLandDistance(caucasus, uk), is(-1));
+  }
+
+  @Test
+  void testLandDistanceSameTerritory() {
+    assertThat(getLandDistance(caucasus, caucasus), is(0));
+  }
+}

game-app/game-core/src/test/java/games/strategy/engine/data/MapTest.java

@@ -5,6 +5,7 @@
 import static org.junit.jupiter.api.Assertions.assertNotNull;
 import static org.junit.jupiter.api.Assertions.assertNull;
 import static org.junit.jupiter.api.Assertions.assertTrue;
+import static org.mockito.Mockito.when;
 
 import java.util.Set;
 import org.junit.jupiter.api.BeforeEach;
@@ -98,6 +99,7 @@ void setUp() {
     map.addConnection(bd, cd);
     map.addConnection(cd, dd);
     nowhere = new Territory("nowhere", false, gameData);
+    when(gameData.getMap()).thenReturn(map);
   }
 
   @Test

game-app/game-core/src/test/java/games/strategy/engine/data/util/BreadthFirstSearchTest.java

@@ -0,0 +1,43 @@
+package games.strategy.engine.data.util;
+
+import static org.hamcrest.MatcherAssert.assertThat;
+import static org.hamcrest.Matchers.is;
+
+import games.strategy.engine.data.GameData;
+import games.strategy.engine.data.Territory;
+import games.strategy.triplea.delegate.Matches;
+import games.strategy.triplea.xml.TestMapGameData;
+import org.junit.jupiter.api.Test;
+
+public class BreadthFirstSearchTest {
+  private final GameData gameData = TestMapGameData.REVISED.getGameData();
+  private final Territory caucasus = gameData.getMap().getTerritory("Caucasus");
+  private final Territory germany = gameData.getMap().getTerritory("Germany");
+  private final Territory russia = gameData.getMap().getTerritory("Russia");
+  private final Territory uk = gameData.getMap().getTerritory("United Kingdom");
+
+  private int getLandDistance(Territory from, Territory to) {
+    var territoryFinder = new BreadthFirstSearch.TerritoryFinder(to);
+    new BreadthFirstSearch(from, Matches.territoryIsLand()).traverse(territoryFinder);
+    return territoryFinder.getDistanceFound();
+  }
+
+  @Test
+  void testLandDistance() {
+    assertThat(getLandDistance(caucasus, russia), is(1));
+    assertThat(getLandDistance(caucasus, germany), is(3));
+  }
+
+  @Test
+  void testLandDistanceNotFound() {
+    assertThat(getLandDistance(caucasus, uk), is(-1));
+  }
+
+  @Test
+  void testLandDistanceSameTerritory() {
+    // Note: This is testing the limitation described in the API doc.
+    // This is a test for the low-level helper class, but the high level API, which is tested by
+    // GameMapTest.testLandDistanceSameTerritory() returns the expected result of 0.
+    assertThat(getLandDistance(caucasus, caucasus), is(-1));
+  }
+}