rails-style-guide icon indicating copy to clipboard operation
rails-style-guide copied to clipboard

Clarify why model[] is preferred over read_attribute

Open MarcPer opened this issue 3 years ago • 3 comments
trafficstars

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

MarcPer avatar Aug 10 '22 11:08 MarcPer

@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
----

MarcPer avatar Oct 21 '22 13:10 MarcPer

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.

pirj avatar Oct 21 '22 14:10 pirj

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.

MarcPer avatar Oct 21 '22 14:10 MarcPer