refactor(TripPlan.ItineraryGroups): incorporate feedback (#2224)

lib/dotcom/trip_plan/itinerary_groups.ex

@@ -6,9 +6,13 @@ defmodule Dotcom.TripPlan.ItineraryGroups do
   But, this does not include walking legs that are less than 0.2 miles.
   """
 
+  import DotcomWeb.TripPlanView, only: [get_one_way_total_by_type: 2]
+
   alias Dotcom.TripPlan.{Itinerary, Leg, PersonalDetail, TransitDetail}
   alias OpenTripPlannerClient.ItineraryTag
 
+  @short_walk_threshold_minutes 5
+
   @type summarized_leg :: %{
           routes: [Routes.Route.t()],
           walk_minutes: non_neg_integer()
@@ -31,8 +35,8 @@ defmodule Dotcom.TripPlan.ItineraryGroups do
   def from_itineraries(itineraries) do
     itineraries
     |> Enum.group_by(&unique_legs_to_hash/1)
-    |> Enum.map(&group_departures/1)
-    |> Enum.reject(&Enum.empty?(&1))
+    |> Enum.map(&to_summarized_group/1)
+    |> Enum.reject(&Enum.empty?(&1.itineraries))
     |> Enum.sort_by(fn
       %{itineraries: [%{tag: tag} | _] = _} ->
         Enum.find_index(ItineraryTag.tag_priority_order(), &(&1 == tag))
@@ -57,46 +61,60 @@ defmodule Dotcom.TripPlan.ItineraryGroups do
     {Routes.Route.type_atom(route.type), leg.from.name, leg.to.name}
   end
 
-  defp group_departures({_hash, grouped_itineraries}) do
-    summarized_legs =
-      grouped_itineraries
-      |> Enum.map(& &1.legs)
-      |> Enum.zip_with(&Function.identity/1)
-      |> Enum.map(fn legs ->
-        legs
-        |> Enum.uniq_by(&combined_leg_to_tuple/1)
-        |> Enum.map(&to_summarized_leg/1)
-        |> Enum.reduce(%{walk_minutes: 0, routes: []}, &summarize_legs/2)
-      end)
-      |> remove_short_intermediate_walks()
-
-    summary =
-      grouped_itineraries
-      |> Enum.map(fn itinerary ->
-        itinerary
-        |> Map.take([:start, :stop, :tag, :duration, :accessible?, :walk_distance])
-        |> Map.put(
-          :total_cost,
-          DotcomWeb.TripPlanView.get_one_way_total_by_type(itinerary, :highest_one_way_fare)
-        )
-      end)
-      |> summarize_itineraries()
-      |> Map.put(:summarized_legs, summarized_legs)
-
-    %{itineraries: ItineraryTag.sort_tagged(grouped_itineraries), summary: summary}
+  defp to_summarized_group({_hash, grouped_itineraries}) do
+    %{
+      itineraries: ItineraryTag.sort_tagged(grouped_itineraries),
+      summary: summary(grouped_itineraries)
+    }
+  end
+
+  defp summary(itineraries) do
+    itineraries
+    |> Enum.map(&to_map_with_fare/1)
+    |> to_summary()
+    |> Map.put(:summarized_legs, to_summarized_legs(itineraries))
+  end
+
+  defp to_summarized_legs(itineraries) do
+    itineraries
+    |> to_transposed_legs()
+    |> Enum.map(&aggregate_legs/1)
+    |> remove_short_intermediate_walks()
+  end
+
+  defp to_transposed_legs(itineraries) do
+    itineraries
+    |> Enum.map(& &1.legs)
+    |> Enum.zip_with(&Function.identity/1)
+  end
+
+  defp aggregate_legs(legs) do
+    legs
+    |> Enum.uniq_by(&combined_leg_to_tuple/1)
+    |> Enum.map(&to_summarized_leg/1)
+    |> Enum.reduce(%{walk_minutes: 0, routes: []}, &summarize_legs/2)
+  end
+
+  defp to_map_with_fare(itinerary) do
+    itinerary
+    |> Map.take([:start, :stop, :tag, :duration, :accessible?, :walk_distance])
+    |> Map.put(
+      :total_cost,
+      get_one_way_total_by_type(itinerary, :highest_one_way_fare)
+    )
   end
 
   defp remove_short_intermediate_walks(summarized_legs) do
     summarized_legs
     |> Enum.with_index()
     |> Enum.reject(fn {leg, index} ->
       index > 0 && index < Kernel.length(summarized_legs) - 1 &&
-        (leg.routes == [] && leg.walk_minutes < 5)
+        (leg.routes == [] && leg.walk_minutes < @short_walk_threshold_minutes)
     end)
-    |> Enum.map(&elem(&1, 0))
+    |> Enum.map(fn {leg, _} -> leg end)
   end
 
-  defp summarize_itineraries(itinerary_maps) do
+  defp to_summary(itinerary_maps) do
     # for most of the summary we can reflect the first itinerary
     [
       %{

test/dotcom/trip_plan/itinerary_group_test.exs → test/dotcom/trip_plan/itinerary_groups_test.exs

@@ -1,4 +1,4 @@
-defmodule Dotcom.TripPlan.ItineraryGroupTest do
+defmodule Dotcom.TripPlan.ItineraryGroupsTest do
   @moduledoc false
 
   use ExUnit.Case, async: true
@@ -83,7 +83,7 @@ defmodule Dotcom.TripPlan.ItineraryGroupTest do
     end
   end
 
-  test "ignores short walking distances of < 0.2 miles", %{stops: [a, b, c]} do
+  test "ignores short walking distances of < 0.2 miles when grouping", %{stops: [a, b, c]} do
     # SETUP
     bus_a_b_leg = TripPlanner.build(:bus_leg, from: a, to: b)
     walk_b_c_leg = TripPlanner.build(:walking_leg, from: b, to: c) |> Map.put(:distance, 0.199)
@@ -101,4 +101,26 @@ defmodule Dotcom.TripPlan.ItineraryGroupTest do
     # VERIFY
     assert Kernel.length(grouped_itineraries) == 1
   end
+
+  test "ignores intermediate legs with short walking durations of < 5 minutes when summarizing",
+       %{stops: [a, b, c]} do
+    bus_a_b_leg = TripPlanner.build(:bus_leg, from: a, to: b)
+
+    walk_b_c_leg =
+      TripPlanner.build(:walking_leg, from: b, to: c)
+      |> Map.merge(%{distance: 0.5, duration: 4})
+
+    walk_c_b_leg =
+      TripPlanner.build(:walking_leg, from: c, to: b)
+      |> Map.merge(%{distance: 0.5, duration: 6})
+
+    bus_b_a_leg = TripPlanner.build(:bus_leg, from: b, to: a)
+
+    itinerary =
+      TripPlanner.build(:itinerary, legs: [bus_a_b_leg, walk_b_c_leg, walk_c_b_leg, bus_b_a_leg])
+
+    [%{summary: %{summarized_legs: legs}}] = ItineraryGroups.from_itineraries([itinerary])
+
+    assert [_, %{walk_minutes: 6}, _] = legs
+  end
 end