# 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:

```elixir
# 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):

```elixir
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:

```elixir
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:

```elixir
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:

```elixir
{: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:

```elixir
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)

```elixir
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:

```elixir
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)

```elixir
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:

```elixir
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`

```elixir
# 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:

```elixir
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:

```elixir
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:

```elixir
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`:

```elixir
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:

```elixir
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:

```elixir
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)

```elixir
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`:

```elixir
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)

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

This is better expressed as:

```elixir
use_ssl: device.mikrotik_use_ssl != false,
```

or with a pattern-matching helper:

```elixir
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:

```elixir
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)

```elixir
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:

```elixir
@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)

```elixir
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:

```elixir
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:

```elixir
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)