Don’t wrap instance variables in attr_reader unless necessary

by Jason Swett,

I’ve occasionally come across advice to wrap instance variables in attr_reader. There are two supposed benefits of this practice, which I’ll describe shortly. First I’ll provide a piece of example code.

Example of wrapping instance variables in attr_reader

Original version

Here’s a tiny class that has just one instance variable, @name. You can see that @name is used in the loud_name method.

class User
  def initialize(name)
    @name = name
  end

  def loud_name
    "#{@name.upcase}!!!"
  end
end

attr_reader version

Here’s that same class with an attr_reader added. Notice how loud_name now references name rather than @name.

class User
  attr_reader :name

  def initialize(name)
    @name = name
  end

  def loud_name
    "#{name.upcase}!!!"
  end
end

The purported benefits

Advocates of this technique seem to find two certain benefits in it. (I’ve also come across other supported benefits but I don’t find them strong enough to merit mentioning.)

It makes refactoring easier

Rationale: If you ever want to change @name from being defined by an instance variable to being defined by a method, then you don’t have to go changing all the instances of @name to name.

This reasoning isn’t wrong but it is weak. First, it’s very rare that a value will start its life as an instance variable and then at some later point need to change to a method. This has happened to me so few times that I can’t recall a single instance of it happening.

Second, the refactoring work that the attr_reader saves is only a trivial amount of work. The cost of skipping the attr_reader is that you have to e.g. change a handful of instances of @name, in one file, to name. Considering that this is a tiny amount of work and that it needs to happen perhaps once every couple years per developer, this justification seems very weak.

It saves you from typo failures

Rationale: If you’re using instance variables and you accidentally type @nme instead of @name, @nme will just return nil rather than raising an error. If you’re using attr_reader and you accidentally type nme instead of name, nme will in fact raise an error.

This justification is also true, but also weak. If typo-ing an instance variable allows a bug to silently enter your application, then your application is not tested well enough.

I would be in favor of saving myself from the typo problem if the attr_reader technique hardly cost anything to use, but as we’ll see shortly, the attr_reader technique’s cost is too high to justify its benefit. Since the benefit is so tiny, the cost would have to be almost nothing, which it’s not.

Reasons why the attr_reader technique is a bad idea

Adding a public attr_reader throws away the benefits of encapsulation

Private instance variables are useful for the same reason as private methods: because you know they’re not depended on by outside clients.

If I have a class that has an instance variable called @price, I know that I can rename that instance variable to @cost or change it to @price_cents (changing the whole meaning of the value) or even kill @price altogether. What I want to do with @price is 100% my business. This is great.

But if I add attr_reader :price to my class, my class suddenly has responsibilities. I can no longer be sure that the class where @price is defined is the only thing that depends on @price. Other clients throughout my application may be referring to @price. I’m no longer free to do away with @price or change its meaning. This makes my code riskier and harder to change.

You can add a private attr_reader, but that’s unnatural

If you want to make use of the attr_reader technique but you don’t want to throw away the benefits of encapsulation, you can add a private attr_reader. Here’s what that would look like.

# attr_reader version

class User
  def initialize(name)
    @name = name
  end

  def loud_name
    "#{name.upcase}!!!"
  end

  private

  attr_reader :name
end

This solves the encapsulation problem, but what have we really gained on balance? In exchange for not having to change @name to name on the off chance that we change name from an instance variable to a method, we have to pay the price of having this weird private attr_reader :name thing at the bottom of our class.

And consider that we would have to do this on every single class that has at least one instance variable!

Don’t do wrap instance variables in attr_reader

Wrapping your instance variables in a public attr_reader changes your instance variables from private to public, increasing the public surface area of your class’s API and making your application a little bit harder to understand.

Wrapping your instance variables in a private attr_reader adds an unnatural piece of boilerplate to all your Ruby classes.

Given the tiny and dubious benefits that the attr_reader technique provides, this cost isn’t worth it.

Using attr_reader is good and necessary for values that really need to be public. As a default policy, wrapping instance variables in attr_reader is a bad idea.

23 thoughts on “Don’t wrap instance variables in attr_reader unless necessary

  1. Josh Cheek

    In this example, the original code is fine (well… in this example, I wouldn’t even bother with a class), but if you have more complex classes, then the private `attr_reader` doesn’t require you to make the class 50% longer and you probably already have a private section, so it’s just a matter of where you put the `attr_reader`. As it gets more complex, there will be more vars, more methods, etc, and you’ll be doing any amount of renaming. Tests should still catch misspellings, but that doesn’t mean that the error will be right at the place that it occurs. Eg if you extract a bunch of logic into a class and pass the ivar to the class, but misspell it, then your tests will observe that something blows up down in the depths of that other class, and you’ll have to debug it back to that ivar being `nil` b/c of the misspelling.

    Anyway, point being, for something this simple, yeah, go with ivars, but as you get more code / more vars / more complexity, start preferring private `attr_accessor` (then you can use the setter and catch misspellings when setting and when getting)

    Reply
      1. Tim C

        I appreciate the logic in your arguments and think that’s fair enough. It’s made me reflect on why I mostly use private attr_readers – I think its the encapsulation angle, and a habit I developed after a watching a talk by ( I _think_) Sandi Metz.

        I’m interested in your thoughts on the barsoom/attrr_extras gem as a shorthand? I heard Ben Orenstein pick it on the Rogues podcast a few years back and I have been using it ever since. Its very concise – your example becomes this (which some will say is the work of devil but I really like):

        class User
        pattr_initialize :name

        def loud_name
        “#{name.upcase}!!!”
        end
        end

        Reply
        1. Jason Swett Post author

          Interesting. I had not heard of attr_extras before now. On one hand I like what it does. On the other hand I feel weird about mixing in a third-party dependency into something so common and fundamental. I guess I’d say I 20% like it and 80% don’t like it.

          Reply
  2. Gareth

    100% agree that public attr_readers are (as a rule of thumb) bad because they break encapsulation.

    There are benefits to using them though.

    Error handling of methods is better, since instance variables default to nil. When you call an undefined method a NameError is raised, making it much easier to figure out that you’ve made a mistake.

    Another reason is during equality comparison. If you want to define == then fairly often you’ll want to compare that the encapsulated objects are equal, so being able to do something like name == other.name is nice.

    For these reasons, I tend to make my attr_readers protected so that they’re callable by instances of the same class, but nothing else. I’ve written a bit more about this at https://www.garethrees.co.uk/2020/06/14/use-protected-attr-readers/

    Reply
  3. Robert Fletcher

    It seems like you are arguing more from an academic perspective and less a practical one. In more than a decade of writing Ruby and Rails code, I don’t think I’ve ever seen a case where public accessors have come back to bite someone in application code. For shared library code it might make sense to be more careful, but even then I’d probably err on the side of private attr accessors.

    You mentioned the benefits, but you’re kind of dismissive of them.

    > it’s very rare that a value will start its life as an instance variable and then at some later point need to change to a method

    In my case it frequently happens in my code that something starts as an attr accessor or an instance variable and wants to become a method. These kinds of refactorings unnecessarily bloat the diff when switching from an instance variable.

    > If typo-ing an instance variable allows a bug to silently enter your application, then your application is not tested well enough.

    Maybe, but I’d rather have the extra layer of protection. Being human, it’s common for me not to get everything right the first time, including my tests. I try to use practices that reduce the cost of my human fallibility. When I typo an instance variable, the error can often be far from the source of the issue. Even when I’m writing the tests this can be frustrating and confusing. Having a mis-typed method name throws an error right at the moment it is called.

    > You can add a private attr_reader, but that’s unnatural

    This is a subjective assessment, and doesn’t provide any concrete rationale. Just like any other practice, if you get used to it, it won’t seem so unnatural anymore.

    Quality code is often not a matter of huge wins, but small wins that add up over time. The more I can integrate small practices that help me avoid getting tripped up, the more I am able to get code out the door that works right the first time.

    Reply
    1. Jason Swett Post author

      Thanks, there’s actually very little of this that I would disagree with.

      I would argue though that I am in fact coming at it from a practical perspective. The specific cost of a public attr_reader is that when I see attributes that are made public by attr_reader, I can’t easily know if it’s safe for me to refactor those attributes. This isn’t an academic concern as it has happened to me. The fact that it has recently happened to me multiple times is what prompted me to write this post.

      Reply
      1. Robert Fletcher

        In that case I might throw your own argument back at you: “your application is not tested well enough.” Generally speaking, in the scenario you describe I would make the change I want to make and see what tests fail. With a well tested codebase, I’ve found it makes these sorts of refactorings super easy. We recently replaced an entire database table using this method. It’s a strategy I use frequently to rename things across the codebase (as well as some grep-fu to track down loose references in the views).

        Reply
    2. Rob Nichols

      I thoroughly agree with Robert Fletcher’s arguments. I’d use attr_reader over using that instance variable directly every time. It’s much cleaner, and the misspelling issue is big for me and not one that can be lightly dismissed with – your tests should deal with that.

      Reply
  4. Adrien Rey-Jarthon

    Good points, there’s also the performance argument, `attr_reader` adds a method call which has some overhead:

    “`ruby
    require “benchmark”

    REP = 10_000_000

    class Foo
    attr_reader :bar
    def initialize; @bar = 99 end
    def get_var; @bar; @bar; @bar; @bar; @bar end
    def get_acc; bar; bar; bar; bar; bar end
    end

    f = Foo.new

    Benchmark.bm do |x|
    x.report(“variable”) { REP.times { f.get_var } }
    x.report(“accessor”) { REP.times { f.get_acc } }
    end
    # user system total real
    # variable 0.352928 0.000520 0.353448 ( 0.353455)
    # accessor 0.645047 0.000031 0.645078 ( 0.645110)
    “`
    If it’s in a hot path this could make a sizeable difference.

    Reply
    1. Jason Swett Post author

      Thanks. I knew about the performance argument but chose not to include it because, frankly, I don’t think it’s that strong of an argument. I don’t disagree that it could theoretically become an issue in certain circumstances, it just seems unlikely to enter the picture in 99%+ of cases.

      Reply
  5. David A. Black

    Thanks for sharing your interesting thoughts on this. Let me offer a slightly different take. I agree that instance variables are private in the sense that, by definition, only ‘self’ can refer to them (other than ivar_set and ivar_get but never mind those). The thing is, I don’t think that wrapping them in a reader method means that they are no longer “private” in the specific way that they were “private” to start with. They still cannot be referenced by a different ‘self’.

    I see the attr_* methods simply as code-writing shortcuts. The question, then, is in my view not whether one should use attr_reader, but how one wants to handle per-object state. def x; @x; end is really just a special case of def x; [do something with @x, maybe memoize a calculation, whatever, log a message, etc….]; end. State is certainly a vexed topic in certain ways, but I don’t think attr_reader per se is a particular culprit.

    One other note: you can always do this instead of putting everything at the bottom:

    attr_reader :x, :y, :z
    private :x, :y, :z

    Reply
    1. Jason Swett Post author

      Hi David, thanks. I think I’m confused about one thing – is it not true that if you do e.g. attr_reader 😡 (no private) then x becomes a public attribute of that class?

      The other stuff makes sense to me. I didn’t know you could do the private :x, :y, :z technique.

      Reply
  6. Will Howard

    I also prefer the private attr_reader approach for many of the reasons above which I won’t echo for brevity.

    I want to add another reason I like the use of attr_reader however and that is that when you use this approach you never end up using an instance variable literal in your code unless you are assigning a value to it.

    As someone who reads a lot of code I appreciate that as it causes me to draw attention to these parts of the code as they are very important parts of the class and deserve the added attention.

    Reply
  7. Adrián Mugnolo

    The article is confused about object orientation.

    OO is about messages between objects, i.e. the methods called, not instance variables. Objects are not structs. Other uses of a class, through inheritance, extension, refinement, etc., should not care if a value comes from a variable, a computation, a database query or a remote API request.

    The article says that changing an instance variable happens “perhaps once every couple years.” But, say that the `User` class above needed a first name and last name:

    “`
    class User
    def initialize(first_name, last_name)
    @first_name = first_name
    @last_name = last_name
    end
    end
    “`

    Too bad, it just happened again.

    Without a `#name` accessor, we would need to know about all the class uses in the app: inheritance, mix-ins, etc. Search and replace if we owned the code or introduce a redundant `@name` instance variable.

    The fact that the language designers made methods and local variables interchangeable and easy to type in Ruby, but used a “noise” prefix, for instance variables and globals, could make one think something else is missing.

    My experience is the opposite. When I choose not to define an accessor, the few times are those not meant for an extension, if the requirement can’t change, where performance is an issue, etc. The code that comes to mind is not application code but test classes, request processing, view rendering, libraries, etc.

    Calling arguments strong or weak based on preference alone can lead to wrong conclusions.

    Reply
    1. Jason Swett Post author

      “Other uses of a class, through inheritance, extension, refinement, etc., should not care if a value comes from a variable, a computation, a database query or a remote API request.”

      I assume by “other uses of a class” you mean clients of the class. If so I would totally agree. I’m not talking about clients though, I’m talking about the class itself.

      Reply
  8. Adrián Mugnolo

    Uses of a class are not limited to clients creating instances and calling class methods. Inheritance, extension, composition, delegation, decoration, etc., also depend on a class.

    Take modules, for example:

    “`
    module TagHelper
    def name_tag
    “#{@name}”
    end
    end

    class User
    include TagHelper
    end
    “`

    If we wanted to reuse that module for other objects with a “name,” we would need to contemplate “name” as a method or an instance variable.

    Public and private interfaces are made of methods. Ivars, their names, format, type, etc., are an implementation detail that can, and will, change over time.

    Reply
  9. Adrián Mugnolo

    To your question:

    >I assume by “other uses of a class” you mean clients of the class. If so I would totally agree. I’m not talking about clients though, I’m talking about the class itself.

    The class itself shouldn’t care either unless the code is trivial. You don’t want to be caught using ivars in a 100-line class with inheritance and a few modules.

    Reply

Leave a Reply

Your email address will not be published. Required fields are marked *