2

I'm building a system that uses a lot of small classes to do data processing (each class does a different processing step, nice & SRP like). I initialize each class with some parameters then call an execute() method on the class, which then uses other private methods to do some of the data calculations.

My question is: if I'm not accessing any of the parameters outside of the class, should I just use the instance variables in the private methods? Or define an attr_reader?

For example, here's a bit of contrived code that I whipped up:

class Car
  attr_reader :make

  def initialize(make, model)
    @make = make
    @model = model
  end

  def execute
    apply_paint
    rotate_tires
  end

  private

  def apply_paint
    if make == "Toyota"
      Painter.paint(self, "red")
    else
      Painter.paint(self, "green")
    end
  end

  def rotate_tires
    if @model == "Sequoia"
      set_tire_size(:large)
    else
      set_tire_size(:normal)
    end
  end
end

So which is better? How I handled "make" or how I handled "model"?

If I know that I'm not using either of those variables outside of this class at all, should I just stick with instance variables? Or is it nicer to use the attr_reader because then I could convert them into actual methods without changing the rest of the code... i.e. since apply_paint uses make instead of @make, I could change make to be a local method if need be and not change the apply_paint method.

I keep waffling back-n-forth and can't figure out which is better. Maybe this is just a preference thing, but I wanted to see what thoughts others had on this issue.

Dan Sharp
  • 1,209
  • 2
  • 12
  • 31
  • It doesn't cost you anything to use `attr_reader` and it might be quite beneficial in the future, as you observed. So how is this even a question? – Sergio Tulentsev Oct 31 '15 at 22:17
  • It does cost me: It's extra code, it exposes an accessor (unless I make it private) and it means that every time I see "make" in the code, I have to remember that it's an accessor (and not a local variable or other method), as opposed to "@make" which is clearly an instance variable. So I think it is a good question, as there are clearly tradeoffs either way. – Dan Sharp Oct 31 '15 at 22:27
  • Interesting, we had almost the same question just 9 hours ago on this site: http://stackoverflow.com/a/33452093/2483313 – spickermann Oct 31 '15 at 22:37
  • @DanSharp: "I have to remember that it's an accessor" - do you, though? If your methods are short (which they should be), it will be immediately obvious whether this is a parameter/local or not (in which case it's some kind of a method and you don't care which kind exactly). – Sergio Tulentsev Oct 31 '15 at 22:47
  • You can also use private attr_ methods. That way if it needs to be readonly inside of the class, you can do that without exposing the variable to the outside world. – the happy mamba Nov 01 '15 at 06:30
  • More interesting comments, from Avdi Grimm: https://bugs.ruby-lang.org/issues/9453#note-4 – Dan Sharp Nov 05 '15 at 17:36

1 Answers1

3

Use attr_ methods. You can (and probably should) make them private if you're not accessing them outside of the class. Why use them? You improve readability by stating your intentions towards how it should be used. An attr_reader should be interpreted as "this variable should only be read".

baron816
  • 701
  • 4
  • 13