0

これは私の after_create コールバックです:

after_create { |f| if f.target.class.eql?(Question)  
        if f.target.user != User.current_user
          Notify.create_notify( Notify::QUESTION_FOLLOW, f.target.user, User.current_user, f.target) 
        end
      elsif f.target.class.eql?(User)   
        Notify.create_notify( Notify::USER_FOLLOW, f.target, User.current_user,f.target)  if f.target.can_mail_user(:follower) 
      end 
  }

それをブロックに移動しようとしたので、以下のようになりました。

after_create do |f| 
  if f.target.class.eql?(Question)  
    if f.target.user != User.current_user
      Notify.create_notify( Notify::QUESTION_FOLLOW, f.target.user, User.current_user, f.target) 
    end
  elsif f.target.class.eql?(User)   
    Notify.create_notify( Notify::USER_FOLLOW, f.target, User.current_user,f.target)  if f.target.can_mail_user(:follower) 
  end 
end

そのコードを改善するために他に何ができますか?

4

2 に答える 2

3

そのほとんどはに行く必要がありNotifyます。これはFollowクラスだと思います。リファクタリングで自問するのに役立つ質問は、「このクラスはこの動作について知る必要がありますか?」であり、この場合、答えはノーだと思います。次のようにしてみてください。

after_create do |f|
  Notify.create_notify(f.target.user, User.current_user, f.target)
end

...残りの分岐ロジックは Notify に入ります。Notify の仕事は、どの通知を送信するかを決定することです。

于 2012-10-06T19:40:09.720 に答える
0

それを改善する 1 つの方法は、is_a? クラス名を直接比較するのではなく、メソッドを使用します。また、変数 f は十分に説明的ではありません。最後に、f.target を格納する新しい変数を作成して、同じことを繰り返さないようにします。

例えば：

after_create do |record| 
  target = record.target
  if target.is_a?(Question) 
    if target.user != User.current_user
      Notify.create_notify( Notify::QUESTION_FOLLOW, target.user, User.current_user, target) 
    end
  elsif target.is_a?(User)   
    Notify.create_notify( Notify::USER_FOLLOW, target, User.current_user, target)  if target.can_mail_user(:follower) 
  end 
end
于 2012-10-06T19:31:26.343 に答える