elixir-patterns/smells/anti-patterns.md

# Anti-Patterns (Code Smells) in Elixir

Patterns the Elixir core team actively avoids, extracted from studying what they DON'T do in their source code.

---

## 1. Process.sleep for Synchronization (Timing-Based Tests)

**What they avoid:** Using `Process.sleep(N)` to wait for async operations to complete.

**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.

**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
# 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
```

```elixir
# 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, _, _, _}
```

### When to Apply This Rule

**Triggers:**
- Any `Process.sleep/1` call with a numeric argument in test code
- Flaky tests that pass locally but fail on CI
- Tests with comments like "wait for process to finish"

**Example — the smell:**
```elixir
test "pubsub delivers messages" do
  PubSub.subscribe(:topic)
  PubSub.publish(:topic, "hello")
  Process.sleep(100)
  assert_received {"hello"}
end
```

**Example — fixed:**
```elixir
test "pubsub delivers messages" do
  PubSub.subscribe(:topic)
  PubSub.publish(:topic, "hello")
  assert_receive {"hello"}, 1000
end
```

### Exceptions (When This Rule Doesn't Apply)

**It's OK when:**
- Parking a process indefinitely with `Process.sleep(:infinity)` (it's not timing-based, it's a deliberate block)
- Testing actual time-dependent behavior (e.g., "this rate limiter allows 1 req/sec") where the delay IS the thing under test

**Example of acceptable use:**
```elixir
# Process parking — not synchronization, just "stay alive forever"
spawn(fn ->
  Process.sleep(:infinity)
end)
```

**Why it's OK here:** The process isn't waiting for something to happen — it's deliberately kept alive as a fixture. There's no race condition because nothing depends on timing.

---

## 2. Mutable Global State in Tests

**What they avoid:** Tests that depend on or modify global state without cleanup.

**Source evidence:** `lib/mix/test/test_helper.exs:99-115` — 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

**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
# 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()

    for {app, _, _} <- Application.loaded_applications(), app not in @apps do
      Application.stop(app)
      Application.unload(app)
    end
  end)
end
```

### When to Apply This Rule

**Triggers:**
- Tests that call `Application.put_env/3` or `System.put_env/2`
- Tests that modify Logger level, Mix env, or any module attribute at runtime
- Tests that fail when run in a different order or with `--seed 0`

**Example — the smell:**
```elixir
test "production mode disables debug" do
  Application.put_env(:my_app, :env, :prod)
  assert MyApp.debug_enabled?() == false
  # Next test inherits :prod env!
end
```

**Example — fixed:**
```elixir
test "production mode disables debug" do
  original = Application.get_env(:my_app, :env)
  Application.put_env(:my_app, :env, :prod)
  on_exit(fn -> Application.put_env(:my_app, :env, original) end)

  assert MyApp.debug_enabled?() == false
end
```

### Exceptions (When This Rule Doesn't Apply)

**It's OK when:**
- The global state change happens in `test_helper.exs` before any tests run (one-time setup for the entire suite)
- Using `ExUnit.Case, async: false` with a dedicated setup/teardown and the state is inherently global (e.g., database migrations)

**Example of acceptable use:**
```elixir
# In test_helper.exs — one-time global setup
Application.put_env(:my_app, :env, :test)
ExUnit.start()
```

**Why it's OK here:** This runs once before any tests execute. It's not mutating state between tests — it's establishing the test environment baseline.

---

## 3. try/rescue for Control Flow

**What they avoid:** Using exceptions for expected conditions.

**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.

**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 — exception as control flow
def find_user(id) do
  try do
    fetch_user!(id)
  rescue
    NotFoundError -> nil
  end
end

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

The standard library uses `with` statements and tagged tuples (`{:ok, result}` / `{:error, reason}`) for all fallible operations.

### When to Apply This Rule

**Triggers:**
- `try/rescue` blocks that catch known, expected error types
- Functions that call a bang (`!`) variant and immediately rescue
- Error handling that converts exceptions back to tuples

**Example — the smell:**
```elixir
def parse_config(path) do
  try do
    content = File.read!(path)
    Jason.decode!(content)
  rescue
    File.Error -> {:error, :file_not_found}
    Jason.DecodeError -> {:error, :invalid_json}
  end
end
```

**Example — fixed:**
```elixir
def parse_config(path) do
  with {:ok, content} <- File.read(path),
       {:ok, decoded} <- Jason.decode(content) do
    {:ok, decoded}
  end
end
```

### Exceptions (When This Rule Doesn't Apply)

**It's OK when:**
- Interfacing with libraries that only provide bang functions (no tuple-returning variant)
- Catching truly unexpected errors at a supervision boundary (e.g., a GenServer `handle_call` that must not crash)
- Using `raise`/`rescue` for assertions in tests (`assert_raise`)

**Example of acceptable use:**
```elixir
# Third-party lib only provides a bang function
def safe_parse(input) do
  try do
    {:ok, ThirdPartyLib.parse!(input)}
  rescue
    ArgumentError -> {:error, :invalid_input}
  end
end
```

**Why it's OK here:** You don't control the library API. Wrapping the bang call is the pragmatic choice until the library adds a non-bang variant.

---

## 4. God Modules (Unbounded Module Growth)

**What they avoid:** Stuffing everything into one module.

**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

Each module has a single, clear responsibility.

**Why it's bad:** Large modules are hard to navigate, hard to test in isolation, and create implicit coupling between unrelated features.

**What they do instead:** Single-responsibility modules that compose. `use ExUnit.Case` imports from multiple focused modules.

### When to Apply This Rule

**Triggers:**
- A module exceeds ~300 lines
- You find yourself adding section comments like `# --- User helpers ---`
- Functions in the module don't share data or call each other
- The module name is generic (`Helpers`, `Utils`, `Common`)

**Example — the smell:**
```elixir
defmodule MyApp.Helpers do
  def format_date(date), do: ...
  def send_email(to, subject, body), do: ...
  def validate_phone(number), do: ...
  def geocode_address(addr), do: ...
  def generate_pdf(data), do: ...
end
```

**Example — fixed:**
```elixir
defmodule MyApp.DateFormatter do
  def format(date), do: ...
end

defmodule MyApp.Mailer do
  def send(to, subject, body), do: ...
end

defmodule MyApp.PhoneValidator do
  def validate(number), do: ...
end
```

### Exceptions (When This Rule Doesn't Apply)

**It's OK when:**
- The module is a Facade that delegates to sub-modules (thin wrapper providing a unified API)
- The module is a protocol implementation that must implement all callbacks in one place
- Kernel-style modules that define language primitives (you're not writing Kernel)

**Example of acceptable use:**
```elixir
defmodule MyApp.Orders do
  # Facade — delegates to focused modules
  defdelegate create(params), to: MyApp.Orders.Creator
  defdelegate cancel(order), to: MyApp.Orders.Canceller
  defdelegate refund(order, reason), to: MyApp.Orders.Refunder
end
```

**Why it's OK here:** The module is a thin routing layer. The actual logic lives in focused sub-modules. The facade exists for API convenience, not because responsibilities are lumped together.

---

## 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}
```

### When to Apply This Rule

**Triggers:**
- Maps with string keys used internally (not from external JSON)
- Functions that accept string arguments for mode/type selection (`"asc"`, `"desc"`)
- Pattern matching on string values for dispatch

**Example — the smell:**
```elixir
def sort_users(users, direction) when direction in ["asc", "desc"] do
  case direction do
    "asc" -> Enum.sort(users)
    "desc" -> Enum.sort(users, :desc)
  end
end
```

**Example — fixed:**
```elixir
def sort_users(users, :asc), do: Enum.sort(users)
def sort_users(users, :desc), do: Enum.sort(users, :desc)
```

### Exceptions (When This Rule Doesn't Apply)

**It's OK when:**
- Data comes from external sources (JSON APIs, CSV, database text columns) and hasn't been validated yet
- The set of values is truly unbounded (user names, free-text fields)
- You're building a string-keyed map specifically for JSON serialization output

**Example of acceptable use:**
```elixir
# External API response — strings are correct here
def handle_webhook(%{"event" => event_type, "data" => data}) do
  case event_type do
    "payment.completed" -> process_payment(data)
    "subscription.cancelled" -> cancel_subscription(data)
    _ -> {:error, :unknown_event}
  end
end
```

**Why it's OK here:** The data comes from an external webhook as JSON. Converting to atoms would risk atom exhaustion from untrusted input. String matching at the boundary is correct — but convert to atoms/structs after validation for internal use.

---

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

### When to Apply This Rule

**Triggers:**
- A map with the same keys appears in 3+ places
- You find yourself documenting "this map must have keys X, Y, Z"
- Functions validate map keys at runtime (`Map.has_key?` checks)
- Bugs from typos in map keys (`%{stauts: :active}` instead of `:status`)

**Example — the smell:**
```elixir
def create_order(user, items) do
  %{
    user_id: user.id,
    items: items,
    status: :pending,
    total: calculate_total(items),
    created_at: DateTime.utc_now()
  }
end

def ship_order(order) do
  %{order | stauts: :shipped}  # Typo! No error, just a new key added
end
```

**Example — fixed:**
```elixir
defmodule Order do
  @enforce_keys [:user_id, :items, :total]
  defstruct [:user_id, :items, :total, status: :pending, created_at: nil]
end

def create_order(user, items) do
  %Order{
    user_id: user.id,
    items: items,
    total: calculate_total(items),
    created_at: DateTime.utc_now()
  }
end

def ship_order(%Order{} = order) do
  %{order | status: :shipped}  # Typo would raise KeyError!
end
```

### Exceptions (When This Rule Doesn't Apply)

**It's OK when:**
- The shape is genuinely dynamic (user-defined fields, plugin metadata)
- It's a short-lived intermediate value in a pipeline
- You're working with ETS/Mnesia where struct overhead adds complexity

**Example of acceptable use:**
```elixir
# Dynamic key-value config — shape unknown at compile time
def merge_config(defaults, overrides) do
  Map.merge(defaults, overrides)
end
```

**Why it's OK here:** The config keys are user-defined and unbounded. A struct would require knowing all possible keys upfront, which contradicts the purpose of dynamic configuration.

---

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

### When to Apply This Rule

**Triggers:**
- More than 2 levels of `case`/`if`/`cond` nesting
- Each nested level only handles `{:ok, _}` and passes through `{:error, _}`
- The "else" branches are all identical error pass-through

**Example — the smell:**
```elixir
def register_user(params) do
  case validate_email(params.email) do
    :ok ->
      case validate_password(params.password) do
        :ok ->
          case check_uniqueness(params.email) do
            :ok ->
              case create_user(params) do
                {:ok, user} -> send_welcome_email(user)
                error -> error
              end
            error -> error
          end
        error -> error
      end
    error -> error
  end
end
```

**Example — fixed:**
```elixir
def register_user(params) do
  with :ok <- validate_email(params.email),
       :ok <- validate_password(params.password),
       :ok <- check_uniqueness(params.email),
       {:ok, user} <- create_user(params) do
    send_welcome_email(user)
  end
end
```

### Exceptions (When This Rule Doesn't Apply)

**It's OK when:**
- Each nesting level handles different error cases with distinct logic (not just pass-through)
- The nesting represents genuinely different decision branches (a state machine)
- You need to bind variables from outer scopes in inner blocks

**Example of acceptable use:**
```elixir
def handle_response(response) do
  case response.status do
    200 ->
      case Jason.decode(response.body) do
        {:ok, %{"type" => "redirect"}} -> follow_redirect(response)
        {:ok, data} -> {:ok, data}
        {:error, _} -> {:error, :invalid_json}
      end
    401 -> refresh_token_and_retry()
    429 -> schedule_retry(response)
    _ -> {:error, {:http_error, response.status}}
  end
end
```

**Why it's OK here:** Each branch has genuinely different handling logic. The inner `case` isn't just passing through errors — it's making a distinct decision based on the decoded content. A `with` would actually obscure the branching intent.

---

## 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:29-32` — Each test gets a uniquely-named Registry:
```elixir
name = :"#{config.test}_#{partitions}_#{inspect(keys)}"
```

`lib/elixir/test/elixir/gen_server_test.exs:164` — 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).

### When to Apply This Rule

**Triggers:**
- Tests that use hardcoded registered names (`:my_server`, `MyApp.Cache`)
- ETS tables created with a fixed name across tests
- Tests that must run with `async: false` but you're not sure why

**Example — the smell:**
```elixir
test "cache stores values" do
  :ets.new(:test_cache, [:set, :named_table, :public])
  :ets.insert(:test_cache, {:key, "value"})
  assert :ets.lookup(:test_cache, :key) == [{:key, "value"}]
end
# Second test crashes: :test_cache already exists!
```

**Example — fixed:**
```elixir
test "cache stores values", %{test: test_name} do
  table = :ets.new(test_name, [:set, :public])
  :ets.insert(table, {:key, "value"})
  assert :ets.lookup(table, :key) == [{:key, "value"}]
end
```

### Exceptions (When This Rule Doesn't Apply)

**It's OK when:**
- The test explicitly needs to verify interaction with a shared resource (e.g., testing a distributed lock)
- Using `async: false` with proper setup/teardown for integration tests that inherently need global state

**Example of acceptable use:**
```elixir
# Integration test that MUST test global behavior
describe "cluster-wide rate limiter" do
  @tag :integration
  setup do
    RateLimiter.reset(:global_limiter)
    on_exit(fn -> RateLimiter.reset(:global_limiter) end)
  end

  test "limits across processes" do
    # ...
  end
end
```

**Why it's OK here:** The test is explicitly verifying shared-state behavior. The global name is the thing under test, not an incidental implementation detail.

---

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

### When to Apply This Rule

**Triggers:**
- `:sys.get_state(pid)` in test code
- Accessing `__struct__` fields that aren't part of the public API
- Testing that internal ETS tables have specific entries
- Assertions on GenServer state shape rather than behavior

**Example — the smell:**
```elixir
test "adding item updates internal state" do
  {:ok, pid} = ShoppingCart.start_link()
  ShoppingCart.add_item(pid, "book")

  state = :sys.get_state(pid)
  assert state.items == ["book"]
  assert state.count == 1
end
```

**Example — fixed:**
```elixir
test "adding item makes it appear in cart" do
  {:ok, pid} = ShoppingCart.start_link()
  ShoppingCart.add_item(pid, "book")

  assert ShoppingCart.list_items(pid) == ["book"]
  assert ShoppingCart.count(pid) == 1
end
```

### Exceptions (When This Rule Doesn't Apply)

**It's OK when:**
- Testing the GenServer implementation itself (e.g., you're writing a GenServer library)
- Debugging a flaky test and temporarily inspecting state to understand what's happening
- The process IS a data store and `:sys.get_state` is effectively part of the contract (rare)

**Example of acceptable use:**
```elixir
# Testing a custom GenServer behaviour/library
test "init callback receives options" do
  {:ok, pid} = MyCustomServer.start_link(initial_count: 5)
  # We're testing the framework, not the app — state IS the public contract
  assert :sys.get_state(pid) == %{count: 5}
end
```

**Why it's OK here:** You're testing the server framework itself, where the state management IS the feature. The state shape is the public contract of your library.

---

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

### When to Apply This Rule

**Triggers:**
- `setup` block exceeds 15 lines
- Tests can't be understood without reading the setup first
- Setup creates objects the test doesn't use (over-fetching)
- Multiple describes share the same bloated setup

**Example — the smell:**
```elixir
setup do
  user = insert(:user, role: :admin, verified: true, plan: :pro)
  org = insert(:org, owner: user, plan: :enterprise)
  team = insert(:team, org: org, name: "Engineering")
  project = insert(:project, team: team, status: :active)
  repo = insert(:repo, project: project)
  branch = insert(:branch, repo: repo, name: "main")
  commit = insert(:commit, branch: branch, author: user)
  %{user: user, org: org, team: team, project: project, repo: repo, branch: branch, commit: commit}
end

test "user can see their name", %{user: user} do
  assert user.name != nil  # Only needed the user!
end
```

**Example — fixed:**
```elixir
test "user can see their name" do
  user = insert(:user, name: "Alice")
  assert user.name == "Alice"
end

# If multiple tests need a complex fixture, use a helper:
defp setup_full_project do
  user = insert(:user, role: :admin)
  org = insert(:org, owner: user)
  # ... only when tests actually need all of this
  %{user: user, org: org}
end
```

### Exceptions (When This Rule Doesn't Apply)

**It's OK when:**
- Integration tests that genuinely need a complex world state (database with relationships)
- The setup IS the test subject (testing that complex initialization works)
- All tests in the describe block actually use all setup values

**Example of acceptable use:**
```elixir
# Every test in this describe needs the full graph
describe "organization billing" do
  setup do
    org = insert(:org, plan: :enterprise)
    team = insert(:team, org: org)
    members = insert_list(5, :user, team: team)
    %{org: org, team: team, members: members}
  end

  test "charges per seat", %{org: org, members: members} do
    assert Billing.calculate(org) == length(members) * org.per_seat_price
  end

  test "prorates mid-month additions", %{org: org, team: team} do
    new_member = insert(:user, team: team, joined_at: mid_month())
    assert Billing.calculate(org) |> includes_proration?(new_member)
  end
end
```

**Why it's OK here:** Every test uses the org/team/members graph. The setup represents the minimum viable state for billing tests.

---

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

### When to Apply This Rule

**Triggers:**
- `spawn/1` or `spawn_link/1` in `lib/` code (not test code)
- Processes that aren't in any supervision tree
- "Ghost" processes found in Observer that nobody owns
- Application shutdown hangs (orphan processes blocking)

**Example — the smell:**
```elixir
def handle_cast({:process_job, job}, state) do
  spawn(fn ->
    result = expensive_computation(job)
    notify_completion(result)
  end)
  {:noreply, state}
end
```

**Example — fixed:**
```elixir
def handle_cast({:process_job, job}, state) do
  Task.Supervisor.start_child(MyApp.JobSupervisor, fn ->
    result = expensive_computation(job)
    notify_completion(result)
  end)
  {:noreply, state}
end
```

### Exceptions (When This Rule Doesn't Apply)

**It's OK when:**
- Test helpers that need a short-lived process fixture
- `spawn_link` in a supervised process that intentionally ties the child's fate to the parent
- One-shot scripts or Mix tasks (not long-running application code)

**Example of acceptable use:**
```elixir
# Test helper — short-lived, test process will clean up
test "monitors detect process death" do
  pid = spawn(fn -> Process.sleep(:infinity) end)
  ref = Process.monitor(pid)
  Process.exit(pid, :kill)
  assert_receive {:DOWN, ^ref, :process, ^pid, :killed}
end
```

**Why it's OK here:** It's test code. The spawned process is a fixture that will be cleaned up when the test process exits. Adding supervision would be over-engineering for a test helper.

---

## 12. Atom Creation from User Input

**What they avoid:** Converting untrusted strings to atoms.

**Source evidence:** `lib/elixir/lib/option_parser.ex:859` — 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)
```

### When to Apply This Rule

**Triggers:**
- `String.to_atom/1` called on data from HTTP requests, WebSocket messages, or file input
- Atom conversion inside a loop or recursive function
- Phoenix controller that converts params to atoms

**Example — the smell:**
```elixir
def handle_event(event_name, payload, socket) do
  # event_name comes from the client!
  atom_event = String.to_atom(event_name)
  apply(__MODULE__, atom_event, [payload, socket])
end
```

**Example — fixed:**
```elixir
@allowed_events ~w(click submit toggle)a

def handle_event(event_name, payload, socket) do
  case String.to_existing_atom(event_name) do
    event when event in @allowed_events ->
      apply(__MODULE__, event, [payload, socket])
    _ ->
      {:noreply, socket}
  end
rescue
  ArgumentError -> {:noreply, socket}
end
```

### Exceptions (When This Rule Doesn't Apply)

**It's OK when:**
- Compile-time code generation (macros) where the set of atoms is fixed
- Deserializing from a trusted source (e.g., `:erlang.binary_to_term` with `:safe` option from your own nodes)
- The input is already validated against a known allowlist before conversion

**Example of acceptable use:**
```elixir
# Compile-time macro — bounded, known set
defmacro define_events(events) do
  for event <- events do
    quote do
      def handle_event(unquote(Atom.to_string(event)), payload, socket) do
        unquote(event)(payload, socket)
      end
    end
  end
end
```

**Why it's OK here:** The atoms are created at compile time from a developer-defined list. The set is bounded and trusted. No runtime user input is involved.

---

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

### When to Apply This Rule

**Triggers:**
- A behaviour with callbacks like `format/1`, `serialize/1`, `display/1` — data transformation, not process lifecycle
- You want third-party types to implement your interface without modifying their code
- The dispatch is based on the data type, not the module providing the logic

**Example — the smell:**
```elixir
defmodule Formatter do
  @callback format(term()) :: String.t()
end

defmodule JSONFormatter do
  @behaviour Formatter
  def format(data), do: Jason.encode!(data)
end

defmodule XMLFormatter do
  @behaviour Formatter
  def format(data), do: XmlBuilder.generate(data)
end

# Usage requires knowing which module to call
JSONFormatter.format(data)
```

**Example — fixed:**
```elixir
defprotocol Formattable do
  @doc "Format data for output"
  def format(data)
end

defimpl Formattable, for: Map do
  def format(data), do: Jason.encode!(data)
end

defimpl Formattable, for: List do
  def format(data), do: Enum.join(data, ", ")
end

# Dispatch is automatic based on type
Formattable.format(%{name: "Alice"})
Formattable.format(["a", "b", "c"])
```

### Exceptions (When This Rule Doesn't Apply)

**It's OK when:**
- The interface defines process lifecycle callbacks (init, handle_call, terminate)
- You need compile-time guarantees that a module implements all required functions
- The dispatch is by module (strategy pattern), not by data type

**Example of acceptable use:**
```elixir
defmodule Storage do
  @callback store(key :: String.t(), value :: binary()) :: :ok | {:error, term()}
  @callback fetch(key :: String.t()) :: {:ok, binary()} | {:error, :not_found}
  @callback delete(key :: String.t()) :: :ok
end

# Used as a strategy — the MODULE is chosen, not the data type
defmodule MyApp.Upload do
  @storage Application.compile_env(:my_app, :storage_backend)
  def save(file), do: @storage.store(file.name, file.content)
end
```

**Why it's OK here:** The dispatch is by configured module (strategy pattern), not by data type. You want compile-time verification that the storage module implements all required operations. A protocol wouldn't help because the data going in is always the same type — it's the *implementation* that varies.

<!-- PATTERN_COMPLETE -->