何をしたか
if文を多用している箇所を、Decoratorを利用してhashで管理するようにしました。
これにより、View上で 35行 あったコードを 16行 まで減らすことができました。
また、同じ文言が複数箇所に散らばっていたため、一箇所で管理するようにし、改修しやすくしました。
どんなメリットがあるか
- Viewファイルの可読性の向上
- 文言修正の依頼があった場合、変更箇所が一箇所で済むようになった
具体的な内容
Userは3つの状態を持つ2種類のステータスをenumで管理していました。
class User
enum :hoge_status, { a: 0, b: 1, c: 2 }, prefix: true
enum :fuga_status, { a: 0, b: 1, c: 2 }, prefix: true
end
Viewではそれぞれのステータスで表示する内容を切り替えたかったようです。
また、ステータスがCの時のみCになった時刻が別カラムで保存されるようになっており、
その時間を表示したいという要望だったようです。
h3 ー ステータスhogeについて ー
- if @user.hoge_status_a?
p ステータスhogeはAです。
- if @user.hoge_status_b?
p ステータスhogeはBです。
- if @user.hoge_status_c?
p ステータスhogeはCです。
p = l(@user.hoge_status_at)
- unless @user.fuga_status_a?
h3 ー ステータスfugaについて ー
- if @user.fuga_status_b?
p ステータスfugaはBです。
- if @user.fuga_status_c?
p ステータスfugaはCです。
p = l(@user.fuga_status_at)
また、上記はユーザー側で、管理者側のユーザー詳細のページでも、このステータスを確認することができます。
そして、管理者側では下記のように、ステータスfugaがaであっても、aであることが明示的に表示されるようになっています。
h3 ー ステータスfugaについて ー
- if @user.fuga_status_a?
p ステータスfugaはAです。
- if @user.fuga_status_b?
p ステータスfugaはBです。
- if @user.fuga_status_c?
p ステータスfugaはCです。
p = l(@user.fuga_status_at)
実際は上記のようにシンプルではないですが、
今回は条件分岐にフォーカスするため割愛しています。
これらを踏まえてやりたかったこと
- このViewの文言はユーザー側だけでなく管理者側でも利用しているため、使いまわしたい
- 条件分岐の可読性を上げたい
どのようにしたか
初めはif文をcase文に書き換えviewで切り替えていましたが、
コードを見ていくと、文言だけ切り替えればいいことに気づき、
Docoratorに切り出すことにしました。
また、Decoratorの中でもcase文を使用していたのですが、
途中からHashへ変更しました。
ifからcaseへ
case文で記述することで@user.hoge_status_〇〇を複数回記述する必要がありません。
また、case文によって ある1つのステータスの値 によって条件分岐していることが、わかりやすくなりました。
今回はhoge_statusという短い属性名だったので問題ないのですが、
実際のコードでは、hoge_hogehoge_fugafuga_hoge_statusのようなやたらと長い属性名であったため、初めのif文を見た際に「if文ごと全て違うステータスを判定しているのか？」と思い、ステータスの確認から始めていました。
調べてみると、全て同じステータスだったため、コードリーディングに無駄な時間がかかってしまっていました。
Before
h3 ー ステータスhogeについて ー
- if @user.hoge_status_a?
p ステータスhogeはAです。
- if @user.hoge_status_b?
p ステータスhogeはBです。
- if @user.hoge_status_c?
p ステータスhogeはCです。
p = l(@user.hoge_status_at)
h3 ー ステータスfugaについて ー
- if @user.fuga_status_a?
p ステータスfugaはAです。
- if @user.fuga_status_b?
p ステータスfugaはBです。
- if @user.fuga_status_c?
p ステータスfugaはCです。
p = l(@user.fuga_status_at)
After
h3 ー ステータスhogeについて ー
- case @user.hoge_status
- when 'a'
p ステータスhogeはAです。
- when 'b'
p ステータスhogeはBです。
- when 'c'
p ステータスhogeはCです。
p = l(@user.hoge_status_at)
h3 ー ステータスfugaについて ー
- case @user.fuga_status
- when 'a'
p ステータスfugaはAです。
- when 'b'
p ステータスfugaはBです。
- when 'c'
p ステータスfugaはCです。
p = l(@user.fuga_status_at)
viewからDecoratorへ
段々とコードの全貌が見えてきたため、文言のみ変えれば良いということに気がつきました。
ロジックがviewから少なくなったため見通しが良くなり、
viewではレイアウトなどの情報に集中しやすくなりました。
Before
h3 ー ステータスhogeについて ー
- case @user.hoge_status
- when 'a'
p ステータスhogeはAです。
- when 'b'
p ステータスhogeはBです。
- when 'c'
p ステータスhogeはCです。
p = l(@user.hoge_status_at)
h3 ー ステータスfugaについて ー
- case @user.fuga_status
- when 'a'
p ステータスfugaはAです。
- when 'b'
p ステータスfugaはBです。
- when 'c'
p ステータスfugaはCです。
p = l(@user.fuga_status_at)
After
h3 ー ステータスhogeについて ー
p = @user.hoge_status_text
- if @user.hoge_status_c?
p = l(@user.hoge_status_at)
h3 ー ステータスfugaについて ー
p = @user.fuga_status_text
- if @user.fuga_status_c?
p = l(@user.fuga_status_at)
def hoge_status_text
case hoge_status
when 'a'
'ステータスhogeはAです。'
when 'b'
'ステータスhogeはBです。'
when 'c'
'ステータスhogeはCです。'
end
end
def fuga_status_text
case fuga_status
when 'a'
'ステータスfugaはAです。'
when 'b'
'ステータスfugaはBです。'
when 'c'
'ステータスfugaはCです。'
end
end
caseからhashへ
プログラミング中級者になりたい人のためのクリーンコード入門
の「条件分岐」に関する章で、
同じような処理をswitch文で書くと冗長な記述が増えてしまうため、連想配列でまとめられないか検討する。
とあったため、応用してみることにしました。
(また、私の現場のRubocopではcaseをhashにしろと怒られていました。)
Before
def hoge_status_text
case hoge_status
when 'a'
'ステータスhogeはAです。'
when 'b'
'ステータスhogeはBです。'
when 'c'
'ステータスhogeはCです。'
end
end
def fuga_status_text
case fuga_status
when 'a'
'ステータスfugaはAです。'
when 'b'
'ステータスfugaはBです。'
when 'c'
'ステータスfugaはCです。'
end
end
After
def hoge_status_text
text = {
'a' => 'ステータスhogeはAです。',
'b' => 'ステータスhogeはBです。',
'c' => 'ステータスhogeはCです。'
}
text[hoge_status]
end
def fuga_status_text
text = {
'a' => 'ステータスfugaはAです。',
'b' => 'ステータスfugaはBです。',
'c' => 'ステータスfugaはCです。'
}
text[fuga_status]
end
今後
今回の件を通じて、コードにはさまざまな記述方法ができることを実感し、
そのケースに応じて適宜使い分けることが大切だということを知りました。