docs: add codebase optimization & refactoring review notes
Captures a high-level review of module decomposition, context-layer boundaries, query patterns, error handling, and test coverage gaps to inform future refactor work.
This commit is contained in:
parent
434386816e
commit
a356495b6a
1 changed files with 93 additions and 0 deletions
93
results.md
Normal file
93
results.md
Normal file
|
|
@ -0,0 +1,93 @@
|
||||||
|
# Codebase Optimization & Refactoring Review
|
||||||
|
|
||||||
|
## 1. Module Decomposition
|
||||||
|
|
||||||
|
Several context modules have grown beyond manageable sizes, making them difficult to navigate and maintain.
|
||||||
|
|
||||||
|
* **`Towerops.Snmp` (~3000 lines)**: This module is acting as a "God Context" for all things SNMP.
|
||||||
|
* **Recommendation**: Split into sub-contexts like `Towerops.Snmp.Discovery`, `Towerops.Snmp.Queries`, and `Towerops.Snmp.Monitoring`.
|
||||||
|
* **`Towerops.Accounts` (~1900 lines)**: Handles everything from user registration to TOTP device management.
|
||||||
|
* **Recommendation**: Move TOTP logic to `Towerops.Accounts.TOTP` and session management to `Towerops.Accounts.Sessions`.
|
||||||
|
* **`Towerops.Topology` (~1600 lines)**: Contains complex graph inference logic alongside standard CRUD.
|
||||||
|
* **Recommendation**: Extract the inference engine logic into `Towerops.Topology.InferenceEngine`.
|
||||||
|
|
||||||
|
## 2. DRY Refactoring Opportunities
|
||||||
|
|
||||||
|
### SNMP Check Creation
|
||||||
|
In `Towerops.Snmp.create_checks_from_discovery`, there is a repetitive pattern for creating checks for sensors, interfaces, processors, and storage.
|
||||||
|
|
||||||
|
```elixir
|
||||||
|
# Current Pattern
|
||||||
|
results = Enum.reduce(snmp_device.sensors, results, fn sensor, acc ->
|
||||||
|
case create_sensor_check(device, sensor) do
|
||||||
|
{:ok, _check} -> Map.update!(acc, :sensors, &(&1 + 1))
|
||||||
|
{:error, reason} -> Map.update!(acc, :errors, &[{:sensor, sensor.id, reason} | &1])
|
||||||
|
end
|
||||||
|
end)
|
||||||
|
```
|
||||||
|
|
||||||
|
**Recommendation**: Create a generic `create_discovered_checks(device, entities, type, check_fun)` helper to reduce boilerplate.
|
||||||
|
|
||||||
|
### Topology Evidence Collection
|
||||||
|
`collect_lldp_evidence`, `collect_cdp_evidence`, `collect_mac_evidence`, and `collect_arp_evidence` all perform similar transformations.
|
||||||
|
|
||||||
|
**Recommendation**: Standardize the evidence collection pipeline. Use a common internal function that takes a query and a transformation mapper.
|
||||||
|
|
||||||
|
### Ecto Query Repetition
|
||||||
|
Many modules repeat basic query logic like `where(organization_id: ^org_id)`.
|
||||||
|
|
||||||
|
**Recommendation**: Implement "Query" modules for each schema (e.g., `Towerops.Devices.DeviceQuery`) containing reusable query fragments that can be composed.
|
||||||
|
|
||||||
|
## 3. Functional Pattern Matching vs. Conditionals
|
||||||
|
|
||||||
|
### Replace `cond` with Pattern Matching
|
||||||
|
In `Towerops.Topology.infer_role_from_capabilities`, a `cond` block is used to match strings in a list.
|
||||||
|
|
||||||
|
**Recommendation**: Use multi-clause functions or a more declarative approach.
|
||||||
|
```elixir
|
||||||
|
defp role_from_cap(["router" | _]), do: :router
|
||||||
|
defp role_from_cap(["bridge", "wlan-ap" | _]), do: :wireless
|
||||||
|
# ...
|
||||||
|
```
|
||||||
|
|
||||||
|
### Avoid `if` for Optional Query Filters
|
||||||
|
In `Towerops.Snmp.get_sensor_readings`, an `if` statement is used to conditionally add a `where` clause.
|
||||||
|
|
||||||
|
**Recommendation**: Use a pipe-friendly helper:
|
||||||
|
```elixir
|
||||||
|
defp maybe_filter_since(query, nil), do: query
|
||||||
|
defp maybe_filter_since(query, since), do: where(query, [r], r.checked_at >= ^since)
|
||||||
|
|
||||||
|
# Usage
|
||||||
|
query
|
||||||
|
|> maybe_filter_since(since)
|
||||||
|
|> Repo.all()
|
||||||
|
```
|
||||||
|
|
||||||
|
## 4. Performance Optimizations
|
||||||
|
|
||||||
|
### Batch Database Operations
|
||||||
|
In `Towerops.Topology.upsert_link`, evidence records are inserted one-by-one in an `Enum.each` loop.
|
||||||
|
|
||||||
|
**Recommendation**: Use `Repo.insert_all` to insert all evidence records in a single round-trip to the database.
|
||||||
|
|
||||||
|
### Preloading Strategy
|
||||||
|
Some functions perform manual preloads inside loops or deep in the call stack.
|
||||||
|
|
||||||
|
**Recommendation**: Ensure associations are preloaded at the edge of the context (the public API) to avoid N+1 queries and redundant database calls.
|
||||||
|
|
||||||
|
## 5. Human-Centric Refactoring
|
||||||
|
|
||||||
|
### Break Down Large Functions
|
||||||
|
Functions like `Towerops.Topology.build_device_lookup` are doing too much in one block.
|
||||||
|
|
||||||
|
**Recommendation**: Break these into smaller, named stages:
|
||||||
|
1. `fetch_lookup_devices(org_id)`
|
||||||
|
2. `map_ips_to_ids(devices)`
|
||||||
|
3. `map_names_to_ids(devices)`
|
||||||
|
4. `map_macs_to_ids(devices)`
|
||||||
|
|
||||||
|
### Consistent Result Handling
|
||||||
|
While `Towerops.Result` exists, it isn't used everywhere.
|
||||||
|
|
||||||
|
**Recommendation**: Standardize on `{:ok, value} | {:error, reason}` across all context boundaries and use `Towerops.Result` helpers to compose operations.
|
||||||
Loading…
Add table
Reference in a new issue