Prefer instance var over attr_* methods inside class

[adsteel]

It is easier to understand and slightly faster to directly access the instance variable inside the class.

# bad
class Person
  attr_reader :age

  def initialize(age)
    @age = age
  end

  def half_age
    age * 0.5
  end
end

# good
class Person
  attr_reader :age

  def initialize(age)
    @age = age
  end

  def half_age
    @age * 0.5
  end
end

The other side of that is, if the accessor is explicitly defined, that custom accessor should be preferred over directly using the instance variable.

# bad
class Person
  def initialize(age)
    @age = age
  end

  def age=(new_age)
    fail 'Can only get older' if @age > new_age

    @age = new_age
  end

  def celebrate_birthday
    @age = @age + 1
  end
end

# good
class Person
  def initialize(age)
    @age = age
  end

  def age=(new_age)
    fail 'Can only get older' if @age > new_age

    @age = new_age
  end

  def celebrate_birthday
    age = @age + 1
  end
end

[jdickey]

I'd argue against this. If you start out using an instance variable and then later change to a custom accessor, you now must go and replace every use of the ivar with the accessor. Risk of breakage increases as a power of the number of changes made, so this could simply inhibit needed refactoring later.

EDIT: How much faster is "slightly faster"? If whether this change is made makes a difference that is detectable to the user, then your app is already amazingly performant or mind-numbingly trivial. Lower cost of maintenance should always trump excessive optimisation of initial implementation — a lesson which many of us take decades to fully appreciate. 😞 Also, is "slightly faster" true across all versions of all Ruby interpreters, or merely a specific version of MRI?

[adsteel]

@jdickey That's a really good argument against.

[equivalent]

+1 on jdickey's argument

named variables are essential in order for better re-factoring.

class Foo
  def call
    user_ids = [1,2,3,4]
    user_ids.each { |id| puts id }
  end
end

# vs 

class Bar
  def call
    @user_ids = [1,2,3,4]
    @user_ids.each { |id| puts id }
  end
end

to

class Foo # or Bar
  def call
    user_ids.each { |id| puts id }
  end

  def user_ids
    [1,2,3,4]
  end
end

[adsteel]

@jdickey @equivalent Would it be worth saying that all instance variables should be accessed with accessor methods? This would make for a consistent style, as well as easily allow for custom accessors if needed.

[equivalent]

@adsteel well yes and no. it really depends what do you want to achieve.

I honestly never touch instance variable in a class I always use attr_reader.

class Foo
  attr_reader :foo

  def initialize(foo)
    @foo = foo
  end

  def do_stuff
    foo + 'bar'
  end
end

But there are cases when you want to make the values private and here where controversy starts. I prefer using private attr_reader like this:

class Foo
  def initialize(foo)
    @foo = foo
  end

  def do_stuff
    foo + 'bar'
  end

  private

  attr_reader :foo
end

Foo.new('hi').do_stuff  # 'hibar'

but lot of Ruby folks would complain that this may be actually against the guide as accessors should be at the top and therefore just doing this is better

  def do_stuff
    @foo + 'bar'
  end

And even if you convince the community you will have situations like this

class Foo
   attr_writer :foo

  def do_stuff
    foo + 'bar'
  end

  private

  attr_reader :foo
end

Foo.new.tap { |f| f.foo = 'hi' }.do_stuff  # 'hibar'

So this is though sell, I would say "yes, everything should be accessed via accessors or readers or writers "but there might be fight against this as simple classes loosk stupid, but to be honest when you have large class it makes sence:

class Foo
  attr_writer :bar, :foo
  attr_accessor :user

  def do_stuff
     foo + bar + long_name + some_other_stuff
  end

  private
    attr_reader :bar, :foo
    delegate :long_name, to: :user

    def some_other_stuff
       @some_other_stuff ||= user.some_complex_calculation_of_string
    end
end

I welcome the feedback on this

[jonahb]

If you'd like all your accessors at the top, you can do:

class Foo
  private

  attr_reader :foo

  public

  def do_stuff
  end
end

Or:

class Foo
  attr_reader :foo
  private :foo

  def do_stuff
  end
end

[equivalent]

👍 on the second case of private :foo :)

update:

I've just remembered, the first example don't comply with Scissors Rule http://www.eq8.eu/blogs/16-scissors-rule-in-coding (my favourite topic whit which I annoy people :) ) Basically the rule of thumb is (use to be) to have interface methods on top as if you would "cut the printed code with scissors" you will end up only with what you can use as an interface. discussion 1 discussion 2

[jdickey]

👍 with @equivalent on the second case of @jonahb's private :foo. Incidentally, this convention lets you define multiple attr_readers or whatnot and then declare them all as private:

class Foo < Magic::Base
  attr_accessor :bar
  attr_reader :foo
  magic_thingie :baz
  private :bar, :baz, :foo

  def do_stuff
    # Magic!
  end
end

This reflects our existing shop practice in three respects:

1. the ordering of attr_accessor, attr_reader and attr_writer, followed by DSLish declarations, at the top of the class; and
2. enumeration of multiple identifiers must be in alphabetic order (in the private declaration)
3. The first method in a class (excluding inner modules/classes) is always public without requiring explicit declaration as such.

[besen]

Using attr_accessor this way requires one more step (private :bar=), unless you really want to expose the setter.

2.1.5 :001 > class Foo
2.1.5 :002?>   attr_accessor :bar
2.1.5 :003?>   private :bar
2.1.5 :004?>   end
 => Foo 
2.1.5 :006 > foo = Foo.new
 => #<Foo:0x000000032fdf20> 
2.1.5 :007 > foo.bar
NoMethodError: private method `bar' called for #<Foo:0x000000032fdf20>
        from (irb):7
        from /home/besen/.rvm/rubies/ruby-2.1.5/bin/irb:11:in `<main>'
2.1.5 :008 > foo.bar= 'nope'
 => "nope" 

[anujbiyani]

Is there a cop to enforce the opposite of this? (e.g. prefer attr_* over accessing the instance variable directly)

I couldn't find one in the list of style cops, but I might have missed it.

[giner]

I'd vote for never using attr accessors within the class. Linking worth reading discussion in the topic https://forum.upcase.com/t/using-instance-variables-vs-attribute-accessors/1788/16.

[jdickey]

It took several reads to convince myself that Greg’s arguments in the discussion @giner linked to is not intended as satire. I wonder how large/old the biggest/oldest project he’s ever had to work on is?

Point: “I know right away that I’m using the instance variable @item as opposed to guessing wether item method is doing something extra.”
Counterpoint: You shouldn’t care, as far as attr_readers go. Why that is is covered in the next point but, basically, you’re hard-coding an assumption about How Things Work that you know is subject to change. Use the accessor method.

Point: “The argument is that readers are easier to re-factor is YAGNI, in my opinion. I’ve hardly ever seen that strategy being taken advantage of.”
Counterpoint: See the well-known trope about opinions. Just because you’ve “hardly ever seen that strategy being taken advantage of” doesn’t imply either that your experience is representative of a sufficiently large subset of the community to be valid as a RuboCop rule or that you haven’t quickly read through code and not realised that it was using accessors. That happens to the best of us and is not meant as a personal jibe.

Point: “As for consistency, this is the same as our stance on single vs double quotes for strings. The argument there is that if you see double quotes you know something interesting is happening (e.g. interpolation, special characters). Say, for consistency, we decided to use instance variables, then if I encounter an item method, I know that something interesting is happening, and it’s not just a getter for the instance variable and I should probably go and look at that method. If we always used readers then it’s very easy to assume they all are just readers, and not a method that does something extra.”
Counterpoint: No, it’s quite different. Interpolation is not allowed within single quotes; it is within double quotes; that rule is defined by the language, not by RuboCop. Thinking of accssors-vs-instance-variables as equivalent may well be a failure to fully appreciate the relative implications.

Point: “ Defining a reader requires more typing.”
Counterpoint: People completely, and clearly deliberately, misunderstand Larry Wall’s famous “programmers should be lazy” quote. He wasn’t talking so much about physical sloth as described in this point (and which he has vehemently stomped on since); he’s pointing out that if you do some boring, repetitive task 20x a week and it takes 10 minutes each time, then investing 4 hours in automating that will save you time and effort the second week that your automation is in use. That’s “good” laziness; sloth is bad laziness. Know the difference.

Sorry for the slow response; I’ve been sick for most of the last 3 months and have checked GitHub notifications sporadically at best.

I still stand by my comment from 3-1/2 years ago, and am absolutely flabbergasted that this issue has yet to be shot down in the flames it so richly deserves.

[radarek]

I'm a big fan of using attributes accessors methods but I don't think they should be enforced by Rubocop.

Speaking of attibutes accessors I created feature request in Ruby tracker so that we could define visibility for attibutes like this:

class Foo
  private attr_accessor :foo, :bar
end

Also I made a draft PR for that: ruby/ruby#3757.

[pirj]

shot down in the flames

Done.

[ddnexus]

A simple rule: use instance variables when they are available (i.e. in the same class or subclass), and use the accessors when you really need them to access the instance variables from an instance.

A couple of points:

• If a class defines an instance variable it is a core part of it and the class should know what is doing with it. Its tests should avoid typos and if you need to refactor, any decent IDE will be able to rename all its occurrences in the code in a single command and any basic text editor will be able to do a search and replace. You will get the best possible performance avoiding accessors that have no real advantages in that context.
• For the instance variables that have to be exposed, the accessors will nicely provide the method to access them from the instance. In that case you will pay a small price in performance only when it is needed and justified by the functionality.