Rubyのコードレビューでよく指摘すること

コードレビューはじめました

昨年の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は条件式の複雑さ、ブロックのネストを避けられるケースが多いので、使用を推奨しています。リーダブルコードでも同じような指摘がありました。

リーダブルコード ―より良いコードを書くためのシンプルで実践的なテクニック (Theory in practice)

Amazon

改善案(一例)

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で慣れている分、頭が混乱するので、レビューの対象としています。

温度感としては「使うな」というわけではなく、「最終手段として使ってね」という方針にしています。