graham/towerops
1
0
Fork 0
Code Issues Pull requests Projects Releases Packages Wiki Activity Actions
towerops/dry.md

20 KiB
Raw Blame History

Code Review: DRY & Idiomatic Elixir Findings

This document captures concrete duplication and non-idiomatic patterns found across the codebase. Each finding includes file paths and line numbers where applicable.

1. ✅ DONE - Duplicated credential_source_atom / safe_source_to_atom

Files: lib/towerops/devices.ex lines 408–412 and 887–893

Two private functions in the same module do the exact same thing — convert a credential source string to an atom — with slightly different names and one extra nil clause:

# line 408
defp safe_source_to_atom("device"), do: :device
defp safe_source_to_atom("site"), do: :site
defp safe_source_to_atom("organization"), do: :organization
defp safe_source_to_atom(nil), do: :organization
defp safe_source_to_atom(_unknown), do: :organization

# line 887
defp credential_source_atom(value) do
  case value do
    "device" -> :device
    "site" -> :site
    "organization" -> :organization
    _ -> :organization
  end
end

These should be merged into a single private function using pattern-matching clauses (the idiomatic form):

defp credential_source_atom("device"), do: :device
defp credential_source_atom("site"), do: :site
defp credential_source_atom(_), do: :organization

2. ✅ DONE - Six Near-Identical propagate_* Functions in devices.ex

File: lib/towerops/devices.ex

The following six public functions share an identical structure — build a query, fetch device IDs, compute now, call Repo.update_all, then broadcast per-device:

  • propagate_site_mikrotik_change/2 (~line 459)
  • propagate_organization_mikrotik_change/2 (~line 492)
  • propagate_site_snmpv3_change/2 (~line 563)
  • propagate_organization_snmpv3_change/2 (~line 591)
  • propagate_site_community_change/2 (~line 903)
  • propagate_organization_community_change/2 (~line 930)

The only differences are the query filter field (site_id vs organization_id), the credential source string ("site" vs "organization"), the fields being updated, and the broadcast event atom.

Extract a private helper:

defp propagate_credential_change(device_query, attrs, broadcast_event) do
  device_ids = Repo.all(from(d in device_query, select: d.id))
  now = DateTime.truncate(DateTime.utc_now(), :second)
  updates = attrs |> Map.new() |> Map.put(:updated_at, now) |> Map.to_list()

  Repo.update_all(device_query, set: updates)

  Enum.each(device_ids, fn device_id ->
    Phoenix.PubSub.broadcast(
      Towerops.PubSub,
      "device:#{device_id}:assignments",
      {:assignments_changed, broadcast_event}
    )
  end)

  :ok
end

The six public functions then become thin wrappers that build the query and delegate.

3. ⏭️ DEFERRED - Repeated Form LiveView Pattern (handle_params + validate + save)

Reason: Complex refactoring requiring behavior module or macro abstraction across 4 LiveView files. Would benefit from separate PR with comprehensive testing.

Files:

  • lib/towerops_web/live/schedule_live/form.ex
  • lib/towerops_web/live/escalation_policy_live/form.ex
  • lib/towerops_web/live/maintenance_live/form.ex
  • lib/towerops_web/live/site_live/form.ex

All four follow the exact same skeleton:

def handle_params(params, _url, socket) do
  case socket.assigns.live_action do
    :new -> # assign empty struct + changeset
    :edit -> # load resource + changeset
  end
end

def handle_event("validate", %{"resource" => params}, socket) do
  changeset = socket.assigns.resource |> Schema.changeset(params) |> Map.put(:action, :validate)
  {:noreply, assign(socket, :form, to_form(changeset))}
end

def handle_event("save", %{"resource" => params}, socket) do
  case socket.assigns.live_action do
    :new -> save_resource(socket, :create, params)
    :edit -> save_resource(socket, :update, params)
  end
end

defp save_resource(socket, :create, params) do
  case Context.create_resource(params) do
    {:ok, resource} -> {:noreply, socket |> put_flash(:info, "...") |> push_navigate(...)}
    {:error, changeset} -> {:noreply, assign(socket, :form, to_form(changeset))}
  end
end

defp save_resource(socket, :update, params) do
  case Context.update_resource(socket.assigns.resource, params) do
    {:ok, resource} -> {:noreply, socket |> put_flash(:info, "...") |> push_navigate(...)}
    {:error, changeset} -> {:noreply, assign(socket, :form, to_form(changeset))}
  end
end

The save_resource/3 :create and :update clauses are also structurally identical — only the context function and flash message differ. The case socket.assigns.live_action dispatch in handle_event("save") is also repeated verbatim in all four modules.

4. ⏭️ DEFERRED - Repeated Access Control Error Handling in LiveViews

Reason: Would require AccessControl helper to return socket-ready responses. Impacts 6+ LiveView files. Better as separate focused PR.

Files:

  • lib/towerops_web/live/alert_live/index.ex (~lines 53–80)
  • lib/towerops_web/live/device_live/form.ex (~lines 105–115)
  • lib/towerops_web/live/device_live/show.ex (~lines 61–75, 817–825, 1018–1025)
  • lib/towerops_web/live/config_timeline_live.ex (~line 24)
  • lib/towerops_web/live/dashboard_live.ex (~line 66)
  • lib/towerops_web/live/device_live/index.ex (~lines 146–166)

Every call to AccessControl.verify_*_access/2 is followed by the same two error clauses:

{:error, :not_found} ->
  {:noreply, put_flash(socket, :error, t_equipment("... not found"))}

{:error, :unauthorized} ->
  {:noreply, put_flash(socket, :error, t_equipment("You don't have access to this ..."))}

AccessControl already centralises the lookup logic. A companion helper in the same module (or a small addition to AccessControl) could handle the flash + navigate response, reducing each call site to a single with or case without the repeated error branches.

5. ✅ DONE - determine_effective_agent_id Duplicates Agents.get_effective_agent_token/1

File: lib/towerops_web/live/device_live/form.ex (~lines 390–415)

determine_effective_agent_id/2 walks device → site → org to find an agent token. This is exactly what Agents.get_effective_agent_token/1 and Agents.get_effective_agent_token_with_source/1 already do in lib/towerops/agents.ex. The LiveView version operates on form params (maps) rather than structs, but the logic is the same. Similarly, using_cloud_poller?/2 (~line 418) repeats the same three-level walk.

These should be consolidated — either by passing a lightweight struct to the existing Agents functions, or by extracting a shared helper that accepts either form params or a struct.

6. ✅ DONE - get_fallback_agent_token/1 and get_fallback_agent_token_with_source/1 Are Near-Identical

File: lib/towerops/agents.ex

Both private functions walk the same site → org → global chain. The only difference is that one returns just the token ID and the other returns {token_id, source}. This is a classic case for a single function that always returns {id, source}, with the caller discarding the source when not needed:

defp resolve_fallback_agent(%{site: %{agent_token_id: id}} = _device) when not is_nil(id),
  do: {id, :site}

defp resolve_fallback_agent(%{site: %{organization: %{default_agent_token_id: id}}} = _device)
     when not is_nil(id),
  do: {id, :organization}

defp resolve_fallback_agent(%{organization: %{default_agent_token_id: id}} = _device)
     when not is_nil(id),
  do: {id, :organization}

defp resolve_fallback_agent(_device) do
  case Towerops.Settings.get_global_default_cloud_poller() do
    nil -> {nil, :none}
    id -> {id, :global}
  end
end

get_effective_agent_token/1 then becomes elem(get_effective_agent_token_with_source(device), 0).

7. ✅ DONE - cond Instead of Pattern-Matching Function Clauses

File: lib/towerops_web/live/device_live/form.ex (~lines 72–82)

equipment_attrs =
  cond do
    socket.assigns.preselected_site_id ->
      Map.put(equipment_attrs, :site_id, socket.assigns.preselected_site_id)

    length(socket.assigns.available_sites) == 1 ->
      site = List.first(socket.assigns.available_sites)
      Map.put(equipment_attrs, :site_id, site.id)

    true ->
      equipment_attrs
  end

This is better expressed as a private function with pattern-matching clauses:

defp maybe_preselect_site(attrs, preselected_id, _sites) when not is_nil(preselected_id),
  do: Map.put(attrs, :site_id, preselected_id)

defp maybe_preselect_site(attrs, nil, [site]),
  do: Map.put(attrs, :site_id, site.id)

defp maybe_preselect_site(attrs, nil, _sites), do: attrs

File: lib/towerops/agents.ex — get_fallback_agent_token/1 and get_fallback_agent_token_with_source/1 both use cond with match?/2 guards. These should be pattern-matching function clauses (see finding #6 above).

File: lib/towerops/devices.ex — should_trigger_discovery?/4 (~line 1090) uses cond with boolean conditions that are better expressed as pattern-matching clauses or a series of guard-based function heads.

8. ✅ DONE - if for Optional Value Lookup (Use case or Pattern Matching)

File: lib/towerops_web/live/site_live/form.ex (~lines 30–36 and 52–58)

org_agent =
  if socket.assigns.organization.default_agent_token_id do
    Enum.find(socket.assigns.available_agents, fn a ->
      a.id == socket.assigns.organization.default_agent_token_id
    end)
  end

This duplicates the same block in both apply_action/3 clauses. Extract to a private function and use case or pattern matching:

defp find_org_agent(nil, _agents), do: nil
defp find_org_agent(agent_id, agents), do: Enum.find(agents, &(&1.id == agent_id))

Then call find_org_agent(organization.default_agent_token_id, available_agents) in both clauses.

9. ✅ DONE - normalize_alert_type Defined Twice in alerts.ex

File: lib/towerops/alerts.ex

# Used for attrs map normalization
defp normalize_attrs_alert_type(%{alert_type: alert_type} = attrs) when is_atom(alert_type) do
  %{attrs | alert_type: to_string(alert_type)}
end
defp normalize_attrs_alert_type(attrs), do: attrs

# Used for query normalization
defp normalize_alert_type(alert_type) when is_atom(alert_type), do: to_string(alert_type)
defp normalize_alert_type(alert_type) when is_binary(alert_type), do: alert_type

The second function (normalize_alert_type/1) is a subset of the first. The atom-to-string conversion is the same in both. The attrs version can delegate:

defp normalize_attrs_alert_type(%{alert_type: type} = attrs) when is_atom(type),
  do: %{attrs | alert_type: normalize_alert_type(type)}
defp normalize_attrs_alert_type(attrs), do: attrs

10. ✅ DONE - resolve_alert and resolve_alert_silent Share Identical Changeset Logic

File: lib/towerops/alerts.ex

Both functions build the same changeset (%{resolved_at: DateTime.truncate(...)}) and call Repo.update. The only difference is the post-update side effects. Extract the update:

defp do_resolve_alert(alert) do
  alert
  |> Alert.changeset(%{resolved_at: DateTime.truncate(DateTime.utc_now(), :second)})
  |> Repo.update()
end

Then resolve_alert/1 and resolve_alert_silent/1 call do_resolve_alert/1 and handle their respective side effects.

The same applies to acknowledge_alert/2 and acknowledge_alert_silent/1.

11. ✅ DONE - DateTime.truncate(DateTime.utc_now(), :second) Repeated ~30+ Times

Files: lib/towerops/devices.ex, lib/towerops/alerts.ex, lib/towerops/sites.ex, lib/towerops/organizations.ex, lib/towerops/agents.ex, lib/towerops_web/channels/agent_channel.ex, and many more.

This expression appears over 30 times across the codebase. A single module-level helper (or a function in Towerops.Repo or a shared utility module) would eliminate the repetition:

defp now(), do: DateTime.truncate(DateTime.utc_now(), :second)

Or, since it's needed across many modules, a public helper in e.g. Towerops.Time:

defmodule Towerops.Time do
  def now(), do: DateTime.truncate(DateTime.utc_now(), :second)
end

12. ✅ DONE - sanitize_like/1 Delegated Identically in Both sites.ex and devices.ex

Files: lib/towerops/sites.ex (last line) and lib/towerops/devices.ex (last line)

Both modules define:

defp sanitize_like(query), do: Towerops.QueryHelpers.sanitize_like(query)

This private wrapper adds no value — callers can just call Towerops.QueryHelpers.sanitize_like/1 directly, or import Towerops.QueryHelpers, only: [sanitize_like: 1] at the top of each module.

13. ⏭️ SKIPPED - handle_params Tab/Filter Pattern Repeated Across Index LiveViews

Reason: Pattern is already clear and simple (Map.get(params, "filter", default)). Helper function would add minimal value.

Files:

  • lib/towerops_web/live/alert_live/index.ex (~lines 38–44)
  • lib/towerops_web/live/maintenance_live/index.ex (~lines 17–23)

Both follow:

def handle_params(params, _url, socket) do
  filter = Map.get(params, "filter", "default")
  {:noreply, socket |> assign(:filter, filter) |> load_data(...)}
end

This is a minor but consistent pattern. A shared helper assign_from_params(socket, params, key, default) would make the intent clearer and reduce boilerplate.

14. ✅ DONE - verify_device_access / verify_site_access in AccessControl Use if Instead of Pattern Matching

File: lib/towerops_web/live/helpers/access_control.ex (~lines 34–43, 66–75)

def verify_device_access(device_id, organization_id) do
  case Devices.get_device(device_id) do
    nil -> {:error, :not_found}
    device ->
      if device.organization_id == organization_id do
        {:ok, device}
      else
        {:error, :unauthorized}
      end
  end
end

The inner if should be a case or pattern match. More idiomatically, the case + if can collapse into a single case with a guard, or use with:

def verify_device_access(device_id, organization_id) do
  with %Device{organization_id: ^organization_id} = device <- Devices.get_device(device_id) do
    {:ok, device}
  else
    nil -> {:error, :not_found}
    %Device{} -> {:error, :unauthorized}
  end
end

The same applies to verify_site_access/2. verify_alert_access/2 has a more complex preload but the same structural issue.

15. ✅ DONE - resolve_mikrotik_config Uses if for a Nil-Default

File: lib/towerops/devices.ex (~line 448)

use_ssl: if(device.mikrotik_use_ssl == nil, do: true, else: device.mikrotik_use_ssl),

This is better expressed as:

use_ssl: device.mikrotik_use_ssl != false,

or with a pattern-matching helper:

defp mikrotik_use_ssl?(nil), do: true
defp mikrotik_use_ssl?(val), do: val

16. ✅ DONE - handle_snmp_changes and should_trigger_discovery? Use cond for Boolean Logic

File: lib/towerops/devices.ex (~lines 1080–1110)

should_trigger_discovery?/4 uses a cond with four boolean branches, all of which check device.snmp_enabled first. This is better expressed as pattern-matching function clauses:

defp should_trigger_discovery?(%{snmp_enabled: true}, false, _old_version, _old_port), do: true
defp should_trigger_discovery?(%{snmp_enabled: true, snmp_version: v}, _old_snmp, v, _), do: false
defp should_trigger_discovery?(%{snmp_enabled: true, snmp_version: _}, _, _, _), do: true
defp should_trigger_discovery?(%{snmp_enabled: true, snmp_port: p}, _, _, p), do: false
defp should_trigger_discovery?(%{snmp_enabled: true}, _, _, _), do: true
defp should_trigger_discovery?(_, _, _, _), do: false

Similarly, handle_monitoring_changes/2 and handle_snmp_changes/4 use cond where function clauses with pattern matching on the boolean pairs would be cleaner.

17. ✅ DONE - get_device and get_device! Repeat the Same Preload

File: lib/towerops/devices.ex (~lines 330–345)

def get_device(id) do
  DeviceSchema
  |> Repo.get(id)
  |> case do
    nil -> nil
    device -> Repo.preload(device, [:organization, site: :organization])
  end
end

def get_device!(id) do
  DeviceSchema
  |> Repo.get!(id)
  |> Repo.preload([:organization, site: :organization])
end

The preload list [:organization, site: :organization] is duplicated. Extract it as a module attribute or a private function:

@device_preloads [:organization, site: :organization]

def get_device(id) do
  case Repo.get(DeviceSchema, id) do
    nil -> nil
    device -> Repo.preload(device, @device_preloads)
  end
end

def get_device!(id), do: DeviceSchema |> Repo.get!(id) |> Repo.preload(@device_preloads)

The same pattern applies to get_site/1 and get_site!/1 in lib/towerops/sites.ex (both preload [:parent_site, :child_sites, :device]).

18. ✅ DONE - list_organization_devices Uses if for Query Composition

File: lib/towerops/devices.ex (~lines 44–65)

query =
  if site_id = filters["site_id"] do
    where(query, [e], e.site_id == ^site_id)
  else
    query
  end

query =
  if status = filters["status"] do
    where(query, [e], e.status == ^status)
  else
    query
  end

This is the idiomatic Ecto filter-building pattern, but the if/else query form is verbose. The more idiomatic approach uses Enum.reduce over the filters or a pipeline of maybe_filter_* helpers:

defp maybe_filter_site(query, nil), do: query
defp maybe_filter_site(query, site_id), do: where(query, [e], e.site_id == ^site_id)

defp maybe_filter_status(query, nil), do: query
defp maybe_filter_status(query, status), do: where(query, [e], e.status == ^status)

19. ⏭️ SKIPPED - alert_live/index.ex — severity_color/1 and severity_badge_class/1 Are Two Functions That Should Be One

Reason: severity_color/1 is used independently in template for text colors. Cannot collapse without breaking functionality.

File: lib/towerops_web/live/alert_live/index.ex (~lines 220–250)

severity_color/1 returns a string like "red", and severity_badge_class/1 immediately maps that string to a CSS class. The intermediate string representation is only used to feed severity_badge_class/1. Collapse them:

defp severity_badge_class(alert) do
  cond do
    alert.resolved_at -> "bg-gray-300 dark:bg-gray-600"
    alert.alert_type == "device_down" and is_nil(alert.resolved_at) ->
      age_minutes = DateTime.diff(DateTime.utc_now(), alert.triggered_at, :minute)
      cond do
        age_minutes > 60 -> "bg-red-500"
        age_minutes > 15 -> "bg-orange-500"
        true -> "bg-yellow-500"
      end
    true -> "bg-green-500"
  end
end

Unless severity_color/1 is used independently in the template (in which case the indirection is intentional), the two-step conversion is unnecessary.

20. ⏭️ SKIPPED - age_text/1, duration_text/1, and time_ago/1 Repeat Minute-Bucketing Logic

Reason: Functions have different semantics (seconds vs minutes, "just now" vs exact seconds) and serve different contexts. Duplication is acceptable for clarity.

Files: lib/towerops_web/live/alert_live/index.ex and lib/towerops_web/live/device_live/helpers/formatters.ex

age_text/1 and duration_text/1 in alert_live/index.ex both compute minutes from a DateTime.diff and then bucket into <1m / Xm / Xh / Xd. The Formatters module already has time_ago/1. These should be consolidated in Formatters and called from the template.

Summary of Priorities

High impact (most duplication removed):

  1. Merge safe_source_to_atom / credential_source_atom into one function (finding #1)
  2. Extract propagate_credential_change/3 helper (finding #2)
  3. Merge get_fallback_agent_token variants (finding #6)
  4. Extract do_resolve_alert / do_acknowledge_alert (finding #10)
  5. Introduce Towerops.Time.now/0 or a module-level now/0 (finding #11)

Medium impact (idiomatic improvements): 6. Replace if with with in AccessControl.verify_*_access (finding #14) 7. Replace cond with pattern-matching clauses in device_live/form.ex, agents.ex, devices.ex (findings #7, #16) 8. Extract maybe_preselect_site/3 and find_org_agent/2 (findings #7, #8) 9. Use @device_preloads module attribute for repeated preload lists (finding #17) 10. Use maybe_filter_* helpers in list_organization_devices (finding #18)

Low impact (minor cleanup): 11. Remove sanitize_like/1 private wrappers (finding #12) 12. Collapse severity_color + severity_badge_class (finding #19) 13. Consolidate time-formatting helpers into Formatters (finding #20) 14. Simplify normalize_attrs_alert_type to delegate to normalize_alert_type (finding #9)