Apply review feedback

Author: leonardehrenfried

src/ext/java/org/opentripplanner/ext/stopconsolidation/DecorateConsolidatedStopNames.java

@@ -7,9 +7,9 @@
 import org.opentripplanner.routing.algorithm.filterchain.framework.spi.ItineraryDecorator;
 
 /**
- * A decorating filter that checks if a transit leg contains any primary stops and if it does,
- * then replaces it with the secondary, agency-specific stop name. This is so that the in-vehicle
- * display matches what OTP returns as a board/alight stop name.
+ * A decorating filter that checks if a transit leg contains any consolidated stops and if it does,
+ * then replaces it with the appropriate, agency-specific stop name. This is so that the physical
+ * signage and in-vehicle display matches what OTP returns as a board/alight stop name.
  */
 public class DecorateConsolidatedStopNames implements ItineraryDecorator {
 
@@ -21,20 +21,28 @@ public DecorateConsolidatedStopNames(StopConsolidationService service) {
 
   @Override
   public void decorate(Itinerary itinerary) {
-    replacePrimaryNamesWithSecondary(itinerary);
+    replaceConsolidatedStops(itinerary);
   }
 
   /**
-   * If the itinerary has a from/to that is the primary stop of a {@link org.opentripplanner.ext.stopconsolidation.model.ConsolidatedStopGroup}
-   * then we replace its name with the secondary name of the agency that is
-   * operating the route, so that the name in the result matches the name in the in-vehicle
-   * display.
+   * If the itinerary has a "from" stop that is the secondary stop of a
+   * {@link org.opentripplanner.ext.stopconsolidation.model.ConsolidatedStopGroup}
+   * then we replace its name with the primary name of the agency that is
+   * operating the route, so that the name in the result matches the physical signage on the stop.
+   * <p>
+   * If the leg has a "to" stop that is a primary stop, then we don't want to show the stop that's on
+   * the signage but what is shown _inside_ the vehicle. That's why we use the agency-specific (aka
+   * secondary) stop.
+   * <p>
+   * This follows the somewhat idiosyncratic logic of the consolidated stops feature.
    */
-  private void replacePrimaryNamesWithSecondary(Itinerary i) {
+  private void replaceConsolidatedStops(Itinerary i) {
     i.transformTransitLegs(leg -> {
       if (leg instanceof ScheduledTransitLeg stl && needsToRenameStops(stl)) {
         var agency = leg.getAgency();
+        // to show the name on the stop signage we use the primary stop's name
         var from = service.primaryStop(stl.getFrom().stop.getId());
+        // to show the name that's on the display inside the vehicle we use the agency-specific name
         var to = service.agencySpecificStop(stl.getTo().stop, agency);
         return new ConsolidatedStopLeg(stl, from, to);
       } else {
@@ -43,6 +51,14 @@ private void replacePrimaryNamesWithSecondary(Itinerary i) {
     });
   }
 
+  /**
+   * Figures out if the from/to stops are part of a consolidated stop group and therefore
+   * some stops need to be replaced.
+   * <p>
+   * Please consult the Javadoc of {@link DecorateConsolidatedStopNames#replaceConsolidatedStops(Itinerary)}
+   * for details of this idiosyncratic business logic and in particular why the logic is not the same
+   * for the from/to stops.
+   */
   private boolean needsToRenameStops(ScheduledTransitLeg stl) {
     return (service.isSecondaryStop(stl.getFrom().stop) || service.isPrimaryStop(stl.getTo().stop));
   }

src/ext/java/org/opentripplanner/ext/stopconsolidation/internal/DefaultStopConsolidationService.java

@@ -77,12 +77,12 @@ public StopLocation agencySpecificStop(StopLocation stop, Agency agency) {
     if (agency.getId().getFeedId().equals(stop.getId().getFeedId())) {
       return stop;
     } else {
-      return agencySpecificStopOpt(stop, agency).orElse(stop);
+      return findAgencySpecificStop(stop, agency).orElse(stop);
     }
   }
 
   @Nonnull
-  private Optional<StopLocation> agencySpecificStopOpt(StopLocation stop, Agency agency) {
+  private Optional<StopLocation> findAgencySpecificStop(StopLocation stop, Agency agency) {
     return repo
       .groups()
       .stream()