attr_accessorの変数を増やす時の基準とかってありますか？

Question

題名通りです。

Railsのattr_accessorに、変数をシンボルで定義できますが、例えば一つのメソッド内でしかその変数を使わないのにattr_accessorに定義するというのは、設計的にどうなのかという質問です。

例えば、

Userインスタンスがupdateされたときに、nameカラムだけが更新されたかどうかをコントローラーで判定し、それによってリダイレクト先を変えたいとします。
まずbefore_updateにchanged メソッドを用意しようとしました。

class UsersController
  def methodA
    user.update
  end
end

と

class User
　　before_update :update_only_name?
　　def update_only_name?
  　　self.changed == ['name']
　　end
end

です。

しかし、このままでは、update_only_name?の返り値が取れず、controllerで判定できません。
なので、attr_accessorを用意し、

class User
　　attr_accessor :update_only_name
　　before_update :update_only_name?
　　def update_only_name?
  　　if self.changed == ['name']
       update_only_name = true
     end
　　end
end

にして、コントローラーで

class UsersController
  def methodA
    user.update
    if user.update_only_name == true
      redirect_to :root
    else
      redirect_to :other_path
    end
  end
end

にできます。

ですが、update_only_nameはこの機能のためだけに作られる変数であり、Userモデルの他の部分では使われない可能性が高いです。
この実装はクリーンだと思いますか？
他に方法はありますでしょうか。

何卒宜しくお願いします。

Answer 1

変更されたカラムがname(とupdated_at)だけであることを判別するだけであれば、attr_accessorもbefore_updateも不要です。

class User
  def update_only_name?
    self.previous_changes.keys == %w[name, updated_at]
  end
end

class UsersController
  def methodA
    user.update
    if user.update_only_name?
      redirect_to :root
    else
      redirect_to :other_path
    end
  end
end

ソースコードから一部抜粋

  # Change the name:
  #
  #   person.name = 'Bob'
  #   person.changed?       # => true
  #   person.name_changed?  # => true
  #   person.name_changed?(from: nil, to: "Bob") # => true
  #   person.name_was       # => nil
  #   person.name_change    # => [nil, "Bob"]
  #   person.name = 'Bill'
  #   person.name_change    # => [nil, "Bill"]
  #
  # Save the changes:
  #
  #   person.save
  #   person.changed?      # => false
  #   person.name_changed? # => false
  #
  # Reset the changes:
  #
  #   person.previous_changes         # => {"name" => [nil, "Bill"]}
  #   person.name_previously_changed? # => true
  #   person.name_previous_change     # => [nil, "Bill"]
  #   person.reload!
  #   person.previous_changes         # => {}
  #
  # Rollback the changes:
  #
  #   person.name = "Uncle Bob"
  #   person.rollback!
  #   person.name          # => "Bill"
  #   person.name_changed? # => false
  #
  # Assigning the same value leaves the attribute unchanged:
  #
  #   person.name = 'Bill'
  #   person.name_changed? # => false
  #   person.name_change   # => nil
  #
  # Which attributes have changed?
  #
  #   person.name = 'Bob'
  #   person.changed # => ["name"]
  #   person.changes # => {"name" => ["Bill", "Bob"]}
  #
  # If an attribute is modified in-place then make use of
  # <tt>[attribute_name]_will_change!</tt> to mark that the attribute is changing.
  # Otherwise \Active \Model can't track changes to in-place attributes. Note
  # that Active Record can detect in-place modifications automatically. You do
  # not need to call <tt>[attribute_name]_will_change!</tt> on Active Record models.
  #
  #   person.name_will_change!
  #   person.name_change # => ["Bill", "Bill"]
  #   person.name << 'y'
  #   person.name_change # => ["Bill", "Billy"]

Comments

[tomoharu]

まさに求めていたやつです。ありがとうございます!

Answer 2

今回の質問に関して言うなら、maisumakunさんが書かれている通りです。
「このままでは、update_only_name?の返り値が取れず」という意味が？？

一般的には、
インスタンス内でのみアクセスできるインスタンス変数を、インスタンスの外から触れるようにするために　attre_* があります。ですからその定義をするか否かは、インスタンス内で使うか否かではなく、インスタンスの外から利用するようにすべきか否かで決めるものかと。（＊）
今回の例は、methodが外から使えるのですからインスタンス変数に取り直す必要はない、ということです。

＊）リファクタリングをバリバリやるようになると、内部だけの利用でも定義することがありますが

Comments

[tomoharu]

ご回答ありがとうございます。下でも回答させていただいたのですが、changedメソッドはコールバック実行時にのみ機能するので、インスタンスメソッド内では動かず、条件文として機能しないという認識です。。。
https://qiita.com/ysKey2/items/7e429e478069b61b53bd
こちらになります

[winterboum]

call back の時に呼ぶように設定した、ということであって、そのmethodはいつ呼んでも良いのです

[tomoharu]

すみません。まだ理解できておらず。。。changedメソッドはrailsでもともと定義されているメソッドで、普通にcontrollerでuser.changed_only_name? としても、changedは機能せず、配列が空になります。。。before_updateでメソッドを呼び出して初めて、['name']という配列が返ってくるという認識なのですが。。。

[winterboum]

ああ、使うmethod間違えてますね。changed は　true、falseが返りますから。
changed_attributes　とか、使ってください。
attribute名だけ返すのもあると思う

[tomoharu]

ありがとうございます。やってみます。そうなりますと、winterboumさんの考えとしては、特にattr_accessorの数が増えること自体は悪ではないとお考えですか？

[winterboum]

微妙
必要なattr_accessorが増えることは問題無いですが、
「必要」の判定基準が問題。インスタンス内部のデータを外に扱わせると言うのは独立性が減り結合が密になるので、オブジェクト指向に限らず望ましいことでは無いので

[tomoharu]

承知しました。ありがとうございます。

Answer 3

ですが、update_only_nameはこの機能のためだけに作られる変数であり、Userモデルの他の部分では使われない可能性が高いです。

それなら変数である必要がないのではないでしょうか。直接user.update_only_name?とすれば、変数は不要かと思います。

Comments

[tomoharu]

ご回答ありがとうございます。changedメソッドはコールバック実行時にのみ機能するので、インスタンスメソッド内では動かず、条件文として機能しないんですよね。。
https://qiita.com/ysKey2/items/7e429e478069b61b53bd
こちらになります。

[maisumakun]

changesやprevious_changesなど、コールバック外で変更列を取得する手段も存在します。

[tomoharu]

ありがとうございます　調べます