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:

# 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

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

test "pubsub delivers messages" do
  PubSub.subscribe(:topic)
  PubSub.publish(:topic, "hello")
  Process.sleep(100)
  assert_received {"hello"}
end

Example — fixed:

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:

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

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:

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

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:

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:

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

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

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:

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:

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

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:

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:

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:

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

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:

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:

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

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

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:

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:

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

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

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:

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:

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:28-31 — Each test gets a uniquely-named Registry:

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

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:

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:

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:

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

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:

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:

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

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:

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:

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

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

def handle_cast({:process_job, job}, state) do
  spawn(fn ->
    result = expensive_computation(job)
    notify_completion(result)
  end)
  {:noreply, state}
end

Example — fixed:

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:

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

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

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:

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

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

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:

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:

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.