これはドリコム Advent Calendar 2018の4日目です。
3日目はSmithさんによる、デファクト開発環境が無い HTML5 ゲームの世界で逞しく生きるです。
はじめに
プログラミングにおいて、読みやすいコードを書く重要性は改めて述べる必要はないだろう。あえて一言添えるなら、ソフトウェア開発者がコードを読む時間は書く時間よりも遥かに長い。本稿では筆者が見つけた読みにくい書き方を主観的な好みで一つ挙げ、なぜ読みにくいのか、どう書くべきか、そして書き換える場合の注意点について散文的に論じる。
読みにくい書き方
今回のテーマとなるのは下のような書き方だ。
def apply_f_procedurally(original_collection) result = [] original_collection.each do |e| result << f(e) end result end
元となる集合（上の例ではoriginal_collection）の各要素に何らかの処理（f）を施した結果を得るために、結果を格納する空配列を作り、要素毎の値を加えていくやり方だ。
なぜこれが読みにくいかというと、最終的に欲しい値ではない[]を結果（result）として宣言するステップが一番最初にあるからだ。一番最初にあるので一番最初に読む。そして最終的に欲しい値ではないことに気づいて、意識の外へ捨てる。このステップが煩わしい。まるで何かをググったときトップが的外れだったようなやるせなさだ。必ずトップに的外れなページが来ることを想像して欲しい。このようなコードが増えてはいけない理由がわかると思う。
これは下のように書くべきである。
def apply_f_functionally(original_collection) original_collection.map do |e| f(e) end end
これなら元となる集合の各要素に何らかの処理を施した結果を得ると即座にわかる。
客観的な評価
前述のコードを客観的に評価してみる。尺度として以下の2つを用いる：
- ABC Metric
- flog
ABC Metric
ABC Metricはソフトウェアの分量の計測法として1997年にJerry Fitzpatrickによって考案された。必ずしも読みやすさと直結しないが、分量が多ければ読む時間がかかるし、バグが発生する可能性も高まる。算定方法は、Assignment（代入）、Branch（分岐）、Condition（条件）の数をそれぞれ自乗して加えて平方根を取る（つまりベクトル）。ここでBranchは実行地点のジャンプを指すので、メソッド呼び出しはBranchに含まれる。
Rubyの静的解析ツールrubocopはABC Metricに基づいた警告をサポートしているので、下のように設定ファイル(.rubocop.yml)で閾値を0にすれば、コード量が少なくてもABCサイズが表示される。
Metrics/AbcSize: Max: 0
2つのコードを解析した結果（抜粋）は下の通り：
samples/apply_f.rb:5:1: C: Metrics/AbcSize: Assignment Branch Condition size for apply_f_procedurally is too high. [3.16/0] def apply_f_procedurally(original_collection) ... ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ samples/apply_f.rb:15:1: C: Metrics/AbcSize: Assignment Branch Condition size for apply_f_functionally is too high. [2/0] def apply_f_functionally(original_collection) ... ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
つまりapply_f_procedurallyが3.16、apply_f_functionallyが2で、1.5倍以上の差がある。
flog
flogはSeattle.rbというRubyコミュニティで作られたRuby用の静的解析ツールで、Rubyの文法やコミュニティの価値観を反映しており、「テストしにくさ」を評価する。Seattle.rbにはAaron Pattersonを始めとした複数のRailsコミッターが在籍しており、代表作としてminitestが挙げられる。
2つのコードを書いたファイルをそのままflogにかけたところ何故かapply_f_functionallyの方が処理されなかったため、片方ずつ書いて実行した。その結果以下の値が得られた。
4.0: main#apply_f_procedurally samples/apply_f.rb:5-12 2.5: main#apply_f_functionally samples/apply_f.rb:5-7
apply_f_procedurallyが4.0、apply_f_functionallyが2.5で、apply_f_procedurallyの方が1.6倍テストしにくいと判定された。
なお本稿で示すコードはirohirokid/refactor_collection_manipulationsで公開している。
様々なケース
ここまで極めて単純な例で議論してきたが、当然ながら現場のコードはもっと複雑で、上の趣旨が当てはまるものばかりではない。以下では現実の本番コードで見つかったパターンについて、改善方法を検討していく。
参考にしたコードはいわゆるソシャゲのサーバーサイドで、約5年間に68人が何らかの手を加え、LOCは約4万となっている。
取捨選択する
元となる集合のすべての要素を使わず、一部を捨てるパターンで、例えば下のようなコードのこと。
def filter_and_apply_f(original_collection) result = [] original_collection.each do |e| next if e < 2 result << f(e) end result end
この場合はfを適用する前にselectを挟めばよい。
def select_and_apply_f(original_collection) original_collection .select { |e| e >= 2 } .map { |e| f(e) } end
結果を振り分ける
結果を１つの配列にせず、何らかの条件で２つの配列に振り分けるパターン。例えば下のコード。
def partition_procedurally(original_collection) result1 = [] result2 = [] original_collection.each do |e| result = f(e) if result < 2 result1 << result else result2 << result end end [result1, result2] end
この場合はpartitionメソッドを使えばよい。
def partition_functionally(original_collection) original_collection .map { |e| f(e) } .partition { |r| r < 2 } end
結果を３つ以上に振り分ける
下のようなパターンも見られた。
def apply_f_and_case(original_collection) result1 = [] result2 = [] result3 = [] original_collection.each do |e| result = f(e) case result when 1 result1 << result when 2 result2 << result else result3 << result end end { result1: result1, result2: result2, result3: result3, } end
この場合はgroup_byを使えばよい。下の例では、振り分け先を判定する処理を別のメソッド(group_of)として展開し、読みやすくしている。
def group_of(value) case value when 1 :result1 when 2 :result2 else :result3 end end def apply_f_and_group(original_collection) original_collection .map { |e| f(e) } .group_by { |r| group_of(r) } end
組み合わせを列挙する
下のように走査済みの要素を保存するパターンもあった。つまり各要素を走査済みの要素（最初は空）と比較して衝突を検知し、走査済みとして保存している。
def detect_collision(original_collection) collisions = [] checked = [] original_collection.each do |unchecked| collision = checked.find do |e| e == unchecked end collisions << unchecked if collision checked << unchecked end collisions.any? end
これはcombinationメソッドで書ける。
def detect_collision_w_combination(original_collection) original_collection .combination(2) .detect { |a, b| a == b } end
戻り値がBooleanではないが、変換は簡単だろう。（なおcombinationはEnumerableのメソッドではなくArrrayのメソッドである。）
複数の値を処理しながら取捨選択
さらに複雑なケースも見つかった。下の例を見て欲しい。
def complex_procedural(original_collection) result = [] original_collection.each do |a, b| result << f(a) if a result << g(b) if b end result end
元の集合は下のような配列の配列で、所々nilが混じっている。
original_collection = [ [1, 2], [nil, 3], [4, nil], ]
値があるときだけ処理するのだが、要素の位置によって処理が異なる（fまたはg）。
ここまでの議論に従えば上のコードは下のように書き直せるだろう。
def complex_functional(original_collection) original_collection .flat_map { |a, b| [(f(a) if a), (g(b) if b)] } .compact end
括弧と後置ifが見にくければ下のように書いてもよい。
def complex_functional(original_collection) original_collection .flat_map { |a, b| [a && f(a), b && g(b)] } .compact end
だがこの書き換えには矛盾がある。議論の発端は「最終的に欲しい値」に直結するコードを書くことであった。しかし上のコードでは最終的に欲しくないnilや配列を作って、compactやflat_mapで辻褄を合わせている。つまり「最終的に欲しい値」に関係ないものを意識の中に持ち込んでいる。一貫したポリシーに従っているとは言いにくい。どうするかはコンテキストに合わせて判断すべきだろう。
注意すべきケース
以上のような書き直しによってパフォーマンスが劣化、あるいは実行不能になる場合もある。以下に2つの例を挙げる。
パフォーマンスが落ちるケース
1ヶ月の日数は月によって異なるため、元の集合から目的の集合を作る関数が非効率になることがある。例えば任意の期間にかかるすべての月を列挙する場合が挙げられる。期間の初日をd1、期間の最終日をd2とすると、下のように書けるだろう。
def months_functional(d1, d2) (d1..d2) .map { |d| [d.year, d.month] } .uniq end
上のコードはd1からd2までのすべての日を挙げているので効率が悪い。結果を保存する配列を使う場合と比べてみる。比較するコードは下の通り。
def months_procedural(d1, d2) result = [] d = Date.new(d1.year, d1.month, 1) while d < d2 result << [d.year, d.month] d = d.next_month end result end
2つのコードの実行速度をbenchmark-ipsで計測したところ、下のようになった。3年分と10年分でそれぞれ比較している（詳細はリポジトリを参照）。
3 years functional 199.770 (± 1.5%) i/s - 1.007k in 5.041909s 3 years procedural 11.934k (± 3.8%) i/s - 60.435k in 5.072089s 10 years functional 196.028 (± 6.1%) i/s - 988.000 in 5.065849s 10 years procedural 12.037k (± 2.1%) i/s - 60.690k in 5.044012s
months_proceduralの方が約60倍速い。しかも前述の「複数の値を処理しながら取捨選択」で述べたのと同様、「最終的に欲しい値」ではない期間中のすべての日を列挙している。どう書くかはコンテキストに依存するだろう。
走査に負荷がかかるケース
走査に負荷がかかる、例えばクラウドストレージに保存されている複数の巨大な圧縮ファイルを走査するような場合は注意を要する。つまり全ファイルをメモリに展開したり、何度も走査したりしてはいけない。
あるいは対象が無限集合（生成され続けるログなど）の場合も同様である。たとえ各要素が1バイトでも全要素をロードできない。
つまり下のように書いてはいけない。
def stupid_billionaire URLS.map { |url| load_and_extract(url) } end
上のように書くとmapが全要素の処理結果を配列にしようとする。このような場合は下のようにlazyを使えば１要素ずつ取得できる。
def lazy_smart URLS.lazy.map { |url| load_and_extract(url) } end
まとめ
要素の集合を処理するとき、最初に結果を保持する配列を空配列として確保する書き方は不要なものを意識に持ち込むので読みにくい。またコード量も多くなり、テストもしにくい。そのような場合はmap、select、partition、group_byなどを使った改善を考慮すべきである。ただし書き換えによってパフォーマンスが劣化したり実行不能になるケースもあるので注意が必要である。また別の不要な要素を持ち込んでしまうケースもあるので、コンテキストに合わせて判断すること。
