Prefer method invocation over instance variables

[joshuaclayton]

Referring to instance variables in various methods within a class
violates the single level of abstraction principle; it applies a
specificity that methods using this information need no context of.

By preferring method invocation over instance variables, it provides the
developer the ability to refer to a concept which can be defined by a
method via direct definition or via attr, attr_reader, or attr_accessor.

[jferris]

I don't feel strongly about this, but I think there are reasons not to do this:

• The Ruby language seems to disagree with this guideline. If you attempt to define a private attr_reader, Ruby issues a warning. This seems to imply that Ruby considers attr_reader to be useless unless there's a reference from a separate instance.
• Using methods is slower. Although this doesn't make much difference in most applications, all abstractions in Ruby cost something in terms of performance, so I'm hesitant to agree with a guideline that explicitly dictates poorer performance.
• Using an attr_reader is an additional layer of abstraction. Unless this gains something, it simply decreases usage-site knowledge and increases the amount of complexity you have to hold in your head as a developer. When I see an instance variable reference, I know how it works. When I see an attr_reader reference, it could be anything.
• In the rare cases an instance variable needs to be replaced with a method, replacing references is trivial with a find and replace. This is one of the easiest refactorings to perform in Ruby.

I follow the style of whichever code base I'm participating in, but when I start projects from scratch, I avoid unnecessary attr_reader usage.

[gylaz]

I almost never (ever) have seen a usage of attribute reader, for anything other than returning the instance variable.

I'm a fan of instance variables,
I can right away tell what they are,
and I don't need to guess "is that method doing something other than returning the variable".
I also like how they stand out in syntax highlighting.

[joshuaclayton]

@gylaz @jferris, to address some of your points:

The Ruby language seems to disagree with this guideline. If you attempt to define a private attr_reader, Ruby issues a warning. This seems to imply that Ruby considers attr_reader to be useless unless there's a reference from a separate instance.

There seems to be some disagreement with this: https://bugs.ruby-lang.org/issues/10967

Tangentially related: we promote the use of protected (over private) for attr_*, which doesn't issue a warning.

I can right away tell what they are,

I felt similarly, years ago, about using self. when invoking public instance methods - I knew immediately that it was a public method versus a private or protected method. We as a company agreed a while ago that self. is an unnecessary descriptor; any internal methods using the information do not care about internal visibility. I feel this is similar in that regard.

I don't need to guess "is that method doing something other than returning the variable".

and

When I see an attr_reader reference, it could be anything.

I guess my thought is that, if you're referring to a value, it doesn't matter what it's doing other than returning what it's required to. Referencing an instance variable provides unnecessary specificity that the internals of the object likely don't care about.

[drapergeek]

I guess my thought is that, if you're referring to a value, it doesn't matter what it's doing other than returning what it's required to. Referencing an instance variable provides unnecessary specificity that the internals of the object likely don't care about.

I agree. I've been following this guideline in my projects for the last year and I've really enjoyed it. It makes me less concerned about what something is and instead I worry about the method I want.

I'm 👍 on this.

[gylaz]

I guess my thought is that, if you're referring to a value, it doesn't matter what it's doing other than returning what it's required to.

Eventually, yes. But at first, when I see a customer being used in a given method, I'm naturally curious what that customer is, and where it comes from. Is it an attribute reader for something that was passed in, or is it a private method that does some interesting logic to infer a customer? There's a pause to look that method up, and learn about it. Eventually, I will remember what customer does, or so I hope. On the other hand, if I see @customer, I know it's just an instance var that likely was passed into the initializer.

If we want to follow the spirit of YAGNI, instance variables, or direct variable access, would be what we should go with. Using attribute readers requires more code, for what quantifiable, proved value?

[derekprior]

I'm 👍 on this, but I also don't care that strongly and am not sure we need to have a guideline. We discussed this in a Boston dev discussion eons ago and the room was split 50/50.

Using attribute readers requires more code, for what quantifiable, proved value?

My $0.02 is that they are worth it for typo protection alone. NoMethodError is a lot more helpful than an unexpected null due to a typo you might not notice for quite some time.

I'd love to see everyone use attr_reader but don't think we'll have enough agreement here. Is inconsistency class to class among a project going to be a problem? Will it be a problem if Greg and I are on a project and I turn ivars in an object into attrs while I'm making another change? In practice I haven't seen this come up -- I've been either with like-minded individuals or able to persuade them...

[gylaz]

NoMethodError is a lot more helpful than an unexpected null due to a typo you might not notice for quite some time.

NameError: undefined local variable or method `nmae' for #<Person:0x007fe2d291af70 @name="Greg">

I would argue that the error produced by mistyping the instance variable is just as helpful as NoMethodError.

Will it be a problem if Greg and I are on a project and I turn ivars in an object into attrs while I'm making another change?

When I'm on a project that uses attr_reader and friends, I tend to stick to consistency. I wouldn't be entirely opposed to having a guideline one way or the other, for consistency. However, I remain unconvinced that indirect variable access, as coined by Kent Beck, has clear advantages over direct (i.e. instance vars).

[derekprior]

Huh - is that name error new?

[jmromer]

A quick sanity check from the peanut gallery...

This is Ruby 2.2.1:

class Cat
  attr_reader :name

  def initialize(name)
    @name = name
  end

  def speak
    puts "Hello my name is #{@nmae}"
  end

  def speak_again
    puts "Hello my name is #{nmae}"
  end

  def speak_once_more
    puts "Hello my name is #{self.nmae}"
  end
end

> kitten = Cat.new('garfield')
#<Cat:0x007fff3a169270 @name="garfield">

> kitten.speak
Hello my name is
nil

> kitten.speak_again
NameError: undefined local variable or method `nmae' for #<Cat:0x007fff3a169270 @name="garfield">
        from: (pry):21:in `speak_again'

> kitten.speak_once_more
NoMethodError: undefined method `nmae' for #<Cat:0x007fff399fedc8 @name="garfield">
        from: (pry):39:in `speak_once_more'

[gylaz]

I was wrong, mistyping an instance var results in nil.

[calleluks]

Syntax-wise, calling methods is more flexible than accessing instance variables. When using instance variables, transitioning to using a local, a constant or calling method requires updating every reference to the instance variable. Changing what a method refers to is a one line change.

This is in line with depending on behavior instead of data.

I can't remember feeling the pain of using an instance variable instead of calling a method, though.

[gabebw]

I am in favor.

[mcmire]

I like this too.

[sikachu]

I'm against this rule.

When you are seeing @foo you actually know right away that it's the vanilla instance variable, that there's no other logic hidden under that.

When you see foo, unless you wrote it yourself you probably have to check where it's being defined and which scope it belongs to.

Making a private accessor feels like a layer of abstraction that you don't really need. Writing @foo gives the reader the clear intention that this is an instance variable that is available within this object.

[c-lliope]

I'm in favor, and I've been doing this for a while on all my projects.

I prefer not to mutate my instance variables. @variable is assignable, attr_reader :variable is not. Avoiding @variables gives me confidence I'm not mutating anything accidentally.

Also, the errors for typos are great.

[jferris]

Avoiding @variables gives me confidence I'm not mutating anything accidentally.

It's good to remember that attr_reader is vulnerable to the same mistakes that freeze is, like my_attr_reader[:key].downcase!. If you don't freeze, it also allows direct mutation like my_attr_reader << :wowzers.

Using attr_reader prevents re-assignment (changing identity), but not mutation (changing state).

Also, the errors for typos are great.

This is probably the best argument for attr_reader I've heard. That nil NoMethodError thing you get when you typo an instance variable (or forget to extract one when extracting a class) is super confusing.

[jakecraige]

Also, the errors for typos are great.

This is why I'm 👍 this.

[nickrivadeneira]

I am 👍 on this because I find it easier to determine where the assignment occurs (assuming you're using an attr_reader):

class Foo
  attr_reader :bar

  def one
    bar.one
  end

  def two
    @bar = Two.new
  end

  def three
    bar.three + 8998
  end
end

vs.

class Foo
  def one
    @bar.one
  end

  def two
    @bar = Two.new
  end

  def three
    @bar.three + 8998
  end
end

That's not a great example, but I recently ran into a situation where an ivar was called from a controller action, and it was difficult to tell where it was assigned. In order to find where it had been assigned, I had to read through every method that was called prior. It turns out it was assigned in a before_filter. If we were using method access, I'd just be looking at method names.

[croaky]

+1 for "typo returns NoMethodError" reason

[sikachu]

Given how the tide turns on this PR, I think I'm okay of being overruled.

Note that you have to wait until Ruby 2.3 to be out before you could use private attr_accessor though.

[teoljungberg]

This PR has been open for a staggering amount of time. Is there anything we can do to reach a consensus, or have it been reached?

[gylaz]

How about no guideline, and let the project members decide?

[jferris]

I have created a poll to settle this matter: https://docs.google.com/a/thoughtbot.com/forms/d/1P5u_6tbN18Phl8bKo0Bh0EM5wmPIzT-j805zIn0xYFQ/viewform

[mjankowski]

@jferris did your poll reveal compelling new data?

@joshuaclayton how are you feeling on this after so long?

[joshuaclayton]

I still feel strongly that this is the right route to go down:

• easier to catch NoMethodError than handling mistyped ivars
• consistent level of abstraction means the source of information and its access level of data isn't necessary to be used correct (ivar, method, public/private/protected plays no part in retrieving data)

Looks like 2/3 of the responses (out of 25 respondents) prefer this as a guideline.

[jmromer]

WWSMD?

[jferris]

There were no votes for using @variable directly, and most people were in favor of having this guideline.

Also, the private/protected issue discussed earlier in the pull request has been resolved in Ruby 2.3, so Ruby itself no longer has any opinion here.

I think this is good to merge.