From 3f2e0252d50d383b4e15dc2b0ecbc64a71130816 Mon Sep 17 00:00:00 2001 From: Graham McIntire Date: Tue, 28 Apr 2026 14:18:17 -0500 Subject: [PATCH] refactor: dedupe utilities, prefer pattern matching, tighten specs - consolidate Haversine: CoordinateUtils delegates to GeoUtils; InfoLive.haversine/4 reuses it (m -> km) - AprsSymbol: collapse 3 sprite-info builders onto build_sprite_info/2 and add a sprite_info @type - DataBuilder: 3 weather unit converters share convert_unit/4 with explicit clauses for "N/A", numeric, and binary inputs - mobile_channel: do_callsign_match/2 uses pattern-match clauses for wildcard handling instead of nested if - info_live: replace nested case in position_changed? with multi-clause coords_changed?/1; format_timestamp_for_display pipelines into a guarded build_timestamp_display/1 - api_docs_live: collapse chained single-key assigns into keyword-list assign/2 calls - add strict @spec coverage on all touched helpers; tighten new specs to match dialyzer success typing (no contract_supertype warnings) --- lib/aprsme/geo_utils.ex | 5 + lib/aprsme_web/aprs_symbol.ex | 113 +++++------------ lib/aprsme_web/channels/mobile_channel.ex | 17 +-- lib/aprsme_web/live/api_docs_live.ex | 37 ++---- lib/aprsme_web/live/info_live/show.ex | 114 +++++++----------- lib/aprsme_web/live/map_live/data_builder.ex | 67 +++++----- .../live/shared/coordinate_utils.ex | 26 +--- 7 files changed, 138 insertions(+), 241 deletions(-) diff --git a/lib/aprsme/geo_utils.ex b/lib/aprsme/geo_utils.ex index a396e64..1ff155f 100644 --- a/lib/aprsme/geo_utils.ex +++ b/lib/aprsme/geo_utils.ex @@ -8,11 +8,15 @@ defmodule Aprsme.GeoUtils do @doc """ Calculate the Haversine distance between two points in meters. + Returns `nil` when any coordinate is not a number. + ## Examples iex> Aprsme.GeoUtils.haversine_distance(33.16961, -96.4921, 33.16962, -96.4921) 1.11 """ + @spec haversine_distance(number(), number(), number(), number()) :: float() + @spec haversine_distance(any(), any(), any(), any()) :: nil def haversine_distance(lat1, lon1, lat2, lon2) when is_number(lat1) and is_number(lon1) and is_number(lat2) and is_number(lon2) do # Convert to radians @@ -41,6 +45,7 @@ defmodule Aprsme.GeoUtils do Default threshold is 50 meters to account for typical GPS accuracy variations. """ + @spec significant_movement?(any(), any(), any(), any(), number()) :: boolean() def significant_movement?(lat1, lon1, lat2, lon2, threshold_meters \\ 50) do case haversine_distance(lat1, lon1, lat2, lon2) do nil -> false diff --git a/lib/aprsme_web/aprs_symbol.ex b/lib/aprsme_web/aprs_symbol.ex index 33786d3..64ec2a5 100644 --- a/lib/aprsme_web/aprs_symbol.ex +++ b/lib/aprsme_web/aprs_symbol.ex @@ -25,18 +25,23 @@ defmodule AprsmeWeb.AprsSymbol do background_size: "512px 192px" } """ - @spec get_sprite_info(String.t() | nil, String.t() | nil) :: %{ - sprite_file: String.t(), - background_position: String.t(), - background_size: String.t() - } + @spec get_sprite_info(String.t() | nil, String.t() | nil) :: sprite_info() def get_sprite_info(symbol_table, symbol_code) do compute_sprite_info(overlay_symbol?(symbol_table), symbol_table, symbol_code) end + @typedoc "Sprite-sheet positioning information for a single symbol." + @type sprite_info :: %{ + sprite_file: String.t(), + background_position: String.t(), + background_size: String.t() + } + + @spec overlay_symbol?(any()) :: boolean() defp overlay_symbol?(table) when is_binary(table), do: String.match?(table, ~r/^[A-Z0-9]$/) defp overlay_symbol?(_), do: false + @spec compute_sprite_info(boolean(), String.t() | nil, String.t() | nil) :: sprite_info() defp compute_sprite_info(true, _symbol_table, symbol_code) do get_overlay_base_symbol_info(symbol_code) end @@ -44,21 +49,20 @@ defmodule AprsmeWeb.AprsSymbol do defp compute_sprite_info(false, symbol_table, symbol_code) do symbol_table = normalize_symbol_table(symbol_table) symbol_code = normalize_symbol_code(symbol_code) - table_id = get_table_id(symbol_table) - sprite_file = "/aprs-symbols/aprs-symbols-128-#{table_id}@2x.png" + build_sprite_info(table_id, symbol_code) + end - symbol_code_ord = get_symbol_code_ord(symbol_code) - index = symbol_code_ord - 33 - safe_index = max(0, min(index, 93)) - - column = rem(safe_index, 16) - row = div(safe_index, 16) - x = -column * 128 - y = -row * 128 + # Builds the sprite_info map for a given table and symbol char. + # The 128x128 sprite sheet is a 16-column grid; positions are scaled to 32px display. + @spec build_sprite_info(String.t(), String.t() | nil) :: sprite_info() + defp build_sprite_info(table_id, symbol_char) do + safe_index = symbol_char |> get_symbol_code_ord() |> Kernel.-(33) |> max(0) |> min(93) + x = -rem(safe_index, 16) * 128 + y = -div(safe_index, 16) * 128 %{ - sprite_file: sprite_file, + sprite_file: "/aprs-symbols/aprs-symbols-128-#{table_id}@2x.png", background_position: "#{x / 4}px #{y / 4}px", background_size: "512px 192px" } @@ -68,38 +72,9 @@ defmodule AprsmeWeb.AprsSymbol do Gets sprite information for overlay symbols (A-Z, 0-9). These symbols display the base symbol from the overlay table. """ - @spec get_overlay_base_symbol_info(String.t()) :: %{ - sprite_file: String.t(), - background_position: String.t(), - background_size: String.t() - } + @spec get_overlay_base_symbol_info(String.t()) :: sprite_info() def get_overlay_base_symbol_info(base_symbol_code) do - # Some overlay base symbols are in the alternate table (1), others in overlay table (2) - # Check which table to use based on the symbol code - table_id = get_overlay_base_table_id(base_symbol_code) - sprite_file = "/aprs-symbols/aprs-symbols-128-#{table_id}@2x.png" - - # Get position of the base symbol in the appropriate table - base_symbol_ord = - base_symbol_code - |> String.to_charlist() - |> List.first() - |> then(fn c -> if is_integer(c), do: c, else: 63 end) - - index = base_symbol_ord - 33 - safe_index = max(0, min(index, 93)) - - # Calculate positioning for 16-column grid - column = rem(safe_index, 16) - row = div(safe_index, 16) - x = -column * 128 - y = -row * 128 - - %{ - sprite_file: sprite_file, - background_position: "#{x / 4}px #{y / 4}px", - background_size: "512px 192px" - } + build_sprite_info(get_overlay_base_table_id(base_symbol_code), base_symbol_code) end @doc """ @@ -117,37 +92,8 @@ defmodule AprsmeWeb.AprsSymbol do Gets sprite information for overlay characters (A-Z, 0-9). These are rendered from the overlay table. """ - @spec get_overlay_character_sprite_info(String.t()) :: %{ - sprite_file: String.t(), - background_position: String.t(), - background_size: String.t() - } - def get_overlay_character_sprite_info(overlay_char) do - # Use overlay table (table 2) for the overlay character - sprite_file = "/aprs-symbols/aprs-symbols-128-2@2x.png" - - # Get position of the overlay character in the overlay table - overlay_char_ord = - overlay_char - |> String.to_charlist() - |> List.first() - |> then(fn c -> if is_integer(c), do: c, else: 63 end) - - index = overlay_char_ord - 33 - safe_index = max(0, min(index, 93)) - - # Calculate positioning for 16-column grid - column = rem(safe_index, 16) - row = div(safe_index, 16) - x = -column * 128 - y = -row * 128 - - %{ - sprite_file: sprite_file, - background_position: "#{x / 4}px #{y / 4}px", - background_size: "512px 192px" - } - end + @spec get_overlay_character_sprite_info(String.t()) :: sprite_info() + def get_overlay_character_sprite_info(overlay_char), do: build_sprite_info("2", overlay_char) @doc """ Normalizes a symbol table identifier. @@ -362,8 +308,13 @@ defmodule AprsmeWeb.AprsSymbol do default end - defp get_symbol_code_ord(symbol_code) do - c = symbol_code |> String.to_charlist() |> List.first() - if is_integer(c), do: c, else: 63 + @spec get_symbol_code_ord(String.t() | nil) :: non_neg_integer() + defp get_symbol_code_ord(symbol_code) when is_binary(symbol_code) do + case String.to_charlist(symbol_code) do + [c | _] when is_integer(c) -> c + _ -> 63 + end end + + defp get_symbol_code_ord(_), do: 63 end diff --git a/lib/aprsme_web/channels/mobile_channel.ex b/lib/aprsme_web/channels/mobile_channel.ex index 6f2cce5..68eced5 100644 --- a/lib/aprsme_web/channels/mobile_channel.ex +++ b/lib/aprsme_web/channels/mobile_channel.ex @@ -555,20 +555,21 @@ defmodule AprsmeWeb.MobileChannel do socket end + @spec callsign_matches?(String.t(), String.t()) :: boolean() defp callsign_matches?(packet_callsign, tracked_callsign) do - normalized_packet = String.upcase(packet_callsign) - normalized_tracked = String.upcase(tracked_callsign) - do_callsign_match(normalized_packet, normalized_tracked) + do_callsign_match(String.upcase(packet_callsign), String.upcase(tracked_callsign)) end + @spec do_callsign_match(String.t(), String.t()) :: boolean() defp do_callsign_match(callsign, callsign), do: true + defp do_callsign_match(packet_callsign, "*" <> _ = tracked_callsign), + do: String.starts_with?(packet_callsign, String.replace(tracked_callsign, "*", "")) + defp do_callsign_match(packet_callsign, tracked_callsign) do - if String.contains?(tracked_callsign, "*") do - pattern = String.replace(tracked_callsign, "*", "") - String.starts_with?(packet_callsign, pattern) - else - String.starts_with?(packet_callsign, tracked_callsign <> "-") + case String.split(tracked_callsign, "*", parts: 2) do + [prefix, _suffix] -> String.starts_with?(packet_callsign, prefix) + [_] -> String.starts_with?(packet_callsign, tracked_callsign <> "-") end end diff --git a/lib/aprsme_web/live/api_docs_live.ex b/lib/aprsme_web/live/api_docs_live.ex index 4850c1a..96aa22e 100644 --- a/lib/aprsme_web/live/api_docs_live.ex +++ b/lib/aprsme_web/live/api_docs_live.ex @@ -7,20 +7,18 @@ defmodule AprsmeWeb.ApiDocsLive do @impl true def mount(_params, _session, socket) do {:ok, - socket - |> assign(page_title: "API Documentation") - |> assign(test_callsign: "") - |> assign(loading: false) - |> assign(api_result: nil) - |> assign(error: nil)} + assign(socket, + page_title: "API Documentation", + test_callsign: "", + loading: false, + api_result: nil, + error: nil + )} end @impl true def handle_event("update_callsign", %{"callsign" => callsign}, socket) do - {:noreply, - socket - |> assign(test_callsign: callsign) - |> assign(error: nil)} + {:noreply, assign(socket, test_callsign: callsign, error: nil)} end @impl true @@ -30,15 +28,8 @@ defmodule AprsmeWeb.ApiDocsLive do if normalized_callsign == "" do {:noreply, assign(socket, error: "Please enter a callsign")} else - socket = - socket - |> assign(loading: true) - |> assign(error: nil) - |> assign(api_result: nil) - - # Make the API call send(self(), {:call_api, normalized_callsign}) - {:noreply, socket} + {:noreply, assign(socket, loading: true, error: nil, api_result: nil)} end end @@ -46,16 +37,10 @@ defmodule AprsmeWeb.ApiDocsLive do def handle_info({:call_api, callsign}, socket) do case make_http_request(callsign) do {:ok, json_result} -> - {:noreply, - socket - |> assign(loading: false) - |> assign(api_result: json_result)} + {:noreply, assign(socket, loading: false, api_result: json_result)} {:error, error_message} -> - {:noreply, - socket - |> assign(loading: false) - |> assign(error: error_message)} + {:noreply, assign(socket, loading: false, error: error_message)} end end diff --git a/lib/aprsme_web/live/info_live/show.ex b/lib/aprsme_web/live/info_live/show.ex index 234024a..d8fd414 100644 --- a/lib/aprsme_web/live/info_live/show.ex +++ b/lib/aprsme_web/live/info_live/show.ex @@ -8,6 +8,7 @@ defmodule AprsmeWeb.InfoLive.Show do alias Aprsme.Callsign alias Aprsme.EncodingUtils + alias Aprsme.GeoUtils alias Aprsme.Packets alias AprsmeWeb.AprsSymbol alias AprsmeWeb.Live.SharedPacketHandler @@ -139,39 +140,28 @@ defmodule AprsmeWeb.InfoLive.Show do end end + @spec position_changed?(map() | nil, map() | nil) :: boolean() defp position_changed?(nil, _new_packet), do: true defp position_changed?(_current_packet, nil), do: false defp position_changed?(current_packet, new_packet) do - # Compare lat/lon to see if position actually changed - current_lat = EncodingUtils.to_float(current_packet.lat) - current_lon = EncodingUtils.to_float(current_packet.lon) - new_lat = EncodingUtils.to_float(new_packet.lat) - new_lon = EncodingUtils.to_float(new_packet.lon) - - # If any coordinate is invalid, consider it a change - case {current_lat, current_lon, new_lat, new_lon} do - {nil, _, _, _} -> - true - - {_, nil, _, _} -> - true - - {_, _, nil, _} -> - true - - {_, _, _, nil} -> - true - - {curr_lat, curr_lon, new_lat, new_lon} -> - # Consider position changed if coordinates differ by more than the configured threshold - threshold = Application.get_env(:aprsme, :position_tracking, [])[:change_threshold] || 0.001 - lat_diff = abs(curr_lat - new_lat) - lon_diff = abs(curr_lon - new_lon) - lat_diff > threshold or lon_diff > threshold - end + coords_changed?({ + EncodingUtils.to_float(current_packet.lat), + EncodingUtils.to_float(current_packet.lon), + EncodingUtils.to_float(new_packet.lat), + EncodingUtils.to_float(new_packet.lon) + }) end + @spec coords_changed?({float() | nil, float() | nil, float() | nil, float() | nil}) :: boolean() + defp coords_changed?({curr_lat, curr_lon, new_lat, new_lon}) + when is_number(curr_lat) and is_number(curr_lon) and is_number(new_lat) and is_number(new_lon) do + threshold = Application.get_env(:aprsme, :position_tracking, [])[:change_threshold] || 0.001 + abs(curr_lat - new_lat) > threshold or abs(curr_lon - new_lon) > threshold + end + + defp coords_changed?(_), do: true + # Expose for testing if Mix.env() == :test do def position_changed_for_test(current, new), do: position_changed?(current, new) @@ -254,57 +244,41 @@ defmodule AprsmeWeb.InfoLive.Show do end end + @typep timestamp_display :: %{ + time_ago: String.t(), + formatted: String.t(), + timestamp: String.t() | nil + } + + @spec haversine(any(), any(), any(), any()) :: float() def haversine(lat1, lon1, lat2, lon2) do - # Returns distance in km - # Convert Decimal to float if needed - lat1 = to_float(lat1) - lon1 = to_float(lon1) - lat2 = to_float(lat2) - lon2 = to_float(lon2) - - r = 6371 - dlat = :math.pi() / 180 * (lat2 - lat1) - dlon = :math.pi() / 180 * (lon2 - lon1) - - a = - :math.sin(dlat / 2) * :math.sin(dlat / 2) + - :math.cos(:math.pi() / 180 * lat1) * :math.cos(:math.pi() / 180 * lat2) * - :math.sin(dlon / 2) * :math.sin(dlon / 2) - - c = 2 * :math.atan2(:math.sqrt(a), :math.sqrt(1 - a)) - r * c + # Returns distance in km. Decimals are converted to float; nil/invalid inputs become 0.0. + GeoUtils.haversine_distance(to_float(lat1), to_float(lon1), to_float(lat2), to_float(lon2)) / 1000 end + @spec to_float(any()) :: float() defp to_float(value), do: EncodingUtils.to_float(value) || 0.0 + @spec format_timestamp_for_display(map()) :: timestamp_display() defp format_timestamp_for_display(packet) do - received_at = get_received_at(packet) - - if received_at do - timestamp_dt = AprsmeWeb.TimeHelpers.to_datetime(received_at) - - if timestamp_dt do - %{ - time_ago: AprsmeWeb.TimeHelpers.time_ago_in_words(timestamp_dt), - formatted: Calendar.strftime(timestamp_dt, "%Y-%m-%d %H:%M:%S UTC"), - timestamp: DateTime.to_iso8601(timestamp_dt) - } - else - %{ - time_ago: gettext("Unknown"), - formatted: "", - timestamp: nil - } - end - else - %{ - time_ago: gettext("Unknown"), - formatted: "", - timestamp: nil - } - end + packet + |> get_received_at() + |> AprsmeWeb.TimeHelpers.to_datetime() + |> build_timestamp_display() end + @spec build_timestamp_display(DateTime.t() | nil) :: timestamp_display() + defp build_timestamp_display(nil), do: %{time_ago: gettext("Unknown"), formatted: "", timestamp: nil} + + defp build_timestamp_display(%DateTime{} = dt) do + %{ + time_ago: AprsmeWeb.TimeHelpers.time_ago_in_words(dt), + formatted: Calendar.strftime(dt, "%Y-%m-%d %H:%M:%S UTC"), + timestamp: DateTime.to_iso8601(dt) + } + end + + @spec get_received_at(map()) :: DateTime.t() | NaiveDateTime.t() | String.t() | integer() | nil defp get_received_at(%{received_at: value}), do: value defp get_received_at(%{"received_at" => value}), do: value defp get_received_at(_), do: nil diff --git a/lib/aprsme_web/live/map_live/data_builder.ex b/lib/aprsme_web/live/map_live/data_builder.ex index 7889cf7..b702b5d 100644 --- a/lib/aprsme_web/live/map_live/data_builder.ex +++ b/lib/aprsme_web/live/map_live/data_builder.ex @@ -592,7 +592,18 @@ defmodule AprsmeWeb.MapLive.DataBuilder do defp convert_tuple_entry({k, v}), do: {k, convert_tuples_to_strings(v)} - @spec get_weather_field(map(), atom()) :: String.t() + @typep weather_field :: + :humidity + | :pressure + | :rain_1h + | :rain_24h + | :rain_since_midnight + | :temperature + | :wind_direction + | :wind_gust + | :wind_speed + + @spec get_weather_field(map(), weather_field()) :: any() defp get_weather_field(packet, key) do case SharedPacketUtils.get_weather_field(packet, key) do nil -> "N/A" @@ -637,41 +648,33 @@ defmodule AprsmeWeb.MapLive.DataBuilder do end # Weather unit conversion helpers - defp convert_temperature(value, locale) when is_binary(value) and value != "N/A" do - case Float.parse(value) do - {num_value, _} -> - AprsmeWeb.WeatherUnits.format_temperature(num_value, locale) + @typep unit_formatter :: (number(), String.t() -> {number() | String.t(), String.t()}) + @typep unit_result :: {number() | String.t(), String.t()} - :error -> - {value, "°F"} + @spec convert_temperature(any(), String.t()) :: unit_result() + defp convert_temperature(value, locale), + do: convert_unit(value, locale, &AprsmeWeb.WeatherUnits.format_temperature/2, "°F") + + @spec convert_wind_speed(any(), String.t()) :: unit_result() + defp convert_wind_speed(value, locale), + do: convert_unit(value, locale, &AprsmeWeb.WeatherUnits.format_wind_speed/2, "mph") + + @spec convert_rain(any(), String.t()) :: unit_result() + defp convert_rain(value, locale), do: convert_unit(value, locale, &AprsmeWeb.WeatherUnits.format_rain/2, "in") + + @spec convert_unit(any(), String.t(), unit_formatter(), String.t()) :: unit_result() + defp convert_unit("N/A", _locale, _formatter, default_unit), do: {"N/A", default_unit} + + defp convert_unit(value, locale, formatter, _default_unit) when is_number(value), do: formatter.(value, locale) + + defp convert_unit(value, locale, formatter, default_unit) when is_binary(value) do + case Float.parse(value) do + {num_value, _} -> formatter.(num_value, locale) + :error -> {value, default_unit} end end - defp convert_temperature(value, _locale), do: {value, "°F"} - - defp convert_wind_speed(value, locale) when is_binary(value) and value != "N/A" do - case Float.parse(value) do - {num_value, _} -> - AprsmeWeb.WeatherUnits.format_wind_speed(num_value, locale) - - :error -> - {value, "mph"} - end - end - - defp convert_wind_speed(value, _locale), do: {value, "mph"} - - defp convert_rain(value, locale) when is_binary(value) and value != "N/A" do - case Float.parse(value) do - {num_value, _} -> - AprsmeWeb.WeatherUnits.format_rain(num_value, locale) - - :error -> - {value, "in"} - end - end - - defp convert_rain(value, _locale), do: {value, "in"} + defp convert_unit(value, _locale, _formatter, default_unit), do: {value, default_unit} defp filter_historical_by_distance(historical, most_recent_lat, most_recent_lon) do if most_recent_lat && most_recent_lon do diff --git a/lib/aprsme_web/live/shared/coordinate_utils.ex b/lib/aprsme_web/live/shared/coordinate_utils.ex index 1b329fc..40a4329 100644 --- a/lib/aprsme_web/live/shared/coordinate_utils.ex +++ b/lib/aprsme_web/live/shared/coordinate_utils.ex @@ -5,6 +5,7 @@ defmodule AprsmeWeb.Live.Shared.CoordinateUtils do """ alias Aprs.Types.MicE + alias Aprsme.GeoUtils alias AprsmeWeb.Live.Shared.ParamUtils @doc """ @@ -121,30 +122,7 @@ defmodule AprsmeWeb.Live.Shared.CoordinateUtils do Calculate distance between two lat/lon points in meters using Haversine formula. """ @spec calculate_distance_meters(number(), number(), number(), number()) :: float() - def calculate_distance_meters(lat1, lon1, lat2, lon2) do - # Convert latitude and longitude from degrees to radians - lat1_rad = lat1 * :math.pi() / 180 - lon1_rad = lon1 * :math.pi() / 180 - lat2_rad = lat2 * :math.pi() / 180 - lon2_rad = lon2 * :math.pi() / 180 - - # Haversine formula - dlat = lat2_rad - lat1_rad - dlon = lon2_rad - lon1_rad - - a = - :math.sin(dlat / 2) * :math.sin(dlat / 2) + - :math.cos(lat1_rad) * :math.cos(lat2_rad) * - :math.sin(dlon / 2) * :math.sin(dlon / 2) - - c = 2 * :math.atan2(:math.sqrt(a), :math.sqrt(1 - a)) - - # Earth's radius in meters - earth_radius_meters = 6_371_000 - - # Distance in meters - earth_radius_meters * c - end + defdelegate calculate_distance_meters(lat1, lon1, lat2, lon2), to: GeoUtils, as: :haversine_distance # Private helper functions