コードレビューはじめました
昨年の10月から勤務先企業で開発チームのリーダーになりました。元々はコードをガリガリ書くような業務をしていたのですが、ポジションが変わったため、業務の内容も変化しました。大きく変化した点としてコードをレビューしてもらう側から、コードをレビューする側になったというのがあります。実際にレビューをする側になって、気づいたことはたくさんあります。
そんな中で半年もレビューをしていると、ある程度、指摘するパターンが見えてきましたので記事にまとめておこうと思います。これが必ずしも正解というわけではなく、こういう見方もある、過去にバグが続出した際に、以下で紹介する方針をとって少しずつ改善してきたという実績を元に観点を選別しています。
自分で言うのもなんですが、レビューを行なっているコードは稼働2年以上でお客様がかなりいらっしゃる現役で稼働しているサービスのものです。
よく指摘すること
随時、更新中です。
ifがネストされすぎている(2階層以上)
プロジェクトが肥大化したり、年数が経ってくると仕様が複雑になることが多いです。結果的に設定やオプション項目が複雑化して時には複雑な条件式を作成する必要があるかもしれません。
そんな時に、条件をネストしなければいけないケースが考えられます。サンプルでコードを用意しました。分かり易さのため、煩雑さを目立つようにしました。
def set_setting_option(setting)
mode = setting.mode
if mode == "normal"
if setting.options.nil?
if setting.plan == "free"
setting.options = { :allow: false, :details: false }
else
setting.options = { :allow: true, :details: false }
end
end
elsif mode == "pro"
if setting.options.nil?
setting.options = { :allow: true, :details: true }
end
end
end
幾重にも条件が分岐かつネストしているため、見ただけで「うっ...」となります。また、新しくmodeが追加された時、optionsの内容が変化した際の拡張性も悪く、保守性も微妙ですね。このようなコードはレビューの対象となることが多いです。
対処としてはifを用いて処理を分岐させるだけではなく、関数を細かく作成して、やることを分散させてあげることで、それぞれのmodeの処理の追いやすさと拡張性を確保しています。また、早期returnは条件式の複雑さ、ブロックのネストを避けられるケースが多いので、使用を推奨しています。リーダブルコードでも同じような指摘がありました。
改善案(一例)
def set_setting_options(setting)
mode = setting.mode
case mode
when "normal"
set_normal_seting_options(setting)
when "pro"
set_pro_seting_options(setting)
else
raise "存在しないプランが設定されています👀"
setting
end
def set_normal_seting_options(setting)
return setting if !setting.options.nil?
if setting.plan == "free"
setting.options = init_options(false, false)
else
setting.options = init_options(true, false)
end
setting
end
def set_pro_seting_options(setting)
return setting if !settings.option.nil?
setting.options = init_options(true, true)
setting
end
def init_options(allow, details)
{ :allow: allow, :details: details }
end
init_optionsに関してはオプションの項目が増えた際に、引数が増えてしんどいことになるので、ハッシュを渡すようにするなど改善の余地はまだまだありそうです。
eachが多用される
配列に対する処理として何故かeachの関数が好んで使用される傾向があることに気づきました。確かに、eachだと色んな処理が表現出来るというのは分かりますが、それぞれの関数は役割を想定して作成されているので、適材適所で最もシンプルに記述できる関数を選択してあげるのが良いです。
def filter_pro_mode(settings)
new_settings = []
settings.each do |setting|
if setting.mode == "pro"
new_settings << setting
end
end
new_settings
end
こういったコードを本当によく見かけます。この場合は元の配列からmodeがproのsettingを取得したいだけなので、filterを使ってあげるのが良い選択です。
def filter_pro_mode(settings)
settings.filter { |setting| setting.mode == "pro" }
end
非常にスッキリしました。また読み手にはfilterだから要素を絞っているというのが、関数を通して伝えることが出来ます。eachの場合には処理の中身を詳細に追う必要がありました。
関数名と実行内容が一致していない
いまだに自分もこれをやってしまう時があります。関数には何をしているかが名前から分かるように命名をしてあげるのがベストです。ただ、処理が複雑なものや、言葉で表現が難しいものがあるのも事実です。ただ、以下のような場合ではレビューの対象としています。
def check_set_mode(setting)
mode = setting.mode
if setting.mode != "pro" && setting.mode != "normal"
setting.mode = "normal"
end
setting
end
この関数はcheckといいつつ、setting.modeの値を上書きしています。関数名と行なっている処理が一致していないことが分かります。この場合にはoverride_set_modeという名前が良いのではないでしょうか。
インスタンスの作成時に引数を渡して、必要な情報をインスタンス変数としてclass内に保持しておく実装は頻出で便利な書き方です。しかし、便利なインスタンス変数をよしなに使ってしまうケースがあります。インスタンス変数はインスタンス内では広いスコープを持っているため扱いには注意が必要です。
僕のおすすめとしては、インスタンス作成時に渡した値は読み取り専用で、以降の処理でインスタンス変数の値を極力、更新しないようにすることです。インスタンス変数の更新は思わぬバグを生み出します。関数の実行順序が保証されない場合は特に注意が必要です。以下のサンプルではvalidate_and_set_settingの後に別の関数が呼び出されてしまうと、@userがadmin_userだった場合に挙動が変わる可能性があるので注意が必要です。
どうしてもインスタンス変数を更新したい時は専用の関数を用意する(setter)という形でレビューを通すようにしています。
class SettingAdjusetment
def initialize(setting, request_user)
@setting = setting
@user = request_user
@mode= setting.mode
@plan = setting.plan
@options = setting.options
end
def validate_and_set_setting
return false if @plan != "free"
return false if @options != { :allow: false, :details: false }
if @user.admin_user?
@mode = "pro"
@plan = "paid"
@options = { :allow: true, :details: true }
end
end
end
(どう書いてもインスタンス変数を避ける姿勢が身についていつので、良いサンプルを用意するのが難しいです...😅)
無理に共通化しない
複雑な条件を書いて、処理を分岐させるぐらいなら関数を分割してしまった方が早いです。過去の事例でいうと、引数で渡された値のクラスによって処理を分岐させるというコードがありました。
def update_content(content)
if content.instance_of(String)
content = "string :: #{content}"
elsif content.instance_of(Number)
content = "number :: #{content}"
else
content
end
content
end
まずinstance_ofという関数が認知の低いもので、見ただけでは何をする関数なのか分からない可能性があります。合わせて、それぞれのクラスによってどのような処理がされるのかもコードを追わないと分かりません。今回はたまたま、同じような処理がされていましたが、今後、ArrayやHashの場合には、異なる処理が適応される可能性が考えられます。
このように無理に共通化をすると混乱を生み出す可能性がありますので、レビューの対象としています。改善案としては細かく関数に分割することです。
def update_string_content(content)
"string :: #{content}"
end
def update_number_content(content)
"number :: #{content}"
end
複雑だった判定は呼び出し側で、任意に使用する関数を選択させるようにします。そのためにも分かりやすさのために命名が重要です。
unlessを可能な限り使わない
RubyやElixirではunlessを使用することが出来ますが、言語によってはサポートされていないことがあります。また、unlessで表現出来る条件式は大体がifで表現することが出来ます。ifだと条件式が長くなってしまう場合にはunlessを許可することもありますが、unlessは普段ifで慣れている分、頭が混乱するので、レビューの対象としています。
温度感としては「使うな」というわけではなく、「最終手段として使ってね」という方針にしています。
総括
ベースの考えになっているのは、「明日記憶喪失になっても読めるコード」です。プロジェクトも肥大化しており、仕様もどんどん複雑になっていきます。そんな中に今回紹介したようなコードが紛れていると、複雑、拡張性がない、把握に時間かかる、潜在的なバグがある...など様々な問題を引き起こす可能性があります。
自分の仕事は様々な問題を事前に検知して、可能な限り取り除くことです。経験則に頼る部分は多いのですが、基本的なことは書籍でも広く紹介されています。自分はリーダブルコードや関数型のパラダイムが非常に役立っているなぁと思っていますので興味があれば、手にとってみて下さい。
参考文献
