Shared stop sort for non-ferry timetables (#2510)

* shared stop sort

* use time and test

* dialyzer

* docs

Author: anthonyshull

lib/dotcom_web/controllers/schedule/timetable_controller.ex

@@ -90,7 +90,7 @@ defmodule DotcomWeb.ScheduleController.TimetableController do
     header_schedules =
       trip_schedules
       |> Map.values()
-      |> header_schedules()
+      |> Kernel.then(&header_schedules(route, &1))
 
     track_changes = track_changes(trip_schedules, Enum.map(trip_stops, & &1.id))
 
@@ -513,10 +513,15 @@ defmodule DotcomWeb.ScheduleController.TimetableController do
     {{nil, nil}, schedule}
   end
 
-  @spec header_schedules(list) :: list
-  defp header_schedules(timetable_schedules) do
-    timetable_schedules
-    |> Schedules.Sort.sort_by_first_times()
+  defp header_schedules(%Route{description: :ferry}, schedules) do
+    schedules
+    |> Schedules.Sort.sort_by_first_departure()
+    |> Enum.map(&List.first/1)
+  end
+
+  defp header_schedules(%Route{}, schedules) do
+    schedules
+    |> Schedules.Sort.sort_by_first_shared_stop()
     |> Enum.map(&List.first/1)
   end
 

lib/schedules/sort.ex

@@ -8,42 +8,77 @@ defmodule Schedules.Sort do
   @doc """
   Sorts schedules by grouping them into trips and then comparing the first departure.
   """
-  @spec sort_by_first_times([Schedule.t()]) :: [[Schedule.t()]]
-  def sort_by_first_times(schedules) do
+  @spec sort_by_first_departure([Schedule.t()]) :: [[Schedule.t()]]
+  def sort_by_first_departure(schedules) do
     schedules
+    |> Enum.sort_by(& &1.departure_time, &datetime_sorter/2)
     |> Enum.group_by(& &1.trip.id)
-    |> Enum.sort_by(&mapper/1, &sorter/2)
-    |> Enum.map(fn {_, schedules} -> schedules end)
+    |> Enum.sort_by(&mapper/1, &departure_sorter/2)
+    |> Enum.map(&mapper/1)
   end
 
-  # Gets the first departure time for a list of schedules.
-  defp first_departure_time(schedules) do
+  @doc """
+  Sorts schedules by comparing the time of the first shared stop.
+  """
+  @spec sort_by_first_shared_stop([Schedule.t()]) :: [[Schedule.t()]]
+  def sort_by_first_shared_stop(schedules) do
     schedules
-    |> Enum.map(& &1.departure_time)
-    |> Enum.reject(&is_nil/1)
-    |> Enum.sort(&subsorter/2)
-    |> List.first()
+    |> Enum.sort_by(& &1.time, &datetime_sorter/2)
+    |> Enum.group_by(& &1.trip.id)
+    |> Enum.sort_by(&mapper/1, &first_shared_stop_sorter/2)
+    |> Enum.map(&mapper/1)
   end
 
-  # Ignores the trip id and returns the schedules for sorting.
-  defp mapper({_, schedules}), do: schedules
-
   # Sorts two schedule lists by comparing the first departure time of each.
-  defp sorter(a, b) do
+  defp departure_sorter(a, b) do
     a_departure_time = first_departure_time(a)
 
     b_departure_time = first_departure_time(b)
 
-    subsorter(a_departure_time, b_departure_time)
+    datetime_sorter(a_departure_time, b_departure_time)
   end
 
   # Compares two times.
   # Handles the case where one or more are nil.
-  defp subsorter(nil, nil), do: true
-  defp subsorter(nil, _), do: false
-  defp subsorter(_, nil), do: true
+  defp datetime_sorter(nil, nil), do: true
+  defp datetime_sorter(nil, _), do: false
+  defp datetime_sorter(_, nil), do: true
 
-  defp subsorter(a, b) do
+  defp datetime_sorter(a, b) do
     Timex.compare(a, b) < 1
   end
+
+  # Finds the schedule with the given stop id in a list of schedules.
+  defp find_schedule_with_stop(schedules, stop_id) do
+    Enum.find(schedules, fn schedule -> schedule.stop.id === stop_id end)
+  end
+
+  # Gets the first departure time for a list of schedules.
+  defp first_departure_time(schedules) do
+    schedules
+    |> Enum.map(& &1.departure_time)
+    |> Enum.reject(&is_nil/1)
+    |> Enum.sort(&datetime_sorter/2)
+    |> List.first()
+  end
+
+  # Sorts two schedule lists by comparing the departure time of the first shared stop.
+  defp first_shared_stop_sorter(a, b) do
+    a_stop_ids = Enum.map(a, & &1.stop.id)
+    b_stop_ids = Enum.map(b, & &1.stop.id)
+
+    common_stop = Enum.find(a_stop_ids, &Enum.member?(b_stop_ids, &1))
+
+    if common_stop do
+      a_schedule = find_schedule_with_stop(a, common_stop)
+      b_schedule = find_schedule_with_stop(b, common_stop)
+
+      datetime_sorter(a_schedule.time, b_schedule.time)
+    else
+      departure_sorter(a, b)
+    end
+  end
+
+  # Ignores the trip id and returns the schedules for sorting.
+  defp mapper({_, schedules}), do: schedules
 end

test/schedules/sort_test.exs

@@ -6,35 +6,83 @@ defmodule SortTest do
 
   alias Schedules.Schedule
   alias Schedules.Trip
+  alias Stops.Stop
 
   @now Timex.now()
 
   @schedules [
-    %Schedule{departure_time: shift(@now, minutes: 1), trip: %Trip{id: "1"}},
-    %Schedule{departure_time: shift(@now, minutes: 2), trip: %Trip{id: "1"}},
-    %Schedule{departure_time: shift(@now, minutes: 5), trip: %Trip{id: "2"}},
-    %Schedule{departure_time: shift(@now, minutes: 6), trip: %Trip{id: "2"}},
-    %Schedule{departure_time: shift(@now, minutes: 3), trip: %Trip{id: "3"}},
-    %Schedule{departure_time: shift(@now, minutes: 4), trip: %Trip{id: "3"}}
+    %Schedule{
+      departure_time: shift(@now, minutes: 2),
+      stop: %Stop{id: "2"},
+      time: shift(@now, minutes: 2),
+      trip: %Trip{id: "1"}
+    },
+    %Schedule{
+      departure_time: shift(@now, minutes: 1),
+      stop: %Stop{id: "1"},
+      time: shift(@now, minutes: 1),
+      trip: %Trip{id: "1"}
+    },
+    %Schedule{
+      departure_time: shift(@now, minutes: 6),
+      stop: %Stop{id: "2"},
+      time: shift(@now, minutes: 6),
+      trip: %Trip{id: "3"}
+    },
+    %Schedule{
+      departure_time: shift(@now, minutes: 5),
+      stop: %Stop{id: "1"},
+      time: shift(@now, minutes: 5),
+      trip: %Trip{id: "3"}
+    },
+    %Schedule{
+      departure_time: shift(@now, minutes: 4),
+      stop: %Stop{id: "2"},
+      time: shift(@now, minutes: 4),
+      trip: %Trip{id: "2"}
+    },
+    %Schedule{
+      departure_time: shift(@now, minutes: 3),
+      stop: %Stop{id: "1"},
+      time: shift(@now, minutes: 3),
+      trip: %Trip{id: "2"}
+    }
   ]
 
-  describe "sort_by_first_time/1" do
-    test "groups a list of schedules into lists of trips" do
-      result = sort_by_first_times(@schedules)
+  @expected_sorted_trip_stop_ids [
+    {"1", "1"},
+    {"1", "2"},
+    {"2", "1"},
+    {"2", "2"},
+    {"3", "1"},
+    {"3", "2"}
+  ]
+
+  describe "sort_by_first_departure/1" do
+    test "groups by trip and sorts groups on first departure time" do
+      # Setup / Exercise
+      sorted_trip_stop_ids =
+        @schedules
+        |> sort_by_first_departure()
+        |> List.flatten()
+        |> Enum.map(fn schedule -> {schedule.trip.id, schedule.stop.id} end)
 
-      for [first | _] = trip_list <- result do
-        assert Enum.all?(trip_list, &(&1.trip.id == first.trip.id))
-      end
+      # Verify
+      assert sorted_trip_stop_ids == @expected_sorted_trip_stop_ids
     end
+  end
 
-    test "sorts schedules by their time at the first departure" do
-      result = sort_by_first_times(@schedules)
+  describe "sort_by_first_shared_stop/1" do
+    test "groups by trip and sorts groups on first stop in common" do
+      # Setup / Exercise
+      sorted_trip_stop_ids =
+        @schedules
+        |> sort_by_first_shared_stop()
+        |> List.flatten()
+        |> Enum.map(fn schedule -> {schedule.trip.id, schedule.stop.id} end)
 
-      assert Enum.map(result, &List.first/1) == [
-               %Schedule{departure_time: shift(@now, minutes: 1), trip: %Trip{id: "1"}},
-               %Schedule{departure_time: shift(@now, minutes: 3), trip: %Trip{id: "3"}},
-               %Schedule{departure_time: shift(@now, minutes: 5), trip: %Trip{id: "2"}}
-             ]
+      # Verify
+      assert sorted_trip_stop_ids == @expected_sorted_trip_stop_ids
     end
   end
 end