Edited at

【リファクタリング】こんなコードはイヤなので一刻も早く綺麗にしたい

私は以前、先人たち(と、時には隣の席に鎮座しているエンジニア)が遺した非常に残念なコードを目にすることが多いプロジェクトに参加していることがありました。

その際は、前世で何かとんでも無く悪いコトをしたことによるなのかなと厨二心を震わせるくらい、残念なコードに日夜頭を悩ませていました。

その時の経験のおかげ(??)で、自分だったらこうやって実装するなぁというストックが溜まったのでまとめました。

皆さんのエンジニアライフのお役に立てれば幸いです。 :innocent:


前提


  • この記事で出てくるコードはRuby/Railsです

  • 改善の方法はあくまでも私が良いかなと思う案であり、絶対的なものというわけではありません

  • 記載しているコードは大まかなイメージであり、あまり厳密ではありません

  • 皆さんの経験された残念なコードがあれば是非ともシェアいただきたいです


case文大好きヤロー

一つのメソッドの中で、あるオブジェクトの所属するclassなどによってcase文で条件分岐するコードを見たことある方は多いと思います。

しかもそのcase文がネストしてることも多々あるかと思います。


bad.rb

# 社員の給与計算を行う

def calc_salary(user)
# 雇用形態で分岐
case user.class
when RegularEmployee
# 勤続年数によって分岐
case user.working_period
when 1
# ...
when 2
# ...
when 3
# ...
end
when ContractEmployee
# 勤続年数によって分岐
case user.working_period
when 1
# ...
when 2
# ...
when 3
# ...
end
end
end

処理が羅列されているだけなので、上から順番に処理を追っていく分には(それが追える代物であれば)コードの処理内容を理解できるかもしれません。

その一方でこのコードには再利用性が全くなく、拡張性・保守/メンテナンス性が非常に悪いです。

再利用性に関しては、この処理の中の特定の条件を使った処理を他の箇所で使いたくなったとしても、コピペするしかありません。

拡張性に関しては、既存のコードでは判定基準を「雇用形態」と「勤続年数」の2軸で考えてますが、これに加えて「個人のKPI達成度」と「上司からのフィードバック」といった要素を追加する必要が出てきた場合には、どうしたら良いでしょうか?さらにネストを深くしていきますか??

保守/メンテナンス性に関しては、条件毎に処理する内容が異なるので、その文テストが複雑になります。またバグってた際の調査が非常に大変です。

(私見ですが、)case文を乱用している大半のケースにはOOPの概念が抜け落ちています。

この場合にはダックタイピングを用いることで、手続き的に分岐処理を量産するのでは無く、オブジェクトに対してメッセージを送るようにリファクタリングすることができます。


better.rb

def calc_salary(user)

# userオブジェクトがどのクラスに所属していても、calc_monthly_salaryを実装していればok
user.calc_monthly_salary
end

class RegularEmployee
def calc_monthly_salary
# ...
end
end

class ContractEmployee
def calc_monthly_salary
# ...
end
end


ダックタイプを初めて聞いたという方は、こちらを参考にしてみてください。

【OOP入門】Rubyでダックタイピングを理解する

case/kind_of?/is_a?が出てきた場合にはダックタイピングが使えないか検討してみる絶好のサインだと思います。


if文連結マン

あるpublicメソッドの中で長ったらしいif文を一度は見たことがあるのではないでしょうか。


bad.rb

  def check_bath_condition(water)

# 色々な処理がある
if water.temperature > 100
puts "死ぬほど熱いです"
elsif water.temperature >= 50
puts "結構熱いです"
elsif water.temperature == 40
puts "良い湯加減です"
else
puts "ぬる過ぎます"
end
# まだ他の処理が続く
end

publicメソッドの中でつらつらと長いif文を書かれると、単純にコードの見通しが悪くなりますし、コードの再利用性が下がります。

なのでprivateメソッドに切り出し、その処理をpublicメソッド側で呼んだ方がスッキリします。


better.rb

def check_bath_condition(water)

# 色々処理がある
bath_feeling(water.temperature) # <= privateメソッドを呼び出すだけ
# まだ他の処理が続く
end

private
def bath_feeling(temperature)
if temperature > 100
puts "死ぬほど熱いです"
elsif temperature >= 50
puts "結構熱いです"
elsif temperature == 40
puts "良い湯加減です"
else
puts "ぬる過ぎます"
end
end


また、そもそも切り出した処理の内容がモデル側で記述すべき代物である場合には、キチンとモデルに定義しましょう。

そうしないと、該当のモデル(とその先に繋がっているDB)で変更が発生して関連処理を修正する必要が出た場合に対応漏れが発生します。

その上、似たような判定ロジックが複数のレイヤーに散りばめられるようになるので、どの処理が信頼できるのかが分からなくなります。


better.rb

def check_bath_condition(water)

# 色々処理がある
water.bath_feeling
# まだ処理が続く
end

class BathWater
def bath_feeling
if temperature > 100
puts "死ぬほど熱いです"
elsif temperature >= 50
puts "結構熱いです"
elsif temperature == 40
puts "良い湯加減です"
else
puts "ぬる過ぎます"
end
end
end


またその場合、今のままでは一つのメソッド内で多くのことをやりすぎているので、SRPに則って処理を分解するのがより良いと思います。


better_2.rb

def check_bath_condition(water)

# 色々処理がある
water.bath_feeling
# まだ処理が続く
end

class BathWater
def bath_feeling
too_hot_feeling
a_little_hot_feeling
comfortable_feeling
too_low_feeling
end

def too_hot_temperature?
temperature > 100
end

def a_little_hot_temperature?
temperature >= 50
end

def comfortable_temperature?
temperature == 40
end

def too_low_temperature?
temperature < 50
end

def too_hot_feeling
puts "死ぬほど熱いです" if too_hot_temperature?
end

def a_little_hot_feeling
puts "結構熱いです" if a_little_hot_temperature?
end

def comfortable_feeling
puts "良い湯加減です" if comfortable_temperature?
end

def too_low_feeling
puts "ぬる過ぎます" if too_low_temperature?
end
end


一つの処理で一つのことをすることで、コードの再利用性が高まります。


変数大量生産工場

メソッドの中で処理判定に必要なフラグなどを別途作成しているようなコードを見たことはないでしょうか。

またその上で、お手製のフラグを元に複雑な条件分岐が仕込まれているようなコードが書き連ねてあったり。。


bad.rb

# 雇用形態毎に給与を計算する

def calc_salaries(employees)
employees.each do |employee|
is_regular_employee = false
is_contract_employee = false
is_commuted_minimum_working_days = false

if employee.class == RegularEmployee
is_regular_employee = true
if employee.working_days >= 20
is_commuted_minimum_working_days = true
end
elsif employee.class == ContractEmployee
is_contract_employee = false
if employee.working_days >= 18
is_commuted_minimum_working_days = true
end
end

if is_regular_employee
if is_commuted_minimum_working_days
calc_basic_regular_salary(employee)
else
calc_less_working_regular_salary(employee)
end
elsif is_contract_employee
if is_commuted_minimum_working_days
calc_basic_contract_salary(employee)
else
calc_less_working_contract_salary(employee)
end
end
end
end


もう本当にやめてくれというレベルですね。

これをやられると少なくとも2つの点で詰みます。


  • 判定処理がドメインから滲み出ている


    • ビジネスロジックに関わるような処理がモデルの外で個別に定義されているので、モデル側の情報が変更された際(カラム名の変更、条件変更)に容易に対応漏れが発生する



  • テスタビリティが非常に低い


    • 条件分岐が複雑であり、それに伴ってテストが煩雑になる

    • if文を通るたびに値がコロコロ変わるのでバグが出た際のトレースが非常に辛い



対応策としては、OOPらしくオブジェクト自身にメッセージを送るようにします。

これにより処理をシンプルに保つことができます。


better.rb

# 雇用形態毎に給与を計算する

def calc_salaries(employees)
employees.each(&:calc_salary)
end

class RegularEmployee
def calc_salary
if commuted_minimum_working_days?
calc_basic_salary
else
calc_less_working_salary
end
end

def commuted_minimum_working_days?
working_days >= 20
end

def calc_basic_salary
end

def calc_less_working_salary
end
end

class ContractEmployee
def calc_salary
if commuted_minimum_working_days?
calc_basic_salary
else
calc_less_working_salary
end
end

def commuted_minimum_working_days?
working_days >= 18
end

def calc_basic_salary
end

def calc_less_working_salary
end
end


オブジェクトは自身がどのクラスに所属しているか、どのメソッドに応答できるかなどを全て知っているので、その特性を活かさない手はありません(というか、そうでないとOOPとは呼べません)。

着眼点を手続きからオブジェクトへ変えるだけでも、柔軟性の高いコードを書くための道筋は見えてくるのかなと思います。


雑感

残念なコードはどこの組織にも多かれ少なかれ存在しているとは思いますが、こういうコードを書く方の大半は、そもそもオブジェクト指向という言葉の意味を理解していない(言葉の意味だけ知っている程度)ことが原因だと思うので、ひとまずオブジェクト指向設計実践ガイド ~Rubyでわかる 進化しつづける柔軟なアプリケーションの育て方を読んで頂きたいです。

また、この手のコードを書く人に限って「時間がなかった」という言い訳を口にされますが、このようなコードを書く方が時間がかかると思う(まともにテストを書くのが非常に面倒な設計なので)ので、結局はレベルがそれほど高くない証拠なのかなと思います。

この世から一刻も早くこのような残念なコードがなくなることを祈ってやみません。 :innocent: