年末の大掃除がてらリファクタリングをしよう

この記事は 「Elixir Advent Calendar 2019」 18日目の記事です。

昨日は @QWYNG さんの「OSSとしてのElixir」でした。Elixir自体への貢献、熱いお話でした。

さて、今年も残すところ2週間を切って、大掃除の季節になりました。
書き足していく内に汚くなったコードはないでしょうか？家の掃除のついでに読みにくくなったソースコードも掃除しましょう。

はじめに

本エントリーでは、主に初心者〜中級者向け(初心者や趣味プログラマ寄りかも？)にコードをキレイにするTipsをまとめました。

自分でSOLIDの原則に従って上手くいったと感じたことやOSSを見たり書籍で学んだことを徒然なるままにまとめただけで、他にもお掃除の方法は掃いて捨てるほどあると思います。掃除だけに。

簡単な自己紹介

システム業界、携帯ゲーム業界と経験した後に海外大学院に留学し、CSのMaster of Engineeringを取得しました。大学院在学中にプログラミング言語の授業でHaskellを使い、関数型言語に興味を持ち、昨年からElixirにハマっています。昨年はレイトレーサーの作成をしてみたり、今年はディープラーニングのフレームワークを作ってたりします。Elixirなにもわからない。

関数のお掃除

関数は少しずつ肥大化し、非常に汚れが溜まりやすいです。
定期的に分解してキレイにしましょう。

引数が多い問題

引数が多い時は関数内に要素が詰まりすぎなことが多いです。
関数は基本的に1つの処理をすべきで、その処理を表す名前が付けられるべきです。気付いたら名前と違うことまでしているというのはよくある話。引数が多くなる場合、このサインです。

  def train(
        model,
        loss_layer_type,
        optimizer_type,
        x_train,
        y_train,
        epochs,
        batch_size,
        learning_rate,
        parallel_size \\ 1,
        test_data \\ nil
      ) do

ごっちゃごちゃ。

同じグループのアイテムをTupleやModuleでまとめる

まだ個別で使わない複数の要素はまとめておいて利用箇所でパターンマッチング等で取れば良い。
※ 呼び出し元がまとまっていることを知らなくてはいけない依存関係は生えますが

  defp set_optimizer([layer_type, params], optimizer_type, learning_rate) do
    [
      layer_type,
      params,
      Optimizers.init(optimizer_type, params, learning_rate)
    ]
  end

変更のない設定系は一つに固めても良さそう。

def setting(epochs, batch_size, pallarel_size) do
  [epochs: epochs, batch_size: batch_size, pallarel_size: pallarel_size]
end

セットになる要素、例えばx_train(訓練データ)と y_train(訓練データの正解ラベル)は使うときまでTupleでまとめておく。

あとは、モデルとして固められるものをまとめれば・・・

  def train(model, setting, train_data, test_data \\ nil) do

すっきり。

if文をパターンマッチで置き換える

条件が増えた時にとりあえずifを書いて残ってしまうことがありがち。
ifがある時点で同じ関数で複数の処理をしている場合が多いです。

  ...
  if not is_nil(test_data) do
    IO.puts(
      "Train on #{length(elem(train_data, 1))} samples, Validation on #{length(elem(test_data, 1))} samples."
    )
  else
    IO.puts("Train on #{length(elem(train_data, 1))} samples.")
  end

  defp print_start_log({_, y_train}, nil) do
    IO.puts("Train on #{length(y_train)} samples.")
  end
  defp print_start_log({_, y_train}, {_, y_test}) do
    IO.puts(
      "Train on #{length(y_train)} samples, Validation on #{length(y_test)} samples."
    )
  end

とすると

  ...
  print_start_log(train_data, test_data)

でまとまります。ついでにパターンマッチングで elem も消してみました。

関数名

関数名もOSSとして公開するなら自分が分かる名前でなく、初見で何をする関数かわかるのが望ましいです。report だけじゃ何をレポートするのかわからないので、print_loss であったり、 save_incorrect_pairs であったり、何をするかを伝えると良いと思います。 calculate_and_print みたいな名前になると複数処理をしている可能性が高いので、分解した方が良さそうです。print_acurracy の中で calculate_accuracy を呼ぶとか。若干この例は微妙かも知れませんが、他で accuracy が欲しくなった時の作業は簡単になります。pythonやruby, Elixirは短縮形が慣習的に使われる気がしますが、C++er的にはよほどの長文でない限り省略しない方がわかりやすいと思います。この辺は好みや従うスタイルによるかも知れません。個人的に慣習でも、より良い方法を取り入れたいな、という立場ではあります。

引数はtrue/falseよりatomを

data = load_train_data(true, true, true, false)

これがどんなデータを取っているかわかるでしょうか？関数のシグネチャはフラグの方がわかりやすく感じるかも知れませんが、呼び出された時、true/falseはマジックナンバー同様に魔法のサインとなります。

data = load_train_data(:flatten, :one_hot, :normalized, :no_ops)

の方がわかりやすいと思いませんか？
若干呼ばれる側は美しくなくなるかも知れませんが、関数は定義されたところで輝くのでなく、呼ばれて初めて活躍できるのです。また、一文字のatomとかだとマジックナンバーと変わらないので意味のある長さの単語が良さそうです。

変数のお掃除

変数はそれ自体がごみになっていることもあります。

変数名

「Clean Code」では変数名は適切で長い分には短すぎるより良いと言っています。同じく、適切な長さはスコープよるとも。こちらは「リーダブルコード」にも書かれていますが、見える範囲にあるようなローカル関数(2,3行下までで完結するとか)は一文字でも構わない場面が多いです。一方、関数の仮引数のようなスコープを跨ぐものは意味のある単位が望ましいと思います。行列の場合とかで、もし倒置したものを渡す必要があるなら transposedを変数名に付けたり、付加的に必要な条件も伝わった方がベターです。

変数名と関数名で可読性を上げる

処理はなるべくコンパクトに収めたい。短ければ短ければ良い。
そんな神話がありますが、可読性が犠牲になるのは好ましくないです。

res = get_num() / 1000

で何をしてるかわかるでしょうか？引数に渡す時に多くの計算をした結果をそのまま渡すような書き方は可読性を著しく損なうので、読んでわかる単位の中間状態を変数に渡して、併せて関数名も計算の意図がわかるように書き換えると読んでてエレガントな変数・関数コンビネーションになるでしょう。

vehicle_occupancy = passenger_count() / capacity

あまり上手い例が浮かばなかったのですが、こちらの方がわかりやすいでしょう。
vehicle occupancy は乗車率です。

付け加えると行につくコメントはメンテナンスされず、実態と乖離して腐っていく傾向があるので、コメントより変数名・関数名で説明できた方が美しいです。

モジュールのお掃除

モジュールも書き足している内に汚れやすい単位です。汚れたまま放っておくとカビが生えるのでしっかりキレイにしましょう。

単一責任の原則

アンクルボブことRobert C. MartinさんのSOLIDの原則のSの単一責任の原則では

全てのクラス、モジュール、関数は1つのことに関して責任を負うべき

というようなことを言っています。代表的な文章は

"A class should have only one reason to change,"

なので、「クラスは変更する理由をたった一つだけ持つべき」というような話ですね。reasonについてはブログに説明を譲ります。

雑に言うと、一つのモジュールの意図は一つということです。Util系は若干難しい気はしますが、Utilの下にサブモジュールを切るとか、なるべく意図がわかる単位に分解しましょう。

構造体は最後の切り札に取っておく(基本使わない)

メモリ管理に関してfukuoka.ex Elixir/Phoenix Advent Calendar 17日目で書きましたが、構造体はまさしくメモリ上にゴミが残りやすいようです。構造体は変更がなく常にセットで使う時に限定した方が良さそう。プログラミングElixirでもなるべく避けるように言っていたり、そもそもElixirはErlangのデータ構造をほとんど移植していない(使うなら:queue みたいに呼ぶ)ことからもリストやマップ、Keywordに最適化してそうなので、構造体は避けた方が良さそうです。特に、初心者は構造体の再バインドや更新は地雷を踏みがちだと思います。

それに、最近気づきましたが構造体を使うより、リストやKeywordを華麗に使ったほうが関数型使いっぽくて玄人風に見えます(どうでもいいか)。

構造体の場合、必要のない情報も関数に渡すことになったりしますが、モジュール外でデータを持つ場合、処理の単位で塊にすることができ、関数呼び出しにも必要な情報だけが渡せるという副作用もあります。次に使うとしても、処理後にセットにすればいいだけです。

メモリのお掃除

メモリはゴミがたまるとデータの流れが悪くなって、最悪詰まってしまいます。

末尾再帰になってるか

末尾再帰の最適化というものが多くのプログラミング言語で実装されており、ご多分に漏れずElixirでも採用されています。これを適用すると、関数呼び出しは結果を待たなくなるため、無駄に現在の状況をスタックに積まなくなります(普通の関数呼び出しは結果を使って次の処理を進めるために状況をスタックに積んで、戻ってきた時に取り出す)。メモリを無駄に使わなくなるので、スワップも発生しにくくなり、かつ、戻ってこなくて良いので、速度も向上すると思います。大事なのは戻って処理をしないでいいことです。関数呼び出しが最後の処理の一部(結果を足すとか)になっていると最適化は行われないので気をつけましょう。

再バインドはなるべくしない

再バインドは予期せぬリークに繋がりやすいように思えます。自分でメモリ管理を行っている際に感じたのは変更のあるものとないものは分けて塊(Keywordとか)にして、変更のある場合もreduceでupdate よりput_new で0から作ったものと置き換えたりする方が確実に古きものを殺せそうです。要らない繋がりはなるべくスッキリ断ち切って清く正しく生きていきたいものです。

大容量の時はプロセスを切る

Erlang VMが優秀とはいえ、要らなくなったらすぐにGC(ガーベージコレクション)が行われるわけではないようです。ただ、プロセス終了時にはメモリがVMでなくOSに戻るレベルで(Javaなど多くのVMはVMで確保してるメモリプールに返すだけです)返却されるので、明示的にプロセスを切るのが良さそうです。
プロセス周りはそこまで詳しくないのですが、少し実験した限り、関数の戻りが必要な部分はTaskを呼び出してやるのが良さそうです。この辺は自分の中でも怪しく、ツッコミ大歓迎ですので、プロセスマスターの皆様のご助言をもらえると幸いです。

メモリの監視にrecon_allocを

これも別カレンダーで書いた話ですが、recon_alloc とログを組み合わせれば、どこでメモリが増減したのか確認できるので、どこに汚れがたまってるか確認しましょう。

やらかしがちな失敗も見直しましょう

ついでに自分がやらかしたバグの話。

Atomの罠

菊池先生もatomについて書いていますがatomはハマりがちな落とし穴です。

true は True でない

• pythonではない
• true は :true で True は True ≠ :True
• flg == True みたいなの書いてたら、はいダウト

しかし、真偽値は非常にややこしい。
Elixirは真理値として true と false を提供しています。また、 false と nil 以外は真とみなされます:とかElixir Schoolでかいてるわけですよ。

iex(1)> true
true
iex(2)> true == :true
true
iex(3)> True         
True
iex(4)> True == :True
false
iex(5)> True == :true
false
iex(6)> :a == :true
false
iex(7)> :a == :false
false
iex(8)> if :a ,do: :a, else: :else
:a
iex(9)> if True ,do: :a, else: :else
:a
iex(10)> if False ,do: true, else: false
true
iex(11)> if :a do, :a, else, :else  
true  

最後の方軽くズコーですね。まぁ、C++で数値を渡した感じですかね(0以外はtrue)。大文字はAtomなのに True == :True は false になるので、大文字の場合はコロンなくてもAtomだけど、コロン付けてもAtomだったりする。コロンなしはモジュールに予約されてるようだけど、宣言されてなくても使えるという。

Warningを無視するな

$ iex -S mix

この中に色々Warningが出たりしても、放置してるお客様はいませんか？
警告はバグ発見のヒントです。unusedの変数がホントはreduceで更新されるはずのものでバグってたとかよくある話。使わないなら_を変数名の前に付けるか、まるごと置き換えたり、使っていない関数も消してしまいましょう。もし、適切にCIが整っていればバージョン管理もしてると思うので、将来必要になったらそこから戻せばいいだけです。

スタイルガイドに従って整理整頓

コーディングスタイルに多くの人が合意したものを採用すると可読性は驚くほど上がります。Elixir Style Guideというものがあります。何を採用するかは選択の余地がありますが、なるべく多くの人が採用しているスタイルを用い、可能であればCIを使って自動的に遵守する仕組みを作りましょう。

ドキュメント(doctest含む)を整備する

ドキュメントがないコードは説明書のない製品です。複雑になればなるほど説明がないと謎の豪華な飾り物になってしまうので、負債は貯めない内に解消して、わかりやすいコメントやREADMEをお供に進みましょう。特にdoctestは一番の使い方のガイドになるので、副作用のない関数(引数で結果が一意になるもの)はカバレージ100％を目指したい。

「テストのないコードはレガシーコードだ」(レガシーコード改善ガイドより)

リファクタリングに役立つ本達

言語は違うものが多いですが、コードを美しくする要素はそう変わらないと思います。
以下の本はどの言語を使う上でも参考になると思うので、一読をおすすめします。

• Clean Code
• Clean Coder
• リーダブルコード
• レガシーコード改善ガイド
• 継続的デリバリー
• Clean Architecture

いかがでしたでしょうか？
薄くて長くて申し訳ないですが、最後までお付き合い頂きありがとうございました。

きれいなコードと共に迎える良いお年を！

明日のElixir Advent Calendar 2019は、@ndac_todoroki さんです。乞うご期待！