rubocop / rails-style-guide

A community-driven Ruby on Rails style guide
http://rails.rubystyle.guide
6.47k stars 1.06k forks source link

Clarify why model[] is preferred over read_attribute #326

Open MarcPer opened 2 years ago

MarcPer commented 2 years ago

Addresses https://github.com/rubocop/rails-style-guide/issues/155

MarcPer commented 1 year ago

@pirj that's a good point, I can adapt the PR to take this into account. How about this?

Prefer self[:attribute] over read_attribute(:attribute), if the latter is used without a block. self[] raises an ActiveModel::MissingAttributeError if the attribute is missing, whereas read_attribute does not.

[source,ruby]
----
# bad
def amount
  read_attribute(:amount) * 100
end

# good
def amount
  self[:amount] * 100
end

def amount
  read_attribute(:amount) { 0 } * 100
end
----
pirj commented 1 year ago

I think this is unnecessary, the PR is good as is. The point in preferring [] is to fail during the development phase making typos in attribute names obvious. I can't think of a real-world usage of a useful read_attribute fallback when the attribute doesn't really exist on the model.

MarcPer commented 1 year ago

After looking more into the Rails code, I found out the conclusion from the referenced discussion is not exactly correct.

If I just try any random attribute with [], I don't get an exception raised:

model[:wrong_attr]
# => nil

The MissingAttributeError exception comes in when an attribute exists in the model, but is not initialized, as written in the comments before the method definition:

person = Person.select('id').first
person[:name]            # => ActiveModel::MissingAttributeError: missing attribute: name

The important thing to realize is that the exception is only raised when Person has a name column. Otherwise the result would be nil. Given that, I'd say the text I introduced in this PR is misleading.