Is it better practice to use an attr_accessor here or not?

Question

I have a class QuestionList that stores a list of 'Question' objects.

class QuestionList

    attr_accessor :questions

    def initialize
        @questions = []
    end

end

I then add questions to the list and then "ask" these questions from my main class as so:

list = QuestionList.new
list.questions << Question.new(5, 3)
list.questions << Question.new(1, 5)
list.questions << Question.new(2, 4)

list.questions.each do |question|
    puts "#{question.ask}"
end

where Question.ask simply outputs the question as a string.

I'm not sure how acceptable it is to be writing to an instance variable from my main class using the << operator, and list.questions.push(Question.new(5, 3)) is even more unclear from the main class.

Would it be better to have a QuestionsList.add_question(question) method?

The same goes for list.questions.each - is this acceptable to be used in the main class?

score 1 · Accepted Answer · edited May 23 '17 at 12:24

I think your use of attr_accessor here is fine, though depending on how much functionality you continue to add, it will likely be clearer to restrict functionality of that class to the class itself.

Regarding your question on using a method within QuestionList, this would come down to readability. Something to note first, however: you used QuestionsList.add_question(question) as an example. This would create a class method. What you really want here is an instance method, which would read as list.add_question(question), since you already have an instance of the list created. This blog post has some good information on the difference between class and instance methods.

I personally find instance methods would most clearly communicate your intent. I would write out QuestionList as follows:

class QuestionList

  def initialize
    @questions = []
  end

  def add_question(question)
    @questions << question
  end

  def print_all_questions_in_list
    @questions.each do |question|
      puts "#{question.ask}"
    end
  end

end

This SO post has some excellent information on Ruby's attr methods, if you'd like additional info there.

score 1 · Answer 2 · answered Nov 15 '16 at 18:02

The @questions array is a private internal implementation detail of your class. It should never ever get exposed to clients. There are many reasons for this, two examples are:

You overpromise an interface: now, all your clients depend on it being an array. What if you later want to change it to something else? A textfile? A database? A webservice?
You expose operations that break your object invariants: a client can, for example, add an integer to that array. Or nil. Or anything else that is not a Question.

How you store your questions should be an implementation detail. The QuestionList should have methods to manage the list of questions. It should probably have an each method (and include Enumerable), and an add method (possibly aliased to <<). Maybe also an [], if that makes sense. These methods could simply be delegated to the array, if convenient, but the point is: if you later decide to not use an array, you can do that without anyone even noticing. You can decide to only support those methods you actually want to support, as opposed to all ~100 methods of Array.

Is it better practice to use an attr_accessor here or not?

2 Answers2