From 35bd098b306bedba48653b8aa0729a3bc1b49352 Mon Sep 17 00:00:00 2001 From: Graham McIntire Date: Thu, 23 Apr 2026 14:09:40 -0500 Subject: [PATCH] refactor: more pattern matching + drop unreachable get_field_value clause MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Aprsme.Packet.put_phg_fields: dispatch on the shape of the phg value (map / 4-char binary / anything else) via put_phg_fields_for/2. - Aprsme.Packet.get_field_value: remove the catch-all nil clause. After the put_phg_fields refactor, dialyzer can prove every call site passes a map and flagged the fallback as unreachable — so it's gone, and a future mistyped caller now fails loudly instead of silently. - LogSanitizer.sanitize_map: extract redact_field/2 for readability. - DeviceIdentification.pattern_matches?: match_pattern/3 dispatched on String.contains?/2 keeps the rescue tight around the regex path. - InfoLive.Show: get_received_at pattern-matches on key shape; format_distance splits locale × range via four format_*_distance helpers; calculate_course pipes through normalize_bearing/1. Adds 15 tests exercising the InfoLive.Show helpers (imperial vs. metric formatting, cardinal bearings, haversine symmetry, Decimal inputs). Coverage 67.43 → 67.72%. --- lib/aprsme/device_identification.ex | 20 ++-- lib/aprsme/log_sanitizer.ex | 20 ++-- lib/aprsme/packet.ex | 39 ++++--- lib/aprsme_web/live/info_live/show.ex | 54 ++++------ .../live/info_live/show_helpers_test.exs | 102 ++++++++++++++++++ 5 files changed, 166 insertions(+), 69 deletions(-) create mode 100644 test/aprsme_web/live/info_live/show_helpers_test.exs diff --git a/lib/aprsme/device_identification.ex b/lib/aprsme/device_identification.ex index de16155..6fb6eca 100644 --- a/lib/aprsme/device_identification.ex +++ b/lib/aprsme/device_identification.ex @@ -232,18 +232,18 @@ defmodule Aprsme.DeviceIdentification do end defp pattern_matches?(pattern, identifier) do - if String.contains?(pattern, "?") do - try do - regex = wildcard_pattern_to_regex(pattern) - Regex.match?(regex, identifier) - rescue - _e in Regex.CompileError -> false - end - else - pattern == identifier - end + match_pattern(String.contains?(pattern, "?"), pattern, identifier) end + defp match_pattern(true, pattern, identifier) do + regex = wildcard_pattern_to_regex(pattern) + Regex.match?(regex, identifier) + rescue + _e in Regex.CompileError -> false + end + + defp match_pattern(false, pattern, identifier), do: pattern == identifier + # Converts a pattern with ? wildcards to a regex defp wildcard_pattern_to_regex(pattern) when is_binary(pattern) do # Replace ? with a placeholder, escape all regex metacharacters except the placeholder, diff --git a/lib/aprsme/log_sanitizer.ex b/lib/aprsme/log_sanitizer.ex index e60e15b..fd54612 100644 --- a/lib/aprsme/log_sanitizer.ex +++ b/lib/aprsme/log_sanitizer.ex @@ -32,17 +32,23 @@ defmodule Aprsme.LogSanitizer do Sanitize a map by removing or redacting sensitive fields """ def sanitize_map(data) when is_map(data) do - Enum.reduce(@sensitive_fields, data, fn field, acc -> - cond do - Map.has_key?(acc, field) -> Map.put(acc, field, "[REDACTED]") - Map.has_key?(acc, to_string(field)) -> Map.put(acc, to_string(field), "[REDACTED]") - true -> acc - end - end) + Enum.reduce(@sensitive_fields, data, &redact_field/2) end def sanitize_map(data), do: data + # Redact a sensitive field under either its atom key or its string form, + # whichever actually exists in the map. Missing = untouched. + defp redact_field(field, acc) do + string_key = to_string(field) + + cond do + Map.has_key?(acc, field) -> Map.put(acc, field, "[REDACTED]") + Map.has_key?(acc, string_key) -> Map.put(acc, string_key, "[REDACTED]") + true -> acc + end + end + # Helper to redact a matched sensitive pattern defp redact_match(match) do case String.split(match, ["=", ":"]) do diff --git a/lib/aprsme/packet.ex b/lib/aprsme/packet.ex index bf09f6d..b134ee9 100644 --- a/lib/aprsme/packet.ex +++ b/lib/aprsme/packet.ex @@ -521,25 +521,25 @@ defmodule Aprsme.Packet do end defp put_phg_fields(map, data_extended) do - phg = get_field_value(data_extended, :phg) - - cond do - phg && is_map(phg) -> - map - |> maybe_put(:phg_power, get_field_value(phg, :power)) - |> maybe_put(:phg_height, get_field_value(phg, :height)) - |> maybe_put(:phg_gain, get_field_value(phg, :gain)) - |> maybe_put(:phg_directivity, get_field_value(phg, :directivity)) - - phg && is_binary(phg) && String.length(phg) == 4 -> - # Handle new string format from improved parser (e.g., "1060") - parse_phg_string(map, phg) - - true -> - map - end + put_phg_fields_for(map, get_field_value(data_extended, :phg)) end + # PHG comes in two shapes: a parsed map, or a 4-char string (e.g. "1060"). + # Match on the shape directly instead of `cond`-ing on is_map/is_binary. + defp put_phg_fields_for(map, phg) when is_map(phg) do + map + |> maybe_put(:phg_power, get_field_value(phg, :power)) + |> maybe_put(:phg_height, get_field_value(phg, :height)) + |> maybe_put(:phg_gain, get_field_value(phg, :gain)) + |> maybe_put(:phg_directivity, get_field_value(phg, :directivity)) + end + + defp put_phg_fields_for(map, phg) when is_binary(phg) and byte_size(phg) == 4 do + parse_phg_string(map, phg) + end + + defp put_phg_fields_for(map, _phg), do: map + # Parse PHG string format (e.g., "1060" -> power=1, height=0, gain=6, dir=0) defp parse_phg_string( map, @@ -867,10 +867,9 @@ defmodule Aprsme.Packet do maybe_put(map, :radiorange, get_field_value(data, :radiorange)) end - # Helper to get field value from either atom or string key + # Helper to get field value from either atom or string key. All call sites + # pass a map (dialyzer proves this); no fallback clause needed. defp get_field_value(data, field) when is_map(data) do data[field] || data[to_string(field)] end - - defp get_field_value(_, _), do: nil end diff --git a/lib/aprsme_web/live/info_live/show.ex b/lib/aprsme_web/live/info_live/show.ex index 5aaf436..c37441e 100644 --- a/lib/aprsme_web/live/info_live/show.ex +++ b/lib/aprsme_web/live/info_live/show.ex @@ -305,58 +305,48 @@ defmodule AprsmeWeb.InfoLive.Show do end end - defp get_received_at(packet) do - cond do - Map.has_key?(packet, :received_at) -> packet.received_at - Map.has_key?(packet, "received_at") -> packet["received_at"] - true -> nil - end + defp get_received_at(%{received_at: value}), do: value + defp get_received_at(%{"received_at" => value}), do: value + defp get_received_at(_), do: nil + + # English locale uses imperial (ft/mi), everything else uses metric (m/km). + def format_distance(km, locale \\ "en") + + def format_distance(km, "en") do + miles = Aprsme.Convert.kph_to_mph(km) + format_imperial_distance(miles) end - def format_distance(km, locale \\ "en") do - case locale do - "en" -> - # Use imperial units for English locale - miles = Aprsme.Convert.kph_to_mph(km) + def format_distance(km, _locale), do: format_metric_distance(km) - if miles < 1.0 do - feet = miles * 5280 - "#{Float.round(feet, 0)} ft" - else - "#{Float.round(miles, 2)} mi" - end + defp format_imperial_distance(miles) when miles < 1.0, do: "#{Float.round(miles * 5280, 0)} ft" - _ -> - # Use metric units for other locales - if km < 1.0 do - "#{Float.round(km * 1000, 0)} m" - else - "#{Float.round(km, 2)} km" - end - end - end + defp format_imperial_distance(miles), do: "#{Float.round(miles, 2)} mi" + + defp format_metric_distance(km) when km < 1.0, do: "#{Float.round(km * 1000, 0)} m" + defp format_metric_distance(km), do: "#{Float.round(km, 2)} km" def calculate_course(lat1, lon1, lat2, lon2) do - # Calculate bearing from point 1 to point 2 - # Convert Decimal to float if needed + # Convert Decimal to float if needed. lat1 = to_float(lat1) lon1 = to_float(lon1) lat2 = to_float(lat2) lon2 = to_float(lon2) dlon = :math.pi() / 180 * (lon2 - lon1) - lat1_rad = :math.pi() / 180 * lat1 lat2_rad = :math.pi() / 180 * lat2 y = :math.sin(dlon) * :math.cos(lat2_rad) x = :math.cos(lat1_rad) * :math.sin(lat2_rad) - :math.sin(lat1_rad) * :math.cos(lat2_rad) * :math.cos(dlon) - bearing = :math.atan2(y, x) * 180 / :math.pi() - # Convert to 0-360 range - if bearing < 0, do: bearing + 360, else: bearing + normalize_bearing(:math.atan2(y, x) * 180 / :math.pi()) end + # Convert atan2 output from ±180 to 0-360. + defp normalize_bearing(b) when b < 0, do: b + 360 + defp normalize_bearing(b), do: b + defp get_heard_by_stations(callsign, locale) do alias Aprsme.Repo diff --git a/test/aprsme_web/live/info_live/show_helpers_test.exs b/test/aprsme_web/live/info_live/show_helpers_test.exs new file mode 100644 index 0000000..b3ef37e --- /dev/null +++ b/test/aprsme_web/live/info_live/show_helpers_test.exs @@ -0,0 +1,102 @@ +defmodule AprsmeWeb.InfoLive.ShowHelpersTest do + use ExUnit.Case, async: true + + alias AprsmeWeb.InfoLive.Show + + describe "format_distance/2 with English locale (imperial)" do + test "uses feet for distances under a mile" do + # 0.5 km → ~0.31 miles → ~1641 ft + result = Show.format_distance(0.5, "en") + assert result =~ "ft" + refute result =~ "mi" + end + + test "uses miles for distances >= 1 mile" do + # 10 km → ~6.21 miles + result = Show.format_distance(10.0, "en") + assert result =~ "mi" + refute result =~ "ft" + end + + test "default locale is English" do + assert Show.format_distance(10.0) =~ "mi" + end + end + + describe "format_distance/2 with non-English locale (metric)" do + test "uses meters for distances under 1 km" do + result = Show.format_distance(0.5, "de") + assert result =~ "m" + refute result =~ "km" + end + + test "uses km for longer distances" do + result = Show.format_distance(5.0, "fr") + assert result =~ "km" + end + + test "handles multiple metric locales" do + for loc <- ["es", "de", "fr", "ja"] do + assert Show.format_distance(10.0, loc) =~ "km" + end + end + end + + describe "calculate_course/4" do + test "returns 0 (or 360) for due north" do + # From (0,0) to (1,0) — moving north. + bearing = Show.calculate_course(0.0, 0.0, 1.0, 0.0) + assert bearing == 0.0 or bearing == 360.0 + end + + test "returns ~90 for due east" do + # From (0,0) to (0,1) — moving east. + bearing = Show.calculate_course(0.0, 0.0, 0.0, 1.0) + assert_in_delta bearing, 90.0, 0.5 + end + + test "returns ~180 for due south" do + bearing = Show.calculate_course(1.0, 0.0, 0.0, 0.0) + assert_in_delta bearing, 180.0, 0.5 + end + + test "returns ~270 for due west" do + bearing = Show.calculate_course(0.0, 1.0, 0.0, 0.0) + assert_in_delta bearing, 270.0, 0.5 + end + + test "always returns a value in 0..360" do + for {lat1, lon1, lat2, lon2} <- [ + {0.0, 0.0, 45.0, 45.0}, + {10.0, 20.0, -10.0, -20.0}, + {0.0, 0.0, 0.0, -1.0} + ] do + b = Show.calculate_course(lat1, lon1, lat2, lon2) + assert b >= 0 + assert b <= 360 + end + end + + test "accepts Decimal inputs" do + result = Show.calculate_course(Decimal.new("0.0"), Decimal.new("0.0"), Decimal.new("1.0"), Decimal.new("0.0")) + assert is_float(result) + end + end + + describe "haversine/4" do + test "returns 0 for identical points" do + assert Show.haversine(40.0, -100.0, 40.0, -100.0) == 0.0 + end + + test "is symmetric" do + a = Show.haversine(40.0, -100.0, 41.0, -99.0) + b = Show.haversine(41.0, -99.0, 40.0, -100.0) + assert_in_delta a, b, 0.001 + end + + test "returns positive distance for distinct points" do + d = Show.haversine(40.0, -100.0, 41.0, -99.0) + assert d > 0 + end + end +end