docs: idiomatic Elixir and Phoenix patterns with source citations
Extracted patterns, conventions, and code smells directly from the Elixir and Phoenix source code with file path and line number citations. Covers: GenServer, error handling, data transforms, process design, testing, documentation, typespecs, macros, behaviours, module organization, Phoenix-specific patterns, framework deviations, and anti-patterns.
This commit is contained in:
Aaron Weiker
@@ -0,0 +1,265 @@
|
||||
# Common Mistakes
|
||||
|
||||
What "bad Elixir" looks like, based on what the source code explicitly warns against or demonstrates the correct way to avoid.
|
||||
|
||||
## 1. Using `++` in a Loop (O(n²) List Building)
|
||||
|
||||
**Source:** `lib/elixir/lib/enum.ex:306-320` (internal macros all use prepend)
|
||||
|
||||
```elixir
|
||||
# What the source does: prepend then reverse
|
||||
defmacrop next(_, entry, acc) do
|
||||
quote(do: [unquote(entry) | unquote(acc)])
|
||||
end
|
||||
```
|
||||
|
||||
**The mistake:**
|
||||
```elixir
|
||||
# O(n²) — copies the entire left list for every element
|
||||
Enum.reduce(items, [], fn item, acc -> acc ++ [transform(item)] end)
|
||||
```
|
||||
|
||||
**The fix:**
|
||||
```elixir
|
||||
# O(n) — prepend is O(1), reverse once at the end
|
||||
items |> Enum.map(&transform/1)
|
||||
# or
|
||||
Enum.reduce(items, [], fn item, acc -> [transform(item) | acc] end) |> Enum.reverse()
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
## 2. Forgetting `@impl true`
|
||||
|
||||
**Source:** `lib/elixir/lib/gen_server.ex:44-56` (every callback uses @impl)
|
||||
|
||||
**The mistake:**
|
||||
```elixir
|
||||
defmodule MyServer do
|
||||
use GenServer
|
||||
|
||||
# Typo! This will never be called — no warning without @impl
|
||||
def handle_cll(msg, _from, state) do
|
||||
{:reply, msg, state}
|
||||
end
|
||||
end
|
||||
```
|
||||
|
||||
**The fix:**
|
||||
```elixir
|
||||
@impl true
|
||||
def handle_call(msg, _from, state) do
|
||||
{:reply, msg, state}
|
||||
end
|
||||
```
|
||||
|
||||
With `@impl true`, the compiler catches the typo at compile time.
|
||||
|
||||
---
|
||||
|
||||
## 3. Not Handling All `with` Failure Cases
|
||||
|
||||
**Source:** `lib/elixir/lib/kernel/special_forms.ex:1680-1715` (with Beware! section)
|
||||
|
||||
**The mistake:**
|
||||
```elixir
|
||||
with {:ok, width} <- Map.fetch(opts, "width"),
|
||||
{:ok, height} <- Map.fetch(opts, "height") do
|
||||
{:ok, width * height}
|
||||
else
|
||||
# Only handles one case — what if Map.fetch returns something else?
|
||||
:error -> {:error, :missing_field}
|
||||
end
|
||||
```
|
||||
|
||||
If an `else` block is used and no clause matches, a `WithClauseError` is raised.
|
||||
|
||||
**The fix:** Either handle all possible non-match values in `else`, or better yet, normalize return values in helper functions so you don't need `else` at all.
|
||||
|
||||
---
|
||||
|
||||
## 4. async Without await
|
||||
|
||||
**Source:** `lib/elixir/lib/task.ex:38-40`
|
||||
|
||||
> If you are using async tasks, you **must await** a reply as they are *always* sent.
|
||||
|
||||
**The mistake:**
|
||||
```elixir
|
||||
# Leaked reference — message sits in mailbox forever
|
||||
Task.async(fn -> send_email(user) end)
|
||||
# Never awaited!
|
||||
```
|
||||
|
||||
**The fix:**
|
||||
```elixir
|
||||
# Fire-and-forget: use start_child
|
||||
Task.Supervisor.start_child(MyApp.TaskSupervisor, fn -> send_email(user) end)
|
||||
|
||||
# OR if you need the result:
|
||||
task = Task.async(fn -> send_email(user) end)
|
||||
Task.await(task)
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
## 5. Anonymous Functions in Distributed Agents
|
||||
|
||||
**Source:** `lib/elixir/lib/agent.ex:141-160` ("A word on distributed agents")
|
||||
|
||||
> In a distributed setup with multiple nodes, the API that accepts anonymous
|
||||
> functions only works if the caller (client) and the agent have the same
|
||||
> version of the caller module.
|
||||
|
||||
**The mistake:**
|
||||
```elixir
|
||||
# Fails if nodes have different code versions
|
||||
Agent.get({MyAgent, :remote@node}, fn state -> state.count end)
|
||||
```
|
||||
|
||||
**The fix:**
|
||||
```elixir
|
||||
# Use MFA (module, function, args) for distributed calls
|
||||
Agent.get({MyAgent, :remote@node}, MyModule, :get_count, [])
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
## 6. Starting Processes Outside Supervision Trees
|
||||
|
||||
**Source:** `lib/elixir/lib/task.ex:100-115`
|
||||
|
||||
> We encourage developers to rely on supervised tasks as much as possible.
|
||||
|
||||
**The mistake:**
|
||||
```elixir
|
||||
# No supervision, no monitoring, no logging
|
||||
spawn(fn -> do_important_work() end)
|
||||
|
||||
# Or:
|
||||
Task.async(fn -> do_important_work() end) |> Task.await()
|
||||
# Linked to caller but not supervised
|
||||
```
|
||||
|
||||
**The fix:**
|
||||
```elixir
|
||||
# Add to your supervision tree:
|
||||
{Task.Supervisor, name: MyApp.TaskSupervisor}
|
||||
|
||||
# Then use it:
|
||||
Task.Supervisor.start_child(MyApp.TaskSupervisor, fn -> do_important_work() end)
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
## 7. Putting State Logic in Controllers
|
||||
|
||||
**Source:** `lib/phoenix/controller.ex:28-45`
|
||||
|
||||
Controllers are shown as thin dispatch layers:
|
||||
```elixir
|
||||
def show(conn, %{"id" => id}) do
|
||||
user = Repo.get(User, id)
|
||||
render(conn, :show, user: user)
|
||||
end
|
||||
```
|
||||
|
||||
**The mistake:**
|
||||
```elixir
|
||||
def create(conn, params) do
|
||||
# Business logic in the controller
|
||||
changeset = User.changeset(%User{}, params)
|
||||
if changeset.valid? do
|
||||
user = Repo.insert!(changeset)
|
||||
send_welcome_email(user)
|
||||
update_analytics(user)
|
||||
notify_admin(user)
|
||||
render(conn, :show, user: user)
|
||||
else
|
||||
render(conn, :error, errors: changeset.errors)
|
||||
end
|
||||
end
|
||||
```
|
||||
|
||||
**The fix:** Move business logic to a context module. The controller just dispatches:
|
||||
```elixir
|
||||
def create(conn, params) do
|
||||
case Accounts.register_user(params) do
|
||||
{:ok, user} -> render(conn, :show, user: user)
|
||||
{:error, changeset} -> render(conn, :error, errors: changeset.errors)
|
||||
end
|
||||
end
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
## 8. Using `:permanent` Restart for One-Shot Tasks
|
||||
|
||||
**Source:** `lib/elixir/lib/task.ex:178-186`
|
||||
|
||||
> a Task has a default `:restart` of `:temporary`. This means the task will
|
||||
> not be restarted even if it crashes.
|
||||
|
||||
**The mistake:**
|
||||
```elixir
|
||||
# Will restart infinitely if the HTTP call keeps failing
|
||||
use Task, restart: :permanent
|
||||
|
||||
def start_link(url) do
|
||||
Task.start_link(fn -> HTTP.get!(url) end)
|
||||
end
|
||||
```
|
||||
|
||||
**The fix:** Use `:temporary` (default) for one-shot work. Use `:transient` if you want restart only on abnormal exit:
|
||||
```elixir
|
||||
use Task, restart: :transient
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
## 9. Pattern Matching the Internals of Opaque Types
|
||||
|
||||
**Source:** `lib/elixir/lib/task.ex:298-300`
|
||||
|
||||
```elixir
|
||||
@opaque ref :: reference()
|
||||
```
|
||||
|
||||
**The mistake:**
|
||||
```elixir
|
||||
# Accessing internal structure of an opaque type
|
||||
%Task{ref: ref} = task
|
||||
send(ref, :custom_message) # This breaks if internals change
|
||||
```
|
||||
|
||||
**The fix:** Use the public API. If a type is `@opaque`, its structure is not guaranteed between versions. Use functions like `Task.await/2` that work with the type properly.
|
||||
|
||||
---
|
||||
|
||||
## 10. Not Using `on_exit` for Test Cleanup
|
||||
|
||||
**Source:** `lib/ex_unit/lib/ex_unit/case.ex:86-94`
|
||||
|
||||
**The mistake:**
|
||||
```elixir
|
||||
test "writes to file" do
|
||||
File.write!("/tmp/test_file", "data")
|
||||
assert File.read!("/tmp/test_file") == "data"
|
||||
File.rm!("/tmp/test_file") # Never runs if assert above fails!
|
||||
end
|
||||
```
|
||||
|
||||
**The fix:**
|
||||
```elixir
|
||||
setup do
|
||||
path = "/tmp/test_file_#{System.unique_integer()}"
|
||||
on_exit(fn -> File.rm(path) end)
|
||||
{:ok, path: path}
|
||||
end
|
||||
|
||||
test "writes to file", %{path: path} do
|
||||
File.write!(path, "data")
|
||||
assert File.read!(path) == "data"
|
||||
# Cleanup happens automatically, even on failure
|
||||
end
|
||||
```
|
||||
Reference in New Issue
Block a user