docs: add when/exceptions to smells
This commit is contained in:
Aaron Weiker
@@ -36,6 +36,48 @@ 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
|
||||
@@ -68,6 +110,48 @@ setup do
|
||||
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
|
||||
@@ -100,6 +184,57 @@ 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)
|
||||
@@ -120,6 +255,59 @@ Each module has a single, clear responsibility.
|
||||
|
||||
**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
|
||||
@@ -148,6 +336,50 @@ 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
|
||||
@@ -177,6 +409,69 @@ 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
|
||||
@@ -219,6 +514,73 @@ def process(input) do
|
||||
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
|
||||
@@ -236,6 +598,56 @@ name = :"#{config.test}_#{partitions}_#{inspect(keys)}"
|
||||
|
||||
**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
|
||||
@@ -248,6 +660,56 @@ name = :"#{config.test}_#{partitions}_#{inspect(keys)}"
|
||||
|
||||
**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
|
||||
@@ -266,6 +728,79 @@ name = :"#{config.test}_#{partitions}_#{inspect(keys)}"
|
||||
- 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
|
||||
@@ -289,6 +824,56 @@ spawn(fn -> do_work() end)
|
||||
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
|
||||
@@ -311,6 +896,61 @@ key = String.to_existing_atom(user_input)
|
||||
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)
|
||||
@@ -324,3 +964,74 @@ config = Map.get(settings, user_input)
|
||||
**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.
|
||||
|
||||
Reference in New Issue
Block a user