Is attr_reader bad?

I'm learning ruby and my opinion about attr_reader is bad. Because you can change the value of particular instance variable from outside of class. For example:

class X
  attr_reader :some_string

  def initialize
    @some_string = "abc"
  end
end

tmp = X.new
puts tmp.some_string

tmp.some_string[0] = "aaaaaaaaa"
puts tmp.some_string

When you run this code you can see that the instance variable some_string has changed. So to avoid this I'm always making my own getter, and returning frozen object. For example:

class X
  def initialize
    @some_string = "abc"
  end

  def some_string
    @some_string.freeze
  end
end

tmp = X.new
puts tmp.some_string

tmp.some_string[0] = "aaaaaaaaa"
puts tmp.some_string

Now when you run the code, it throws an error saying can't modify frozen String: "abc", which is what I wanted. So my question is should I use attr_reader and is always returning frozen objects from getters bad practice?

Solution

attr_reader is not "bad", it simply does what it does, which is to return the value of the instance variable, which is a reference. Unlike attr_writer or attr_accessor it will not allow you to change the value of the instance variable (ie you can't change what the instance variable refers to)

The question is do you really want or need the reference. For example say you want to convert the value of some_string to upper case. You could use attr_reader to get the reference to some_string and then call upcase! on it like below. But having the reference allows you to call any method on the object including the []= method which maybe you don't want to allow. Note: []= is a method that manipulates the content of what some_string references it does not set @some_string to a new value, it still points to the same object, but the object it points to was manipulated.

   class Foo
      attr_reader :some_string
    
      def initialize()
        @some_string = "abc"
      end
    end
    
    puts "Foo"
    foo = Foo.new
    some_string = foo.some_string
    puts some_string #=> abc
    some_string.upcase!
    p foo # => #<Foo:0x0000563c38391ac8 @some_string="ABC">
    puts some_string.object_id # => 60
    some_string[0] = "x"
    p foo # => #<Foo:0x0000563c38391ac8 @some_string="xBC">
    puts some_string.object_id # => 60  ... ie same object different content  
    # foo.some_string = "ABC" is a runtime error

If you don't want to allow arbitrary methods to be called on an instance variable then you should not expose it using attr_reader, rather you should manipulate the instance variable via methods in your class. For example below I "delegate" the upcase! method to the instance variable @some_string, and I provide a string_value method to return a new string with the same value as the instance variable.

    class Bar
        def initialize()
            @some_string = "abc"
        end
        def upcase!()
            @some_string.upcase!
        end
        def string_value()
            "#{@some_string}"
        end
    end
    
    puts "Bar"
    bar = Bar.new
    p bar # => #<Bar:0x0000563c383915a0 @some_string="abc">
    bar.upcase!
    p bar # => #<Bar:0x0000563c383915a0 @some_string="ABC">
    some_string = bar.string_value
    p some_string # => "ABC" 
    some_string[0] = "x"
    p bar # => #<Bar:0x0000563c383915a0 @some_string="ABC">
    p some_string # => "xBC"

So I would say attr_reader is not bad, you might argue it is over used, I know I will often use it to "get" an instance variable when all I really need is some property on the instance variable.