From 538349d4e2be6ca436f612fb94a448693e013b85 Mon Sep 17 00:00:00 2001 From: asvitkine Date: Sun, 27 Aug 2023 16:38:10 -0400 Subject: [PATCH] Fix tests and more cleanup. --- .../triplea/attachments/CanalAttachment.java | 27 ++++---- .../move/validation/MoveValidator.java | 62 +++++++++---------- 2 files changed, 45 insertions(+), 44 deletions(-) diff --git a/game-app/game-core/src/main/java/games/strategy/triplea/attachments/CanalAttachment.java b/game-app/game-core/src/main/java/games/strategy/triplea/attachments/CanalAttachment.java index 2b9ca38ad5e..caa2c914b95 100644 --- a/game-app/game-core/src/main/java/games/strategy/triplea/attachments/CanalAttachment.java +++ b/game-app/game-core/src/main/java/games/strategy/triplea/attachments/CanalAttachment.java @@ -13,6 +13,7 @@ import games.strategy.triplea.delegate.Matches; import java.util.HashSet; import java.util.Set; +import java.util.function.Predicate; import java.util.stream.Collectors; import javax.annotation.Nullable; import org.triplea.java.collections.CollectionUtils; @@ -44,13 +45,7 @@ public 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 = !getByCanalName(t, canalName).isEmpty(); if (previousTerritoryHasCanal && currentTerritoryHasCanal) { return true; } @@ -59,10 +54,19 @@ public static boolean isCanalOnRoute(final String canalName, final Route route) return false; } - public static Set get(final Territory t) { + public static Set get(final Territory t, final Route onRoute) { + return get(t, attachment -> isCanalOnRoute(attachment.getCanalName(), onRoute)); + } + + public static Set getByCanalName(final Territory t, final String canalName) { + return get(t, canalAttachment -> canalAttachment.getCanalName().equals(canalName)); + } + + private static Set get(final Territory t, Predicate cond) { return t.getAttachments().values().stream() .filter(attachment -> attachment.getName().startsWith(Constants.CANAL_ATTACHMENT_PREFIX)) .map(CanalAttachment.class::cast) + .filter(cond) .collect(Collectors.toSet()); } @@ -162,11 +166,8 @@ public void validate(final GameState data) throws GameParseException { } final Set territories = new HashSet<>(); for (final Territory t : data.getMap()) { - final Set canalAttachments = get(t); - for (final CanalAttachment canalAttachment : canalAttachments) { - if (canalAttachment.getCanalName().equals(canalName)) { - territories.add(t); - } + if (!getByCanalName(t, canalName).isEmpty()) { + territories.add(t); } } if (territories.size() != 2) { diff --git a/game-app/game-core/src/main/java/games/strategy/triplea/delegate/move/validation/MoveValidator.java b/game-app/game-core/src/main/java/games/strategy/triplea/delegate/move/validation/MoveValidator.java index d5e9cbccab9..75b141494f5 100644 --- a/game-app/game-core/src/main/java/games/strategy/triplea/delegate/move/validation/MoveValidator.java +++ b/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 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())) { @@ -300,36 +298,41 @@ String validateCanal( @Nullable final Collection units, final Map> airTransportDependents, final GamePlayer player) { - List canals = - route.getAllTerritories().stream() - .map(t -> CanalAttachment.get(t)) - .flatMap(Collection::stream) - // Only check canals that are on the route - .filter(c -> CanalAttachment.isCanalOnRoute(c.getCanalName(), route)) - .collect(Collectors.toList()); - if (canals.isEmpty()) { + Map> 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 unitsThatFailCanal = new HashSet<>(); + final Set setWithNull = new HashSet<>(); + setWithNull.add(null); final Collection unitsWithoutDependents = - (units == null) - ? Set.of((Unit) null) - : findNonDependentUnits(units, route, airTransportDependents); + (units == null) ? setWithNull : findNonDependentUnits(units, route, airTransportDependents); for (final Unit unit : unitsWithoutDependents) { - for (CanalAttachment canalAttachment : canals) { - Optional failureMessage = canPassThroughCanal(canalAttachment, unit, player); - final boolean canPass = failureMessage.isEmpty(); - if (mustControlAllCanals != canPass) { - // If need to control any canal and can pass OR need to control all and can't pass. - if (!canPass) { - result = failureMessage.get(); - unitsThatFailCanal.add(unit); + for (final Territory t : route.getAllTerritories()) { + Optional failureMessage = Optional.empty(); + for (CanalAttachment canalAttachment : territoryCanals.get(t)) { + failureMessage = canPassThroughCanal(canalAttachment, unit, player); + final boolean canPass = failureMessage.isEmpty(); + if (mustControlAllCanals != canPass) { + // If need to control any canal and can pass OR need to control all and can't pass. + break; } - break; + } + if (failureMessage.isPresent()) { + result = failureMessage.get(); + unitsThatFailCanal.add(unit); } } } @@ -369,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 unitsWithoutDependents = findNonDependentUnits(units, route, Map.of()); canPass = canAnyPassThroughCanal(canalAttachment, unitsWithoutDependents, player).isEmpty(); final boolean mustControlAllCanals =