docs: idiomatic Elixir and Phoenix patterns with verified source citations
Extracted patterns from Elixir core and Phoenix source code with specific file:line citations, then verified all citations against the actual source in a second pass. Structure: - patterns/ — Elixir core patterns (GenServer, errors, data, types, etc.) - phoenix/ — Phoenix-specific patterns and deviations - comparison/ — Elixir vs Phoenix side-by-side - smells/ — Anti-patterns and common mistakes - changelog/ — Daily Elixir/Phoenix PR digest (auto-updated)
This commit is contained in:
Aaron Weiker
+279
-134
@@ -1,181 +1,326 @@
|
||||
# Anti-Patterns
|
||||
# Anti-Patterns (Code Smells) in Elixir
|
||||
|
||||
Things the Elixir and Phoenix source deliberately avoids — and why you should too.
|
||||
|
||||
## 1. GenServer for Pure Functions
|
||||
|
||||
**Source:** `lib/elixir/lib/gen_server.ex:533-575` ("When (not) to use a GenServer")
|
||||
|
||||
The GenServer docs explicitly say:
|
||||
|
||||
> If you don't need a process, then you don't need a process.
|
||||
|
||||
**What they avoid:** Creating a GenServer to wrap stateless computation.
|
||||
|
||||
**Why:** A process adds message passing overhead, serialization (one request at a time), and supervision complexity — all unnecessary for pure functions.
|
||||
|
||||
**Do this instead:** A plain module with functions:
|
||||
```elixir
|
||||
# Good: pure function, no process needed
|
||||
defmodule MyApp.Calculator do
|
||||
def add(a, b), do: a + b
|
||||
end
|
||||
|
||||
# Bad: unnecessary process
|
||||
defmodule MyApp.Calculator do
|
||||
use GenServer
|
||||
def add(a, b), do: GenServer.call(__MODULE__, {:add, a, b})
|
||||
def handle_call({:add, a, b}, _from, state), do: {:reply, a + b, state}
|
||||
end
|
||||
```
|
||||
Patterns the Elixir core team actively avoids, extracted from studying what they DON'T do in their source code.
|
||||
|
||||
---
|
||||
|
||||
## 2. Dynamic Atoms for Process Names
|
||||
## 1. Process.sleep for Synchronization (Timing-Based Tests)
|
||||
|
||||
**Source:** `lib/elixir/lib/registry.ex:28-60` (Registry as alternative to atoms)
|
||||
**What they avoid:** Using `Process.sleep(N)` to wait for async operations to complete.
|
||||
|
||||
The Registry module exists specifically because dynamic atom creation is dangerous:
|
||||
**Source evidence:** `lib/elixir/test/elixir/task_test.exs` — 13 uses of `Process.sleep`, ALL are `Process.sleep(:infinity)` for process parking. Zero uses of `Process.sleep(100)` or similar for timing.
|
||||
|
||||
> atoms are never garbage collected
|
||||
|
||||
**What they avoid:** `String.to_atom("worker_#{id}")`
|
||||
**Why it's bad:** Race conditions. A sleep might be too short on a loaded CI server, causing flaky tests. Too long wastes time.
|
||||
|
||||
**What they do instead:**
|
||||
```elixir
|
||||
{:via, Registry, {MyApp.Registry, "worker-#{id}"}}
|
||||
# BAD — timing-based synchronization
|
||||
spawn(fn -> send(parent, :done) end)
|
||||
Process.sleep(50)
|
||||
assert_received :done
|
||||
|
||||
# GOOD — event-based synchronization
|
||||
spawn(fn -> send(parent, :done) end)
|
||||
assert_receive :done # waits up to 100ms with proper timeout
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
## 3. Broad `import` Without `:only`
|
||||
|
||||
**Source:** `lib/elixir/lib/enum.ex:250`
|
||||
|
||||
```elixir
|
||||
import Kernel, except: [max: 2, min: 2]
|
||||
# BAD — waiting for process to die
|
||||
Process.exit(pid, :kill)
|
||||
Process.sleep(10)
|
||||
refute Process.alive?(pid)
|
||||
|
||||
# GOOD — monitor-based confirmation
|
||||
ref = Process.monitor(pid)
|
||||
Process.exit(pid, :kill)
|
||||
assert_receive {:DOWN, ^ref, _, _, _}
|
||||
```
|
||||
|
||||
Even within the standard library, imports are scoped. Enum explicitly excludes the specific Kernel functions it replaces.
|
||||
|
||||
**What they avoid:** `import Kernel` without qualification when they define conflicting names.
|
||||
|
||||
**Exception:** Phoenix Router (`lib/phoenix/router.ex:274-276`) imports broadly — but it's a DSL where usability trumps explicitness.
|
||||
|
||||
---
|
||||
|
||||
## 4. Exceptions for Control Flow
|
||||
## 2. Mutable Global State in Tests
|
||||
|
||||
**Source:** `lib/elixir/lib/task.ex:455-460` (async documentation on error handling)
|
||||
**What they avoid:** Tests that depend on or modify global state without cleanup.
|
||||
|
||||
> an asynchronous task should be thought of as an extension of the
|
||||
> caller process rather than a mechanism to isolate it from all errors.
|
||||
**Source evidence:** `lib/mix/test/test_helper.exs:98-113` — MixTest.Case restores ALL global state in `on_exit`:
|
||||
- `Mix.env(:dev)`, `Mix.target(:host)`, `Mix.Task.clear()`, `Mix.Shell.Process.flush()`
|
||||
- Unloads all applications that were loaded during the test
|
||||
|
||||
The Task documentation advises returning `{:ok, val} | :error` for normal flow, NOT using try/rescue:
|
||||
|
||||
> For example, to either return `{:ok, val} | :error` results or,
|
||||
> in more extreme cases, by using `try/rescue`
|
||||
|
||||
**What they avoid:** Using `try/rescue` around expected failure cases.
|
||||
|
||||
**Why:** Pattern matching on tagged tuples is more explicit, composable (works with `with`), and doesn't hide the error path.
|
||||
|
||||
---
|
||||
|
||||
## 5. Trapping Exits in Normal Code
|
||||
|
||||
**Source:** `lib/elixir/lib/task.ex:469-477` (explicit warning)
|
||||
|
||||
> Setting `:trap_exit` to `true` - trapping exits should be used only in special
|
||||
> circumstances as it would make your process immune to not only exits from the
|
||||
> task but from any other processes.
|
||||
>
|
||||
> Moreover, even when trapping exits, calling `await` will still exit if the
|
||||
> task has terminated without sending its result back.
|
||||
|
||||
**What they avoid:** `Process.flag(:trap_exit, true)` in normal application code.
|
||||
|
||||
**Why:** Trapping exits breaks the supervision contract. A supervisor expects to be able to kill its children — if they trap exits, shutdown semantics change unpredictably.
|
||||
|
||||
---
|
||||
|
||||
## 6. Expensive Work in `init/1`
|
||||
|
||||
**Source:** `lib/elixir/lib/gen_server.ex:127-145` (handle_continue pattern)
|
||||
**Why it's bad:** Tests pass in isolation but fail when run together. Order-dependent failures are the hardest bugs to debug.
|
||||
|
||||
**What they do instead:**
|
||||
```elixir
|
||||
# What NOT to do — blocks the supervisor
|
||||
def init(url) do
|
||||
data = HTTP.get!(url) # BAD: blocks here
|
||||
{:ok, data}
|
||||
end
|
||||
# Every test using Mix gets automatic cleanup
|
||||
setup do
|
||||
on_exit(fn ->
|
||||
Mix.env(:dev)
|
||||
Mix.target(:host)
|
||||
Mix.Task.clear()
|
||||
Mix.Shell.Process.flush()
|
||||
Mix.State.clear_cache()
|
||||
Mix.ProjectStack.clear_stack()
|
||||
|
||||
# What TO do — return immediately, do work later
|
||||
def init(url) do
|
||||
{:ok, :unset, {:continue, {:fetch, url}}}
|
||||
end
|
||||
|
||||
def handle_continue({:fetch, url}, _state) do
|
||||
{:noreply, HTTP.get!(url)}
|
||||
for {app, _, _} <- Application.loaded_applications(), app not in @apps do
|
||||
Application.stop(app)
|
||||
Application.unload(app)
|
||||
end
|
||||
end)
|
||||
end
|
||||
```
|
||||
|
||||
**What they avoid:** Network calls, disk I/O, or any slow operation in `init/1`.
|
||||
|
||||
**Why:** `init/1` blocks `start_link`, which blocks the supervisor. If your init takes 5 seconds, the entire supervision tree startup stalls.
|
||||
|
||||
---
|
||||
|
||||
## 7. Unlinking Task Processes
|
||||
## 3. try/rescue for Control Flow
|
||||
|
||||
**Source:** `lib/elixir/lib/task.ex:478-482`
|
||||
**What they avoid:** Using exceptions for expected conditions.
|
||||
|
||||
> Unlinking the task process started with `async`/`await`. If you unlink the
|
||||
> processes and the task does not belong to any supervisor, you may leave
|
||||
> dangling tasks in case the caller process dies.
|
||||
**Source evidence:** `lib/elixir/lib/enum.ex` — 5,242 lines with only 2 `try do` blocks. `lib/elixir/lib/kernel.ex` — 7,102 lines with only 5 `try do` blocks. The ratio is vanishingly small.
|
||||
|
||||
**What they avoid:** `Process.unlink/1` on task processes.
|
||||
|
||||
**Why:** The link is a safety mechanism. If the caller dies, the task should die too (since nobody will read the result). Unlinking creates orphan processes.
|
||||
|
||||
---
|
||||
|
||||
## 8. Blocking the Agent with Expensive Computation
|
||||
|
||||
**Source:** `lib/elixir/lib/agent.ex:62-82` (client vs server computation)
|
||||
**Why it's bad:** Exceptions are for exceptional conditions. Using them for control flow obscures intent, defeats pattern matching, and is slower.
|
||||
|
||||
**What they do instead:**
|
||||
```elixir
|
||||
# BAD: blocks the agent, other callers queue up
|
||||
def get_something(agent) do
|
||||
Agent.get(agent, fn state -> do_something_expensive(state) end)
|
||||
# BAD — exception as control flow
|
||||
def find_user(id) do
|
||||
try do
|
||||
fetch_user!(id)
|
||||
rescue
|
||||
NotFoundError -> nil
|
||||
end
|
||||
end
|
||||
|
||||
# GOOD: copies state to caller, work happens in caller's process
|
||||
def get_something(agent) do
|
||||
Agent.get(agent, & &1) |> do_something_expensive()
|
||||
# GOOD — tagged tuples for expected outcomes
|
||||
def find_user(id) do
|
||||
case fetch_user(id) do
|
||||
{:ok, user} -> user
|
||||
{:error, :not_found} -> nil
|
||||
end
|
||||
end
|
||||
```
|
||||
|
||||
**What they avoid:** Running expensive operations inside the Agent's process.
|
||||
|
||||
**Why:** The Agent is a single process. While it's computing, ALL other get/update/cast operations queue up. Move computation to the caller unless atomicity is required.
|
||||
The standard library uses `with` statements and tagged tuples (`{:ok, result}` / `{:error, reason}`) for all fallible operations.
|
||||
|
||||
---
|
||||
|
||||
## 9. Raw `spawn` Instead of Supervised Processes
|
||||
## 4. God Modules (Unbounded Module Growth)
|
||||
|
||||
**Source:** `lib/elixir/lib/task.ex:24-26` (why Task over spawn)
|
||||
**What they avoid:** Stuffing everything into one module.
|
||||
|
||||
> Compared to plain processes, started with `spawn/1`, tasks include monitoring
|
||||
> metadata and logging in case of errors.
|
||||
**Source evidence:** The Elixir standard library splits functionality aggressively:
|
||||
- `ExUnit.Case` — test case registration
|
||||
- `ExUnit.Callbacks` — setup/teardown lifecycle
|
||||
- `ExUnit.Assertions` — assertion macros
|
||||
- `ExUnit.CaptureIO` — IO capture
|
||||
- `ExUnit.CaptureLog` — log capture
|
||||
- `ExUnit.DocTest` — doctest extraction
|
||||
|
||||
**Source:** `lib/elixir/lib/task.ex:100-115`
|
||||
Each module has a single, clear responsibility.
|
||||
|
||||
> We encourage developers to rely on supervised tasks as much as possible.
|
||||
> Supervised tasks improve the visibility of how many tasks are running
|
||||
> at a given moment and enable a variety of patterns that give you
|
||||
> explicit control on how to handle the results, errors, and timeouts.
|
||||
**Why it's bad:** Large modules are hard to navigate, hard to test in isolation, and create implicit coupling between unrelated features.
|
||||
|
||||
**What they avoid:** `spawn/1` and `spawn_link/1` in application code.
|
||||
**What they do instead:** Single-responsibility modules that compose. `use ExUnit.Case` imports from multiple focused modules.
|
||||
|
||||
**Why:** Unsupervised processes are invisible to the system. They don't appear in observer, don't get logged on crash, and can't be gracefully shut down.
|
||||
---
|
||||
|
||||
## 5. Stringly-Typed APIs
|
||||
|
||||
**What they avoid:** Using strings where atoms, structs, or tagged tuples would be clearer.
|
||||
|
||||
**Source evidence:** Throughout the codebase, atoms and structs are preferred:
|
||||
- `lib/elixir/lib/task.ex` — `defstruct @enforce_keys` (Task is a struct, not a map)
|
||||
- `lib/elixir/lib/uri.ex` — URI is a struct
|
||||
- `lib/elixir/lib/version.ex` — Version is a struct
|
||||
- `lib/elixir/lib/range.ex` — Range is a struct
|
||||
- All ExUnit tags use atoms: `:async`, `:capture_log`, `:tmp_dir`
|
||||
|
||||
**Why it's bad:** Strings lack compile-time checking, are prone to typos, and don't pattern match cleanly.
|
||||
|
||||
**What they do instead:**
|
||||
```elixir
|
||||
# BAD — string keys, no structure
|
||||
%{"type" => "user", "name" => "Alice", "role" => "admin"}
|
||||
|
||||
# GOOD — struct with enforced keys
|
||||
defmodule User do
|
||||
@enforce_keys [:name, :role]
|
||||
defstruct [:name, :role, :email]
|
||||
end
|
||||
%User{name: "Alice", role: :admin}
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
## 6. Bare Maps Where Structs Belong
|
||||
|
||||
**What they avoid:** Using plain maps for data with a known, fixed shape.
|
||||
|
||||
**Source evidence:** 69 `defstruct` declarations in `lib/elixir/lib/` alone. Every domain concept with a stable shape gets a struct: Task, URI, Version, Range, Regex, MapSet, Stream.
|
||||
|
||||
**Why it's bad:**
|
||||
- No compile-time key checking
|
||||
- No default values
|
||||
- No protocol implementations
|
||||
- No `@enforce_keys` for required fields
|
||||
- Pattern matching is fragile (extra keys silently ignored)
|
||||
|
||||
**What they do instead:**
|
||||
```elixir
|
||||
# The Task struct enforces its shape:
|
||||
defmodule Task do
|
||||
@enforce_keys [:ref, :pid, :owner, :mfa]
|
||||
defstruct @enforce_keys
|
||||
end
|
||||
|
||||
# Pattern matching on struct is type-safe:
|
||||
def await(%Task{ref: ref, owner: owner}) when owner == self() do
|
||||
# ...
|
||||
end
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
## 7. Deep Nesting
|
||||
|
||||
**What they avoid:** Deeply nested `case`/`if`/`cond` blocks.
|
||||
|
||||
**Source evidence:** The standard library heavily uses:
|
||||
- Early returns via pattern matching in function heads
|
||||
- `with` statements for sequential fallible operations
|
||||
- Pipeline operator for transformations
|
||||
- Multi-clause functions instead of nested conditionals
|
||||
|
||||
**Why it's bad:** Deep nesting increases cognitive load and makes the "happy path" hard to identify.
|
||||
|
||||
**What they do instead:**
|
||||
```elixir
|
||||
# BAD — nested conditionals
|
||||
def process(input) do
|
||||
case validate(input) do
|
||||
{:ok, valid} ->
|
||||
case transform(valid) do
|
||||
{:ok, result} ->
|
||||
case save(result) do
|
||||
{:ok, saved} -> {:ok, saved}
|
||||
{:error, reason} -> {:error, reason}
|
||||
end
|
||||
{:error, reason} -> {:error, reason}
|
||||
end
|
||||
{:error, reason} -> {:error, reason}
|
||||
end
|
||||
end
|
||||
|
||||
# GOOD — with statement flattens the happy path
|
||||
def process(input) do
|
||||
with {:ok, valid} <- validate(input),
|
||||
{:ok, result} <- transform(valid),
|
||||
{:ok, saved} <- save(result) do
|
||||
{:ok, saved}
|
||||
end
|
||||
end
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
## 8. Shared Mutable State Between Tests
|
||||
|
||||
**What they avoid:** ETS tables, registered names, or application env used across tests without isolation.
|
||||
|
||||
**Source evidence:** `lib/elixir/test/elixir/registry_test.exs:28-31` — Each test gets a uniquely-named Registry:
|
||||
```elixir
|
||||
name = :"#{config.test}_#{partitions}_#{inspect(keys)}"
|
||||
```
|
||||
|
||||
`lib/elixir/test/elixir/gen_server_test.exs:166` — Uses `%{test: name}` for unique process registration.
|
||||
|
||||
**Why it's bad:** Tests that share state can't run concurrently. They're order-dependent and fragile.
|
||||
|
||||
**What they do instead:** Every shared resource gets a unique name derived from `config.test` (the test name atom, guaranteed unique within a module).
|
||||
|
||||
---
|
||||
|
||||
## 9. Testing Internal State Directly
|
||||
|
||||
**What they avoid:** Reaching into process state, private functions, or internal data structures for assertions.
|
||||
|
||||
**Source evidence:** `lib/elixir/test/elixir/gen_server_test.exs` — The Stack GenServer is tested entirely through its public call/cast interface. No `:sys.get_state/1` anywhere in the test.
|
||||
|
||||
**Why it's bad:** Tests become coupled to implementation. Refactoring internals breaks tests even when behavior is unchanged.
|
||||
|
||||
**What they do instead:** Test the behavior (what goes in, what comes out) through the public API.
|
||||
|
||||
---
|
||||
|
||||
## 10. Overly Complex Test Setup
|
||||
|
||||
**What they avoid:** Setup blocks that are longer than the tests themselves.
|
||||
|
||||
**Source evidence:** The Elixir test suite keeps setup minimal:
|
||||
- `lib/elixir/test/elixir/registry_test.exs` — 5-line setup
|
||||
- `lib/elixir/test/elixir/gen_server_test.exs` — no setup at all, each test is self-contained
|
||||
- `lib/elixir/test/elixir/task_test.exs` — helper functions instead of complex setup
|
||||
|
||||
**Why it's bad:** Complex setup obscures what's actually being tested. When a test fails, you have to understand the setup to understand the failure.
|
||||
|
||||
**What they do instead:**
|
||||
- Self-contained tests that set up exactly what they need
|
||||
- Small helper functions for common patterns
|
||||
- `start_supervised!` for process-heavy tests (one line)
|
||||
|
||||
---
|
||||
|
||||
## 11. Unsupervised Processes
|
||||
|
||||
**What they avoid:** Using raw `spawn/1` or `spawn_link/1` in application code.
|
||||
|
||||
**Source evidence:** The standard library provides `Task`, `GenServer`, `Agent`, and `DynamicSupervisor` for every process use case. Raw spawns appear only in test helpers.
|
||||
|
||||
**Why it's bad:** Unsupervised processes are invisible to the system:
|
||||
- Don't appear in Observer
|
||||
- Don't get logged on crash
|
||||
- Can't be gracefully shut down
|
||||
- Create orphan processes on restart
|
||||
|
||||
**What they do instead:**
|
||||
```elixir
|
||||
# BAD — orphan process
|
||||
spawn(fn -> do_work() end)
|
||||
|
||||
# GOOD — supervised, observable, restartable
|
||||
Task.Supervisor.start_child(MyApp.TaskSupervisor, fn -> do_work() end)
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
## 12. Atom Creation from User Input
|
||||
|
||||
**What they avoid:** Converting untrusted strings to atoms.
|
||||
|
||||
**Source evidence:** `lib/elixir/lib/option_parser.ex:855` — Uses `String.to_existing_atom/1` with the `:switches` allowlist pattern. The only `String.to_atom/1` calls in library code are in compiler/macro contexts where the set is bounded.
|
||||
|
||||
**Why it's bad:** Atoms are never garbage collected. User-controlled atom creation is a denial-of-service vector (1,048,576 atom limit by default).
|
||||
|
||||
**What they do instead:**
|
||||
```elixir
|
||||
# BAD — unbounded atom creation
|
||||
key = String.to_atom(user_input)
|
||||
|
||||
# GOOD — bounded, pre-existing atoms only
|
||||
key = String.to_existing_atom(user_input)
|
||||
|
||||
# BETTER — don't convert at all, use string keys
|
||||
config = Map.get(settings, user_input)
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
## 13. Callback-Heavy Abstractions (When Protocols Suffice)
|
||||
|
||||
**What they avoid:** Behaviour callbacks for polymorphism when protocols are more appropriate.
|
||||
|
||||
**Source evidence:** The standard library uses protocols for data-type polymorphism (`Enumerable`, `Collectable`, `Inspect`, `String.Chars`, `List.Chars`) and behaviours only for process contracts (`GenServer`, `Supervisor`, `Application`).
|
||||
|
||||
**Why it's bad:** Behaviours require the implementing module to know about the behaviour at compile time. Protocols allow retroactive implementation for types you don't control.
|
||||
|
||||
**The heuristic:**
|
||||
- **Protocol:** "Different data types need different treatment" (e.g., printing, iterating)
|
||||
- **Behaviour:** "Different modules implement the same process contract" (e.g., GenServer callbacks)
|
||||
|
||||
Reference in New Issue
Block a user