こんにちは！
去年の４月に新卒入社してからお酒ばかり飲んでいるエンジニアのd4teです。
4月から11月まではUX改善チームにてお仕事検索画面のフロントエンド開発を担当しておりましたが、11月からはリファクタリングチームにてcrowdworks.jpのリファクタリングをしています。
現在のcrowdworks.jpの状況
過去の記事にもあるように、crowdworks.jpはサービスインから約8年が経過し、30万行を超えるモノリシックなRailsアプリケーションになってきていて、コード行数の増加量やファイル変更数の推移は年々鈍化してきています。
内部には開発生産性を低下させる技術的負債が溜まってきており、技術的な投資がしづらくなってきているという課題があります。
自分が所属しているチームは、外部から見た動作を変えずに内部のコードを整理するリファクタリングで技術的負債を解消し、開発生産性の向上をミッションとしたチームです。
リファクタリングチームの活動については是非こちらもご覧ください。
engineer.crowdworks.jp
qiita.com
この記事で書くこと
サービス事情はわかったのですが、リファクタリングチーム移動直後は「技術的負債、、なんかやばそう」くらいの認識でコードの良し悪しや複雑さを生んでいる原因については自分で説明できるほど理解していませんでした。
そんな中同僚の@MinoDrivenさんに「リファクタリング 既存のコードを安全に改善する」という本を貸していただきました。
その本曰く、問題を引き起こす可能性の高いコードからは不吉な臭いがするらしく、本にはその不吉な臭いを改善するための手段が詳細に書かれており、自分でもすぐに実践することができました。
コードの臭いとは、コンピュータプログラミングにおいてプログラムのソースコードに深刻な問題が存在することを示す何らかの兆候のことを言う。
この記事では、自分がリファクタリングチームで学んだ不吉な臭いの改善方法について実際にリファクタリングを行いつつ書いていこうと思います。
※ 記事中のコードはRuby書かれており、全て架空のものです。
意図が伝わりにくい名前
def calculate(bottom, height)
return (bottom * height) / 2;
end
result = calculate(3, 4)
何が悪いのか
- 定義元ではロジックが記述してあるため、処理の内容は理解できますが、その後関数を利用する場合、何を計算するものなのかは瞬時に理解できず、開発者は定義元のロジックを観に行かなけれはなりません。
- 名前が曖昧なので、本来この関数でやりたかったこと以外のロジックが入り込んでくる可能性がある（例えば四角形の計算ロジックなど）
改善策
このような曖昧な名前に対して、処理内容が伝わるように具体的な名前に変更すると下記のように修正できるのではないでしょうか？
def calculate_triangle_area(bottom, height)
return (bottom * height) / 2;
end
area = calculate_triangle_area(3, 4)
今回は非常に簡単な例ですが、処理内容が伝わる特化した名前にすることで何を意味するものなのか開発者は瞬時に判断できますし、具体性のある名前にしたことによって意味の範囲を狭めることができ、無関係なロジックの混入も防ぎます。当然仕様変更時に混乱することも少なくなります。
リファクタリングチームではこのように特化した名前を考えることを 「名前の設計」 と呼んでいて、変数名一つとってもお互いに厳しくレビューしあっていますし、この 「名前の設計」 があらゆるリファクタリングの基本になると思っています。
リファクタリングの「第3章 コードの不吉な臭い」では「不可思議の名前」として紹介されています。
コードには突飛な箇所があってはならず、明快でなければなりません。明快なコードにするためにもっとも重要なのは、適切な名前付けです。そのため開発者は関数・モジュール・変数・クラスなどの名前について行っていることや利用方法がはっきり伝わるように考えなければなりません。
複雑な条件分岐
def purchase_alcohol(customer)
if customer.age >= 20
if all_you_can_drink_plan == false
case customer.selected_alcohol
when "wine" then
selected_alcohol = Wine.new(
price: 1008,
dead_flag: false
)
when "tequila" then
selected_alcohol = Tequila.new(
price: 0,
dead_flag: true
)
when "spiritus" then
selected_alcohol = Spiritus.new(
price: 1595,
dead_flag: true
)
else
raise Exeption.new("エラー")
end
notification = Notification.new
self.class.transaciton do
execute_payment(selected_alcohol)
notification.save!
end
end
end
end
何が悪いのか
- 複数のif分岐とcase文が絡み合っており、仕様を理解しにくい
- サンプルコードのような範囲では、さほど問題にはなりませんが、プログラムに機能が追加されるたびに似たような条件分岐の構造が繰り返し出現するようになり、複雑性が増す可能性が高い。
- 冗長にインスタンス生成処理が記述されているため、関数自体が長大になってきていて、仕様の変更時に意図しないバグを仕込んでしまう可能性がある。
改善策
シンプルな例ではありますが、下記の2つを使ってこれら複雑なコードを改善していこうと思います。
関数の抽出
case customer.selected_alcohol
when "wine" then
selected_alcohol = Wine.new(
price: 1008,
dead_flag: false
)
when "tequila" then
selected_alcohol = Tequila.new(
price: 0,
dead_flag: true
)
when "spiritus" then
selected_alcohol = Spiritus.new(
price: 1595,
dead_flag: true
)
else
raise Exeption.new("エラー")
end
まず、これら↑case文内部で行われているインスタンスの生成処理を関数として抽出します。
def generate_wine
Wine.new(
price: 1008,
dead_flag: false
)
end
def generate_tequila
Tequila.new(
price: 0,
dead_flag: true
)
end
def generate_spiritus
Spiritus.new(
price: 1595,
dead_flag: true
)
end
そして既存のインスタンス生成処理を関数に置き換えます。
case customer.selected_alcohol
when "wine" then
selected_alcohol = generate_wine
when "tequila" then
selected_alcohol = generate_tequila
when "spiritus" then
selected_alcohol = generate_spiritus
else
raise Exeption.new("エラー")
end
続けて、それらを制御しているcase文自体も関数として抽出後、メインロジックの処理を置き換えます。
def generate_selected_alcohol(selected_alcohol)
case selected_alcohol
when "wine" then
selected_alcohol = generate_wine
when "tequila" then
selected_alcohol = generate_tequila
when "spiritus" then
selected_alcohol = generate_spiritus
else
raise Exeption.new("エラー")
end
selected_alcohol
end
def purchase_alcohol(customer)
if age >= 20
if all_you_can_drink_plan == false
selected_alcohol = generate_selected_alcohol(customer.selected_alcohol)
notification = Notification.new
self.class.transaciton do
execute_payment(selected_alcohol)
notification.save!
end
end
end
end
これで長い記述が関数に分割されて見通しがよくなりました。
分割された関数は、それぞれ責務ごとに分割されているので、仕様追加などの場合にどこに変更を加えれば良いか瞬時に理解できるようになったのに加えて、それぞれ変更した場合の影響範囲を狭めることができ、変更しやすくなりました。
ガード節
続いて、ガード節を利用してネストしたif文を解消します。
ガード節とは処理の対象外とする条件を関数やループの先頭に集める方法で、ネストを減らして正常系の処理を理解しやすくなる利点があります。
今回の例で言えば、「客（customer）の年齢が20才以上である場合」と「飲み放題プラン（all_you_can_drink_plan）ではない場合」に購入することが可能、ということが読み取れます。
逆に言うと「客が未成年」である場合と、「飲み放題プラン」の場合は購入することができないことがわかります。
if age >= 20 ... end
if all_you_can_drink_plan == false ... end
すでに「飲み放題プラン」を表現する関数はありそうなので「未成年者であるかどうか」をチェックする関数を作成します
def minor?(customer_age)
customer_age <= 19
end
そしてガード節としてif文を移動します。
def purchase_alcohol(customer)
return if minor?(customer.age)
return if all_you_can_drink_plan
selected_alcohol = generate_selected_alcohol(customer.selected_alcohol)
notification = Notification.new
self.class.transaciton do
execute_payment(selected_alcohol)
notification.save!
end
end
ガード節を使って条件記述を置き換えたおかげで、条件分岐のネストが解消され、処理の対象外とする条件が理解しやすくなったと思います。
また、追加したガード節は下記のように購入不可能なケースかどうかを評価する関数として切り出しても良いかもしれません。
def purchase_impossible?(customer_age)
minor?(customer_age) || all_you_can_drink_plan
end
冗長な処理や複雑な分岐処理を関数として抽出し、条件記述をガード節に置き換えただけですが、メインロジックの複雑性は解消され、容易に理解できるようになったかと思います。
リファクタリングの「第3章 コードの不吉な臭い」では「長い関数」として紹介されています。
長い関数は理解するのが困難であり、長い関数は小さい関数に分けるべきです。小さい関数に分けることでコードを追っていくのが大変になりますが、「意図」を示した適切な名前に命名することで、内部の実装を見なくとも先に読み進めていけるはずです。
また今回のサンプルコードではcase文は重複はしていませんでしたが、複雑さを生みやすいものとして書籍中では「重複したswitch文」として紹介されています。
switch/case文や、ネストしたif/else文の形で、コードの様々な箇所に同じ条件分岐ロジックが書かれていればそれは「不吉な臭い」です。重複した条件分岐が問題なのは、新たな分岐を追加したら、すべての重複した条件分岐を探して更新していかなければならないからです。
冗長なコメント
def purchase_alcohol(customer)
if customer.age >= 20
if all_you_can_drink_plan == false
case customer.selected_alcohol
when "wine" then
selected_alcohol = Wine.new(
price: 1008,
dead_flag: false
)
when "tequila" then
selected_alcohol = Tequila.new(
price: 0,
dead_flag: true
)
when "spiritus" then
selected_alcohol = Spiritus.new(
price: 1595,
dead_flag: true
)
else
raise Exeption.new("エラー")
end
notification = Notification.new
self.class.transaciton do
execute_payment(selected_alcohol)
notification.save!
end
end
end
end
何が悪いのか
改善策
処理の責務ごとに関数やクラスに分離され、適切に名前がつけられている処理はコードを読めば十分に理解できるのでコメントは不要になると考えます。
また、コードと比べると、コメントはメンテナンスされづらい傾向にあり、開発が進むにつれてコードとコメントの内容に乖離が発生し、混乱の元になります。
一方でコメントはコードを読む人の手がかりとなるので、コメントには実装ロジックをそのままなぞる説明ではなく、実装上表現が困難な意図・目的・理由を必要に応じて書くべきだと考えます。
def purchase_alcohol(customer)
return if purchase_impossible?(customer.age)
selected_alcohol = generate_selected_alcohol(customer.selected_alcohol)
notification = Notification.new
self.class.transaciton do
execute_payment(selected_alcohol)
notification.save!
end
end
リファクタリングの「第3章 コードの不吉な臭い」では「コメント」として紹介されています。
コメントは不吉な臭いではなく、むしろ良い香りです。問題なのはコメントが消臭剤として使われることがあるからです。コメントが非常に丁寧に書かれているのは、実はわかりにくいコードを補うためだったということがよくあるのです。適切なリファクタリングを行った後は、消臭剤として使われているコメントは表面的で意味のないものだったことがわかるでしょう。
おわりに
リファクタリングチームでの活動を通して学んだことを書いてみました。
不吉な臭いがしていたサンプルコードはリファクタリングによって改善され、理解しやすくなったと思います。
リファクタリングチームに加入する前の自分と同じように、コードの良し悪しについてあまりよくわからない方の参考になれば幸いです。
また、今回紹介させていただいた不吉な臭い や それを改善するためのテクニックはごく一部なので興味のある方は「リファクタリング 既存のコードを安全に改善する」を是非読んでみてください！
We are Hiring!!
クラウドワークス では、積極的にリファクタリングを推進していけるエンジニアを募集しています！
www.wantedly.com
