Using instance variables vs attribute accessors

Ruby on Rails
1

When you are accessing private, internal state of your object, do you prefer to use attribute accessors (getter and setter methods) or direct instance variable access? Consider this contrived example:

class LineItem
  def initialize(item, quantity)
    @item = item
    @quantity = quantity
  end

  # Access our internal state via instance variables
  def total
    @item.price * @quantity
  end

  # Or via attr_readers
  def total
    item.price * quantity
  end

  #... which would require something like this:
  private

  attr_reader :item, :quantity
end

Which would you prefer? In the simplest cases, the two are functionally equivalent, so does it matter? Is there a point where your preference for one over the other changes? What are the pros and cons of each approach?

We’ll be discussing this at the thoughtbot Boston developer discussion tomorrow afternoon and will post our notes and any follow-up here. Please feel free to contribute your thoughts here at any time.

2 Likes
2

I prefer the latter, as it explicitly flags those attributes as read-only. Nothing’s worse than accidentally typing = when you meant to type ==!

3 Likes
3

I prefer using attr_reader or wrapping my instance variables in methods for a couple reasons.

  • I like the idea of sending messages rather than directly accessing data, even if it’s internal.
  • If I run into a change, wrapping instance variables allow changes to happen a little more easily.
  • This is not a reason per se, but I also just think using methods instead of instance variables is just a little more aesthetically pleasing.
2 Likes
4

I’m somewhat on the instance variable side of things.

Instance variables:

###Pros:

  • I like being able to tell at a glance when I’m dealing with state on the object.
  • Does not require writing redundant methods

###Cons:

  • If a property becomes computed or delegated to another object, you may need to change several places in the file.

Private Accessors

###Pros:

  • Can be easier to change
    All of these would work without having to change anything else in the file
private
def item
   @order.items.first
end

# OR
def item
  @item
end

# OR
def item
  Item.new(@sku, @cost)
end

###Cons:

  • Private accessors definitions often come as a surprise and are easily missed when reading a class
  • Ruby will also give you a warning if you create a private accessor.
  • I don’t like the use of class-level macros in the private api

All these cons can be resolved by explicitly writing readers:

private
attr_reader :item

# VS

private
def item
  @item
end

This is much wordier though I like the explicitness and the ease of change.

5

I use private accessor all the time. By sending a message instead of using data directly, that’s one less coupling you have, for now the data structure can change (for instance an hash keys, or an hash to an array) and you only have to rewrite the accessor and not all the methods that use it.

For me that’s a big plus.

6

In our discussion today we were originally evenly split between people that generally use attribute accessors for private state and those that use instance variables directly.

** Attribute Accessors: **

  • PRO: Uniform access. Refactor to the method (add memoization, etc) is done in a single place.
  • PRO: Typo protection. Typos of a method give a more obvious error (and are guaranteed to error). Typos of an instance variable don’t error. You may get a test failure, but the error won’t be as obvious.
  • PRO: Accidental assignment is more difficult (only possibly if you also have a private attr_writer as well).
  • CON: Using attr_reader in private scope warns when running ruby with warnings (ruby -w). Can either live with it (who runs with -w, anyway?), write the reader by hand, or use attr_extras.

** Instance Variables: **

  • PRO: Makes state dependency obvious at a glance (syntax highlighting)
  • PRO: You know immediately that accessing an instance variable is not expensive (no db read, etc).
  • CON: If you need to introduce a reader at a later date, the refactor is simple, but much wider (file, possibly any included modules)

We spent some time wondering why it is that ruby would opt to silently return nil when a previously undefined attribute is accessed. If a warning or error was raised you’d at least be alerted to typos.

We also discussed the pros and cons of the attr_* macros. These really apply to all uses - be they private or public. That is, attr_* yield methods that are difficult to grep for and are not understood by ctags.

As a group, we couldn’t come to a consensus on which practice should be recommended. No one was particularly swayed from their original position, though one person did say they would probably give attribute readers a try.

The one thing we definitely did agree on was that consistency should be valued above all in this case. Don’t use an attribute reader for some private state but instance variables for other private state in the same class. Be consisten in a class and try to be consistent across a project if that’s feasible.

3 Likes
7

One wrinkle we didn’t discuss as it relates to the point of consistency
 Let’s say I’ve been consistently using instance variable access for private state in my class but then need to refactor to a private getter for one bit of state. The consistency guideline we all thought was a good idea now dictates that we change the access method of any other private state as well. Is this really what we want?

To me, this is a further argument for attribute readers at all times (the camp I was originally in and remain in). What do you guys think?

8

Could you give a small code example of an instance variable that would need to be refactored to a private getter?

I tend to use instance variables because I like seeing when I’m accessing an object’s state rather than calling a method. As for the refactoring argument, in JoĂ«l’s example above, if the item method had to be changed to create a new item or return the first item from an order object, I would prefer to change the name of that method to something more descriptive and refactor anyway.

9

@joannecheng What if an item is a hash that the class receives on instanciation, and for some reason it does not know or care about, one of the hash’s key gets renamed. You’ll have to go through all the methods of the class and change it, because you have a coupling to the data structure.

It’s like a small sized shotgun surgery smell.

10

Consider some fictional payment processor that we interface with that to charge or refund credit cards. We’ve wrapped a class around this to isolate that external dependency:

class Payment
  def initialize(card, amount)
    @card = card
    @amount = amount
  end

  def charge
    Processor.charge(@card, @amount)
  end

  def refund
    Processor.refund(@card, @amount)
  end
end

The interface changes one day to support charging in various currencies. It now requires that you pass a Money object so it knows what currency you’re charging in. But for our case, this is always USD and we don’t want to burden the rest of our app with having to pass amounts around as something other than an integer. So


class Payment
  CURRENCY = 'USD'

  def initialize(card, amount)
    @card = card
    @amount = amount
  end

  def charge
    Processor.charge(card, amount)
  end

  def refund
    Processor.refund(card, amount)
  end

  private

  attr_reader :card  
  
  def amount
    Money.new(@amount, CURRENCY)
  end
end

You could create a new method amount_as_money and use that in charge and refund. Had you been using attr_readers all along you’d simply change attr_reader :amount to the method definition and you’d be done. In this case you could also do the conversion in the initializer, but there are times when this isn’t feasible.

11

I think wrapping instance variables is simply a micro-level example of depending upon abstractions. In the above example, we probably wouldn’t directly access a Stripe class, but instead access PaymentProcessor class. If we treat the internal data or state like an object rather than data, our system becomes a little more flexible.

Even though I prefer this method, I’m not really sure if the wins are big enough either way.

12

Thanks for the example, @derekprior. I can see how this would break consistency across a project.

@zamith If I were accessing the value of a hash via key more than twice in my class, I would have created a private method to do so. It’s different from creating a private getter method.

13

@joannecheng Point taken. I guess that if you create a private method anytime your data is more complex than an integer, string, etc, you’ll probably get the same benefits of having the reader on the first place.

You might argue that you’d name it better, but you’d also have to be very disciplined with work refactor, so that no data coupling creeps in to the class.

Maybe having private reader for everything is using a sledgehammer on a nail, but it does get the job done for me.

Another benefit is that you’re sending messages all the time which is nice in an OO environment.

14

On the private accessors, this is a nice tip I got from henrik on the parley.rubyrouges.com forum

Essentially, you don’t have to put this at the bottom:

private 
   attr_reader :foo, :bar

Instead, you can put this anywhere:

   private_attr :foo, :bar

Here’s the small implementation:

class Class
  def private_attr(*attrs)
    attr_reader *attrs
    private *attrs
  end
end

There’s more in this: attr_extras.

15

I’m in the camp of using instance variables.

  • I know right away that I’m using the instance variable @item as opposed to guessing wether item method is doing something extra.
  • 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.
  • 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.
  • Defining a reader requires more typing.
1 Like
16

I’d agree with Greg, primarily for the reason that it’s very easy to grok unfamiliar code using the @instance_variable syntax and then knowing that I’d want to consider that a method is probably doing something, if just memoizing the value.