Elixirのコードレビューで出るやつ

Elixir Advent Calendar 2016 の12日目です。

Elixir で開発していてコードレビューの指摘点で出たものについて、
一般的だと思うものをいくつか列挙しました。

返り値を想定している場合は _ でマッチさせず名前付ける

名前付けておいた方が読んだときにわかる。

関数定義でマッチできるならそっちで

def spam(x) do
  case x do
    :aaa ->
      :ok
    :bbb ->
      :error
  end
end

のようにやるなら、次のようにする。

def spam(:aaa) do
  :ok
end
def spam(:bbb) do
  :error
end

if より case を使う

場合分けで if を使うことは多くない。
case を使う方が良い場合が多い。

user = Repo.get(query, id)
if user != nil do
  ...
else
  ...
end

と書くなら次のようにする。

case Repo.get(query, id) do
  nil ->
    ...
  user ->
    ...
end

エラー返り値の形式が違うものは with でまとめず case ネストで

with {:ok, foo} <- spam(),
     {:ok, bar} <- ham() do
  ...
else
  :error ->
    ...
  {:error, reason} ->
    ...
end

のように spam() は :error、ham() は {:error, reason} を返すような場合、
with でまとめるとどれがどのエラー形式を返すかわからなくなってつらい。
ので普通に case ネストで書く。

case spam() do
  {:ok, foo} ->
    case ham() do
      {:ok, bar} ->
        ...
      {:error, reason} ->
        ...
    end
  :error ->
    ...
end

関数呼び出しはカッコ省略せず必ず () 付ける

統一するメリットがあるので。

再代入しない

再代入するなら基本的に別の名前を付ける。
Phoenix の conn のようなごく一部の例外は除く。
再代入の必要性は今のところ特に感じていない。

%URI{} 使おう

url_base = "https://#{host}"
url = "#{url_base}/user/#{user_id}"

とかやるなら URI を使う。(HTTPoison など URI 型で渡せる)

url = %URI{scheme: "https",
           host: host,
           path: Path.join("/user", user_id)}

String にしたければ URI.to_string() を使えばよい。

条件を満たすならリストに追加する

List や Map を作るときに条件を満たす場合は追加することがある。
例えばこんな感じ。

@spec build_spams() :: list()
def build_spams() do
  spams = base_spams()
  spams = if condition1() do
    [additional1() | spams]
  else
    spams
  end
  spams = if condition2() do
    [additional2() | spams]
  else
    spams
  end
  spams
end

要素追加する部分を関数に切り出すと見通しがよくなる場合がある。
以下のような感じ。

@spec build_spams() :: list()
def build_spams() do
  base_spams()
  |> maybe_add_spams1()
  |> maybe_add_spams2()
end

defp maybe_add_spams1() do
  if condition1() do
    [additional1() | spams]
  else
    spams
  end
end

defp maybe_add_spams2() do
  if condition2() do
    [additional2() | spams]
  else
    spams
  end
end

おまけ: credo

CI で mix credo は必ず回すようにしている。
.credo.exs は次の感じでやってる。

%{
  configs: [
    %{
      name: "default",
      files: %{
        included: ["lib/"],
        excluded: []
      },
      checks: [
        {Credo.Check.Readability.MaxLineLength, max_length: 120},
        {Credo.Check.Readability.ModuleDoc, false},
        {Credo.Check.Refactor.Nesting, false},
        {Credo.Check.Refactor.PipeChainStart, false},
      ]
    }
  ]
}