リーダブルなコードについての私見＜制御フロー編＞

こんにちは、YOUTRUSTのやまでぃ（YOUTRUST/X）です。

今回は何書くの？

下記の項目について私見を述べます。

条件式の引数の並び順
早期リターン
ネストの深さ
後置if

参考. 「読みやすいコード」についてチームで議論しました

前提

私見です。
弊社においては、意見が分かれるようなスタイルについては、必ずしも統一してはいません。
題材として、リーダブルコードを参考にしています。

重要なそもそも話ですが、細かいコーディングスタイルについてあれこれ気にするよりも、ちゃんと責務に応じて適切にクラスやメソッドのサイズをコンパクトに収めることを考えることの方が優先度高いと思っています。

コードがコンパクトになっているのであれば、どんなスタイルで書かれていても大した負荷なく読むことができます。

「細かいコーディングルール」 < 「クラスやメソッドのサイズ」

です。

（クラスの責務分割に関しては同意を得やすいが、コーディングルールに関しては主観が入り混じって統一しにくかったりもしますしね、これから細かいこと書きますが、細かいことは気にしない〜）

それでは早速参りましょう！

条件式の引数の並び順

# A
if (length >= 10)

# B
if (10 <= length)

【私見】B派です。

理由は大小の方向が数直線と一致しており、直感的に理解できるからです。

複数の引数が&&で連結されていても読みやすいです。

if 10 <= length && length <= 100
# →「10〜100ね」（脳負荷なしに即答できる）

if length >= 10 && length <= 100
# →「10以上、100以下…10〜100ってことか」（一瞬考えてしまう）

早期リターン

【私見】状況によっては使わない方が良いと思っています。

ifの解像度を少し上げて、「分岐」「選択」「アサート」に分けて考えてみます。

分岐…1本の道から二手に分かれる二股道路のイメージ（右？左？）
選択…道は1本のまま何かをチョイスするイメージ（金の斧？銀の斧？）
アサート…前提条件をチェックするイメージ（入場チケット見せて？）

この中で早期リターンは「アサート」のifに対して有効なものだと考えています。

class HogeNotification
  def initialize(from_user:)
    @from_user = from_user
  end

  def run
    to_users = fetch_target_users(user: @from_user, type: :friend_of_friend)

    to_users.each do |to_user|
      # 「分岐」のif…早期リターンしなくて良い
      if push_notifiable_to?(to_user)
        send_push_notification_to(to_user)
      end

      # まぁ現時点では↑で早期リターンしても良いが、
      # 将来、下記のような通知処理が追加された場合に手直しが必要になってしまう。
      # if email_notifiable_to?(to_user)
      #   send_email_notification_to(to_user)
      # end
    end
  end

  private

  # 「選択」のif
  def fetch_target_users(user:, type:)
    # case文でも可
    if type == :friend_candidate
      user.friend_candidates
    elsif type == :friend_of_friend
      user.friend_of_friends
    else
      user.friends
    end

    # 下記のように無理して早期リターンする必要はない
    # return user.friend_candidates if type == :friend_candidate
    # return user.friend_of_friends if type == :friend_of_friend
    # user.friends
  end

  # 「アサート」のif
  def push_notifiable_to?(user)
    # 条件が複雑になる時は特に早期リターンが有効
    return false if !user.active?
    return false if user.disabled_push_notification?(:hoge)
    # return false if ...
    # return false if ...
    # return false if ...

    true
  end
end

ネストを浅くする

【私見】状況によっては使わない方が良いと思っています。

# ネストを浅くした例
def hoge
  # a
  if user_result != 'SUCCESS'
    # b
    logger.debug('user failure')
    return
  end

  # c

  if permission_result != 'SUCCESS'
    # d
    logger.debug('permission failure')
    return
  end

  # f
  logger.debug('success')
end

確かに早期リターンを用いることによってネストの深さが1に抑えられています。

ですが、私はこのコードに対して以下のように思ってしまいます。

user_result は成功したがpermission_result は失敗した場合の処理をどこに書けばよいかが瞬時に分からない（d）
user_result も permission_result も失敗した場合の処理を書く場所がない（bに書くの？）
return漏れたらバグる危険性
ifの位置を逆にしたら挙動が変わる

これらのifは「分岐」と捉えられるので、ここでも敢えて早期リターンを使わずに記述してみます。

# 敢えてネストした例
def hoge
  if user_result == 'SUCCESS'
    if permission_result == 'SUCCESS'
      logger.debug('success')
    else
      logger.debug('permission failure')
    end
  else
    logger.debug('user failure')
  end
end

いかがでしょうか。

確かにネストは2に深くなりましたが、先に上げた点が全て解決（迷うことがない、間違えにくい）されてはいないでしょうか。

後置if

【私見】処理部が長くなるような場合には使わない方が良いと思います。

# 後置ifが辛い例
def create
  result = HogeClass.run(...)

  return render 'errors', status: :bad_request, formats: :json if !result.success?

  head :ok
end

私がこれを読んだときの脳内「最初にHogeClassを実行して、えっ？errors返すの！？400！？JSON！？あ、失敗時のみか…（びっくりした）」

…となります。

また、視線が左右に揺れて疲れたりもするし、もしかしたらifを見落としてしまうかもしれません。

下記のようにしてみます。

def create
  result = HogeClass.run(...)

  if result.success?
    head :ok
  else
    render 'errors', status: :bad_request, formats: :json
  end
end