Good and Bad Elixir

June 2, 2021

I’ve seen a lot of elixir at this point, both good and bad. Through all of that code, I’ve seen similar patterns that tend to lead to worse code. So I thought I would document some of them as well as better alternatives to these patterns.

Map.get/2 and Keyword.get/2 vs. Access

Map.get/2 and Keyword.get/2 lock you into using a specific data structure. This means that if you want to change the type of structure, you now need to update all of the call sites. Instead of these functions, you should prefer using Access:

# Don't do
opts = %{foo: :bar}
Map.get(opts, :foo)

# Do
opts[:foo]

Don’t pipe results into the following function

Side-effecting functions tend to return “results” like {:ok, term()} | {:error, term()}. If your dealing with side-effecting functions, don’t pipe the results into the next function. It’s always better to deal with the results directly using either case or with.

# Don't do...
def main do
  data
  |> call_service
  |> parse_response
  |> handle_result
end

defp call_service(data) do
# ...
end

defp parse_response({:ok, result}, do: Jason.decode(result)
defp parse_response(error, do: error)

defp handle_result({:ok, decoded}), do: decoded
defp handle_result({:error, error}), do: raise error

# Do...
def main do
  with {:ok, response} <- call_service(data),
       {:ok, decoded}  <- parse_response(response) do
    decoded
  end
end

Using pipes forces our functions to handle the previous function’s results, spreading error handling throughout our various function calls. The core problem here is subtle, but it’s essential to internalize. Each of these functions has to know too much information about how it is being called. Good software design is mainly about building reusable bits that can be arbitrarily composed. In the pipeline example, the functions know how they’re used, how they’re called, and what order they’re composed.

Another problem with the pipeline approach is that it tends to assume that errors can be handled generically. This assumption is often incorrect.

When dealing with side-effects, the only function with enough information to decide what to do with an error is the calling function. In many systems, the error cases are just as important - if not more important - than the “happy path” case. The error cases are where you’re going to have to perform fallbacks or graceful degradation.

If your in a situation where errors are a vital part of your functions control flow, then it’s best to keep all of the error handling in the calling function using case statements.

# Do...
def main(id) do
  case :fuse.check(:service) do
    :ok ->
      case call_service(id) do
        {:ok, result} ->
          :ok = Cache.put(id, result)
          {:ok, result}

        {:error, error} ->
          :fuse.melt(:service)
          {:error, error}
      end

    :blown ->
      cached = Cache.get(id)

      if cached do
        {:ok, result}
      else
        {:error, error}
      end
  end
end

This increases the size of the calling function, but the benefit is that you can read this entire function and understand each control flow.

Don’t pipe into case statements

I used to be on the fence about piping into case statements, but I’ve seen this pattern abused too many times. Seriously y’all, put down the pipe operator and show a little restraint. If you find yourself piping into case, it’s almost always better to assign intermediate steps to a variable instead.

# Don't do...
build_post(attrs)
|> store_post()
|> case do
  {:ok, post} ->
    # ...

  {:error, _} ->
    # ...
end

# Do...

changeset = build_post(attrs)

case store_post(changeset) do
  {:ok, post} ->
    # ...

  {:error, _} ->
    # ...
end

Don’t hide higher-order functions

Higher-order functions are great, so try not to hide them away. If you’re working with collections, you should prefer to write functions that operate on a single entity rather than the collection itself. Then you can use higher-order functions directly in your pipeline.

# Don't do...
def main do
  collection
  |> parse_items
  |> add_items
end

def parse_items(list) do
  Enum.map(list, &String.to_integer/1)
end

def add_items(list) do
  Enum.reduce(list, 0, & &1 + &2)
end

# Do...
def main do
  collection
  |> Enum.map(&parse_item/1)
  |> Enum.reduce(0, &add_item/2)
end

defp parse_item(item), do: String.to_integer(item)

defp add_item(num, acc), do: num + acc

With this change, our parse_item and add_item functions become reusable in the broader set of contexts. These functions can now be used on a single item or can be lifted into the context of Stream, Enum, Task, or any number of other uses. Hiding this logic away from the caller is a worse design because it couples the function to its call site and makes it less reusable. Ideally, our APIs are reusable in a wide range of contexts.

Another benefit of this change is that better solutions may reveal themselves. In this case, we may decide that we don’t need the named functions and can use anonymous functions instead. We realize that we don’t need the reduce and can use sum.

def main do
  collection
  |> Enum.map(&String.to_integer/1)
  |> Enum.sum()
end

This final step may not always be the right choice. It depends on how much work your functions are doing. But, as a general rule, you should strive to eliminate functions that only have a single call site. Even though there are no dedicated names for these functions, the final version is no less “readable” than when we started. An Elixir programmer can still look at this series of steps and understand that the goal is to convert a collection of strings into integers and then sum those integers. And, they can realize this without needing to read any other functions along the way.

Avoid else in with blocks

else can be helpful if you need to perform an operation that is generic across all error values being returned. You should not use else to handle all potential errors (or even a large number of errors).

# Don't do...
with {:ok, response} <- call_service(data),
     {:ok, decoded}  <- Jason.decode(response),
     {:ok, result}   <- store_in_db(decoded) do
  :ok
else
  {:error, %Jason.Error{}=error} ->
    # Do something with json error
  {:error, %ServiceError{}=error} ->
    # Do something with service error
  {:error, %DBError{}} ->
    # Do something with db error
end

For the same reason, under no circumstances should you annotate your function calls with a name just so you can differentiate between them.

with {:service, {:ok, resp}}   <- {:service, call_service(data)},
     {:decode, {:ok, decoded}} <- {:decode, Jason.decode(resp)},
     {:db, {:ok, result}}      <- {:db, store_in_db(decoded)} do
  :ok
else
  {:service, {:error, error}} ->
    # Do something with service error
  {:decode, {:error, error}} ->
    # Do something with json error
  {:db, {:error, error}} ->
    # Do something with db error
end

If you find yourself doing this, it means that the error conditions matter. Which means that you don’t want with at all. You want case.

with is best used when you can fall through at any point without worrying about the specific error or contrary pattern. A good way to create a more unified way to deal with errors is to build a common error type like so:

defmodule MyApp.Error do
  defexception [:code, :msg, :meta]

  def new(code, msg, meta) when is_binary(msg) do
    %__MODULE__{code: code, msg: msg, meta: Map.new(meta)}
  end

  def not_found(msg, meta \\ %{}) do
    new(:not_found, msg, meta)
  end

  def internal(msg, meta \\ %{}) do
    new(:internal, msg, meta)
  end
end

def main do
  with {:ok, response} <- call_service(data),
       {:ok, decoded}  <- decode(response),
       {:ok, result}   <- store_in_db(decoded) do
    :ok
  end
end

# We wrap the result of Jason.decode in our own custom error type
defp decode(resp) do
  with {:error, e} <- Jason.decode(resp) do
    {:error, Error.internal("could not decode: #{inspect resp}")}
  end
end

This error struct provides a unified way to surface all of the errors in your application. The struct can render errors in a phoenix controller or be returned from an RPC handler. Because the struct your using is an exception, the caller can also choose to raise the error, and you’ll get well-formatted error messages.

case main() do
  {:ok, _}    -> :ok
  {:error, e} -> raise e
end

State what you want, not what you don’t

You should be intentional about your function’s requirements. Don’t bother checking that a value is not nil if what you expect it to be is a string:

# Don't do...
def call_service(%{req: req}) when not is_nil(req) do
  # ...
end

# Do...
def call_service(%{req: req}) when is_binary(req) do
  # ...
end

The same is true for case statements and if statements. Be more explicit about what it is you expect. You’d prefer to raise or crash if you receive arguments that would violate your expectations.

Only return error tuples when the caller can do something about it.

You should only force your user to deal with errors that they can do something about. If your API can error, and there’s nothing the caller can do about it, then raise an exception or throw. Don’t bother making your callers deal with result tuples when there’s nothing they can do.

# Don't do...
def get(table \\ __MODULE__, id) do
  # If the table doesn't exist ets will throw an error. Catch that and return
  # an error tuple
  try do
    :ets.lookup(table, id)
  catch
    _, _ ->
      {:error, "Table is not available"}
  end
end

# Do...
def get(table \\ __MODULE__, id) do
  # If the table doesn't exist, there's nothing the caller can do
  # about it, so just throw.
  :ets.lookup(table, id)
end

Raise exceptions if you receive invalid data.

You should not be afraid of just raising exceptions if a return value or piece of data has violated your expectations. If you’re calling a downstream service that should always return JSON, use Jason.decode! and avoid writing additional error handling logic.

# Don't do...
def main do
  {:ok, resp} = call_service(id)
  case Jason.decode(resp) do
    {:ok, decoded} ->
      decoded

    {:error, e} ->
      # Now what?...
  end
end

# Do...
def main do
  {:ok, resp} = call_service(id)
  decoded = Jason.decode!(resp)
end

This allows us to crash the process (which is good) and removes the useless error handling logic from the function.

Use for when checking collections in tests

This is a quick one, but it makes your test failures much more helpful.

# Don't do...
assert Enum.all?(posts, fn post -> %Post{} == post end)

# Do...
for post <- posts, do: assert %Post{} == post