ブログ

読んで思い出す。忘れるために書く

本に書かれたコードをリファクタリングする

way of thinking

リファクタリングびふぉあーあふたー

まとめ
演習問題
- 感想
改善案
- 結果
Links

まとめ

達人プログラマーの演習問題(P.192)をやってみたよ
Human Readable なコードを書くことを心がけましょう
どこかのタイミングでリファクタリングする機会を定期的に設けましょう
そして安心してリファクタリングするためにはテストを書きましょう(この記事では書いてない)

演習問題

元の言語はRuby じゃないっぽいけど Ruby に換えた

if state == TEXAS
  rate = TX_RATE
  amt = base * TX_RATE
  calc = 2 * basis(amt) + extra(amt) * 1.05
elsif ((state == OHIO) || (state == MAINE))
  rate = (state == OHIO) ? OH_RATE : MN_RATE
  amt = base * rate
  calc = 2 * basis(amt) + extra(amt) * 1.05
else
  rate = 1
  amt = base
  calc = 2 * basis(amt) + extra(amt) * 1.05
end

感想

複雑度が最低限の基準値10 を超える42 点である狂気(flog による計測)
DuplicateMethodCall 警告が何度か出ている(reek による計測 ※メソッドに包んだ場合)
state を何度も比較してるのが我慢ならない
TEXAS と強い関連を持つ TX_RATE が別個の存在として登場しているのが気になる
rate, amt, calc が記述上何度も登場するのが我慢ならない
rate, amt, calc に何度か代入してるのが気に入らない(記述上。実際の処理では2度以下だとは思う)
変数名 amt (amount?)が気になる。略さずともエディタの補完機能でフルに記述すればいいと思う

このコード片以外にも読み込むべきコードはあるはずなので、コレだけで頭が一杯になるので/のは本当に苦労する

改善案

STATES_AND_RATE = {
  texas: 3,
  ohio:  2,
  maine: 2
}.freeze

# TODO: refine method name
def calc(rate: 1, base:)
  # FIXME: magic number: 2
  # FIXME: magic number: 1.05
  amount = base * rate
  2 * basis(amount) + extra(amount) * 1.05
end

base = 1_000
# 一覧になかった場合に計算が落ちてほしい
rate = STATES_AND_RATE.fetch(state.to_sym, nil)
calc(rate: rate, base: base)

結果

複雑度 42 → 16 になった(flog)
reek による警告はゼロ
state は実際数値っぽいけど気にしない(気にしないように...!!)
地域とレートのセットから値を取ってこれるようにすれば比較せずにレート値が取ってこれる(YAML ファイルで明確に設定値一覧として管理してもいいかもしれない)
計算している部分はメソッドとして抜き出した
2, 1.05 が何かわからなかった。分かる人直してほしい

Links