【Rails】通知機能をリファクタリングしてみた

11065 ワード
Rails Ruby リファクタリング Ruby テキストリンク

プログラミングスクールに通っています。
先日ポートフォリオのフィードバックが返って来た際に、通知機能のリファクタリングをした方が良いとの指摘を受けたので、修正方法を記事にします。
通知機能に関しては以下の記事を参考にさせて頂きました。
https://qiita.com/nekojoker/items/80448944ec9aaae48d0a

修正方法

cook.rb
  def create_notification_like!(current_user)
    already_like_check = Notification.where(
      [
        "visitor_id = ? and visited_id = ? and cook_id = ? and action = ? ",
        current_user.id, user_id, id, 'like',
      ]
    )
    if already_like_check.blank?
      notification = current_user.active_notifications.new(
        cook_id: id,
        visited_id: user_id,
        action: 'like'
      )
      if notification.visitor_id == notification.visited_id
        notification.is_checked = true
      end
      notification.save if notification.valid?
    end
  end

投稿した料理に対して「いいね」がされた際に通知が行われるようにしています。
今回はモデルに定義したcreate_notification_like!メソッドをリファクタリングしていきます。

上記では
- already_like_check が blank だった場合
- notification.visitor_id と notification.visited_id が一緒だった場合
- notification が valid? だった場合
と条件が3つ出てきます。しかし、 already_like_check が blank じゃなかった場合の処理は出てきません。
つまり、already_like_check が blank じゃなかった場合 は例外的なパターンとして処理をその時点で中断してあげると良いということになります。
この考え方を適用すると下記のように実装できます。

cook.rb
class Cook < ApplicationRecord
  def create_notification_like!(current_user)
    already_like_check = Notification.where(
      [
        "visitor_id = ? and visited_id = ? and cook_id = ? and action = ? ",
        current_user.id, user_id, id, 'like',
      ]
    )
    return unless already_like_check.blank? #これを追加
    notification = current_user.active_notifications.new(
      cook_id: id,
      visited_id: user_id,
      action: 'like'
    )
    if notification.visitor_id == notification.visited_id
      notification.is_checked = true
    end
    notification.save if notification.valid?
  end
end

早期リターンという手法でifのネストを避けることができました。
（早期リターンとは簡単に言うと、「条件に合致しない場合は処理をそこでストップさせる」ことです。今回のケースではalready_like_check が blank じゃなかった場合、return unless already_like_check.blank?以下の処理を実行しないということです。）
次は メソッドの抽出 という方法を行い処理をみやすくしてみましょう。
メソッドの抽出 というのは処理の塊をメソッドに切り出してあげることです。
今回抽出できそうな処理は下記になります。

notification = current_user.active_notifications.new(
    cook_id: id,
    visited_id: user_id,
    action: 'like'
  )
if notification.visitor_id == notification.visited_id
  notification.is_checked = true
end
notification.save if notification.valid?

この処理は、
- notification.visitor_id と notification.visited_id が一緒だった場合
- notification が valid? だった場合
という2つの条件が含まれており、 create_notification_like! メソッドの中にあるとやや処理が複雑になってしまいます。
Notification クラスから作成したインスタンスに対する処理の実行ですので、 Notification クラスにインスタンスメソッドを作成してあげると良いでしょう。

notification.rb
def update_is_checked_and_save
  if visitor_id == visited_id
    is_checked = true
  end
  save if self.valid?
end

さらに、後置ifを活用して以下のようにします。

notification.rb
def update_is_checked_and_save
  is_checked = true if visitor_id == visited_id
  save if self.valid?
end

そして、このメソッドを create_notification_like! メソッド内で呼び出します。

cook.rb
def create_notification_like!(current_user)
  already_like_check = Notification.where(
    [
      "visitor_id = ? and visited_id = ? and cook_id = ? and action = ? ",
      current_user.id, user_id, id, 'like',
    ]
  )
  return unless already_like_check.blank?
  notification = current_user.active_notifications.new(
    cook_id: id,
    visited_id: user_id,
    action: 'like'
  )
  notification.update_is_checked_and_save
end

これで最初よりかはスッキリとしたと思います。

最後に

あくまで一例ですので、もっとわかり易い方法があれば教えて下さい！
以上です！ありがとうございました！

hue 4.2.0インストール(メタデータベースとしてMySQLを使用)
Hbase shell scanフィルタ