Elixir Code Review

Elixir Code Review

Quick Reference

Review Checklist

Code Style

• [ ] Module names are CamelCase, function names are snake_case
• [ ] Pipe chains start with raw data, not function calls
• [ ] Private functions grouped after public functions
• [ ] No unnecessary parentheses in function calls without arguments

Pattern Matching

• [ ] Functions use pattern matching over conditionals where appropriate
• [ ] With clauses have else handling for error cases
• [ ] Guards used instead of runtime checks where possible
• [ ] Destructuring used in function heads, not body

OTP Basics

• [ ] GenServers use handle_continue for expensive init work
• [ ] Supervisors use appropriate restart strategies
• [ ] No blocking calls in GenServer callbacks
• [ ] Proper use of call vs cast (sync vs async)

Documentation

• [ ] All public functions have @doc and @spec
• [ ] Modules have @moduledoc describing purpose
• [ ] Doctests for pure functions where appropriate
• [ ] No @doc false on genuinely public functions

Security

• [ ] No String.to_atom/1 on user input (use to_existing_atom/1)
• [ ] No Code.eval_string/1 on untrusted input
• [ ] No :erlang.binary_to_term/1 without :safe option

Valid Patterns (Do NOT Flag)

• Empty function clause for pattern match - def foo(nil), do: nil is valid guard
• Using `|>` with single transformation - Readability choice, not wrong
• `@doc false` on callback implementations - Callbacks documented at behaviour level
• Private functions without @spec - @spec optional for internals
• Using `Kernel.apply/3` - Valid for dynamic dispatch with known module/function

Context-Sensitive Rules

When to Load References

• Reviewing module/function naming → code-style.md
• Reviewing with/case/cond statements → pattern-matching.md
• Reviewing GenServer/Supervisor code → otp-basics.md
• Reviewing @doc/@moduledoc → documentation.md

Gates — before reporting

Do these in order for the review batch. Do not publish findings until each step passes.

1. Protocol loaded — Read review-verification-protocol and apply its checks for each finding category you use (unused, validation, security, performance, etc.). Pass: For every substantive finding, you can name which protocol subsection you satisfied or state N/A with reason (pure style). 2. Anchored evidence — Pass: Each finding includes a concrete locator: path:line (or line range), or Module.function/arity plus a short quoted snippet from the file. 3. Claims backed by artifacts — For assertions like unused code, missing validation, or security risk, Pass: You attach the supporting artifact (e.g. search results, file read scope) or downgrade the item to an explicit question / uncertain with what you did not verify.

Before Submitting Findings

Complete Gates — before reporting (section above) first; the verification protocol is mandatory input to those gates.

Elixir Code Style

Naming Conventions

Modules

• CamelCase: MyApp.UserAccount
• Acronyms as words: MyApp.HTTPClient not MyApp.HttpClient

Functions

• snake_case: fetch_user, parse_response
• Predicate functions end with ?: valid?, empty?
• Dangerous functions end with !: save!, fetch!

Variables

• snake_case: user_name, total_count
• Unused variables prefixed with _: _ignored

Formatting

Pipe Chains

# BAD - starts with function call
String.trim(input)
|> String.downcase()
|> String.split()

# GOOD - starts with data
input
|> String.trim()
|> String.downcase()
|> String.split()

Function Ordering

defmodule MyModule do
  # 1. Module attributes
  @moduledoc "..."
  @behaviour SomeBehaviour

  # 2. use/import/alias/require
  use GenServer
  import Guards
  alias MyApp.User
  require Logger

  # 3. Module attributes (constants)
  @timeout 5000

  # 4. Struct definition
  defstruct [:field]

  # 5. Public functions
  def public_function, do: ...

  # 6. Callback implementations
  @impl true
  def handle_call(...), do: ...

  # 7. Private functions
  defp private_helper, do: ...
end

Multi-clause Functions

# GOOD - clauses grouped together
def process(nil), do: {:error, :nil_input}
def process([]), do: {:ok, []}
def process(list) when is_list(list), do: {:ok, Enum.map(list, &transform/1)}

# BAD - clauses separated by other code
def process(nil), do: {:error, :nil_input}

defp helper, do: ...

def process([]), do: {:ok, []}  # Should be with other process/1 clauses

Review Questions

1. Do module names follow CamelCase convention? 2. Do function names follow snake_case with appropriate suffixes? 3. Do pipe chains start with data, not function calls? 4. Are public functions grouped before private functions? 5. Are multi-clause functions grouped together?

Documentation

Module Documentation

@moduledoc

defmodule MyApp.UserManager do
  @moduledoc """
  Manages user lifecycle operations including creation, updates, and deletion.

  This module provides the primary interface for user management and delegates
  to the appropriate subsystems for persistence and notification.

  ## Examples

      iex> UserManager.create(%{name: "Alice", email: "alice@example.com"})
      {:ok, %User{}}

  ## Configuration

  Requires `:user_manager` config with `:repo` key.
  """
end

When to Use @moduledoc false

# Valid uses of @moduledoc false:
# 1. Private implementation modules
defmodule MyApp.Internal.Helper do
  @moduledoc false
  # ...
end

# 2. Protocol implementations
defimpl Jason.Encoder, for: MyStruct do
  @moduledoc false
  # ...
end

Function Documentation

@doc with @spec

@doc """
Fetches a user by their unique identifier.

Returns `{:ok, user}` if found, `{:error, :not_found}` otherwise.

## Examples

    iex> fetch_user(123)
    {:ok, %User{id: 123}}

    iex> fetch_user(-1)
    {:error, :not_found}
"""
@spec fetch_user(pos_integer()) :: {:ok, User.t()} | {:error, :not_found}
def fetch_user(id) when is_integer(id) and id > 0 do
  # ...
end

@spec Patterns

# Basic types
@spec add(integer(), integer()) :: integer()

# Union types
@spec parse(String.t()) :: {:ok, map()} | {:error, term()}

# Custom types
@type result :: {:ok, t()} | {:error, reason()}
@spec fetch(id()) :: result()

# Keyword options
@spec start_link(keyword()) :: GenServer.on_start()

# When clauses for type variables
@spec map(list(a), (a -> b)) :: list(b) when a: term(), b: term()

Doctests

When to Use

# GOOD - pure function, predictable output
@doc """
Calculates the factorial of n.

## Examples

    iex> Math.factorial(0)
    1

    iex> Math.factorial(5)
    120
"""
def factorial(0), do: 1
def factorial(n), do: n * factorial(n - 1)

When NOT to Use

# BAD - side effects, unpredictable
@doc """
Creates a user in the database.

## Examples

    iex> create_user(%{name: "Test"})  # Don't doctest DB operations!
    {:ok, %User{}}
"""

Review Questions

1. Do all public modules have @moduledoc? 2. Do all public functions have @doc and @spec? 3. Are doctests used for pure, deterministic functions? 4. Do @specs accurately reflect function signatures?

OTP Basics

GenServer

Use handle_continue for Expensive Init

# BAD - blocks supervisor during init
def init(args) do
  data = expensive_operation()  # Blocks!
  {:ok, data}
end

# GOOD - defers expensive work
def init(args) do
  {:ok, %{data: nil}, {:continue, :load_data}}
end

@impl true
def handle_continue(:load_data, state) do
  data = expensive_operation()
  {:noreply, %{state | data: data}}
end

Call vs Cast

# call - synchronous, returns result
def get_value(pid) do
  GenServer.call(pid, :get_value)
end

# cast - asynchronous, fire-and-forget
def increment(pid) do
  GenServer.cast(pid, :increment)
end

When to use each:

• call - Need the result, need confirmation, queries
• cast - Fire-and-forget, notifications, can't block caller

Timeouts

# Always consider timeouts for calls
def fetch_data(pid) do
  GenServer.call(pid, :fetch_data, 10_000)  # 10 second timeout
end

# Handle timeout in caller
case GenServer.call(pid, :fetch, 5_000) do
  {:ok, data} -> data
  {:error, reason} -> handle_error(reason)
rescue
  exit -> {:error, :timeout}
end

Supervisor

Restart Strategies

Child Specs

# GOOD - explicit child spec
children = [
  {MyWorker, [name: :worker, arg: value]},
  {DynamicSupervisor, name: MyApp.DynamicSup, strategy: :one_for_one}
]

Supervisor.init(children, strategy: :one_for_one)

Common Anti-Patterns

Blocking in Callbacks

# BAD - blocks the GenServer
@impl true
def handle_call(:fetch_external, _from, state) do
  result = HTTPClient.get!(url)  # Blocks all other messages!
  {:reply, result, state}
end

# GOOD - use Task for async work
@impl true
def handle_call(:fetch_external, from, state) do
  Task.async(fn -> HTTPClient.get!(url) end)
  {:noreply, %{state | pending: from}}
end

@impl true
def handle_info({ref, result}, %{pending: from} = state) do
  GenServer.reply(from, result)
  {:noreply, %{state | pending: nil}}
end

Single Process Bottleneck

# BAD - all requests through one GenServer
defmodule Cache do
  use GenServer
  def get(key), do: GenServer.call(__MODULE__, {:get, key})
  def put(key, val), do: GenServer.call(__MODULE__, {:put, key, val})
end

# GOOD - use ETS for read-heavy workloads
defmodule Cache do
  def get(key), do: :ets.lookup(:cache, key)
  def put(key, val), do: :ets.insert(:cache, {key, val})
end

Review Questions

1. Does GenServer init do expensive work synchronously? 2. Are call/cast used appropriately (sync vs async)? 3. Is there a single GenServer becoming a bottleneck? 4. Do supervisors use appropriate restart strategies?

Pattern Matching

With Clauses

Always Handle Errors

# BAD - no else clause
with {:ok, user} <- fetch_user(id),
     {:ok, account} <- fetch_account(user) do
  {:ok, account}
end
# Returns {:error, reason} tuple unhandled!

# GOOD - explicit error handling
with {:ok, user} <- fetch_user(id),
     {:ok, account} <- fetch_account(user) do
  {:ok, account}
else
  {:error, :not_found} -> {:error, :user_not_found}
  {:error, reason} -> {:error, reason}
end

Use Tagged Tuples for Clarity

# BAD - ambiguous which step failed
with {:ok, user} <- fetch_user(id),
     {:ok, posts} <- fetch_posts(user) do
  {:ok, posts}
else
  {:error, reason} -> {:error, reason}  # Which operation failed?
end

# GOOD - tagged for clarity with helper
defp tag_error({:error, reason}, tag), do: {:error, tag, reason}
defp tag_error(other, _tag), do: other

with {:ok, user} <- tag_error(fetch_user(id), :user),
     {:ok, posts} <- tag_error(fetch_posts(user), :posts) do
  {:ok, posts}
else
  {:error, :user, reason} -> {:error, {:user_fetch_failed, reason}}
  {:error, :posts, reason} -> {:error, {:posts_fetch_failed, reason}}
end

Guards

Prefer Guards Over Runtime Checks

# BAD - runtime check
def process(value) do
  if is_binary(value) do
    String.upcase(value)
  else
    raise ArgumentError
  end
end

# GOOD - guard clause
def process(value) when is_binary(value) do
  String.upcase(value)
end

Multiple Guards

# GOOD - multiple function heads with guards
def categorize(n) when n < 0, do: :negative
def categorize(0), do: :zero
def categorize(n) when n > 0, do: :positive

Destructuring

In Function Heads

# BAD - destructure in body
def process(user) do
  name = user.name
  email = user.email
  # ...
end

# GOOD - destructure in head
def process(%{name: name, email: email} = user) do
  # name and email available, plus full user if needed
end

In Case Statements

# GOOD - pattern match extracts what you need
case fetch_user(id) do
  {:ok, %User{name: name, active: true}} ->
    {:ok, "Active user: #{name}"}

  {:ok, %User{active: false}} ->
    {:error, :inactive}

  {:error, reason} ->
    {:error, reason}
end

Review Questions

1. Do with statements have else clauses handling all error cases? 2. Are guards used instead of runtime type checks? 3. Is destructuring done in function heads where possible? 4. Are pattern matches exhaustive (no unhandled cases)?