Why attr_accessor won't work in setting methods

Question

I am implementing BST in ruby and writing insert method using recursion.

I am trying to use attr_accessor for setting and getting root but it doesn't work. Can anyone help out?

class Node 
  attr_accessor :value, :left_child, :right_child

  def initialize (value)
    @value = value 
    @left_child = nil 
    @right_child = nil 
  end
end 

class BST 
  attr_accessor :root 

  def initialize
    @root = nil
  end 



  def insert(value, node)

    if node == nil 
      node = Node.new(value)
      return node
    end 

    return node if node.value == value

    if node.value > value
      if node.left_child == nil 
        node.left_child = Node.new(value)
        return
      else 
        insert(value, node.left_child) 
      end 

    else

      if node.right_child == nil 
        node.right_child = Node.new(value)
        return
      else 
        insert(value, node.right_child) 
      end
    end 
  end 
end 


mybst = BST.new
p mybst.root
mybst.insert(1, mybst.root)
mybst.insert(2, mybst.root)
mybst.insert(10, mybst.root)
mybst.insert(12, mybst.root)

p mybst

The code above shows a simple implementation of Node class and a BST class with insert method. Gives me #<BST:0x00557398d02378 @root=nil>

If I use a self.root it works.

One can use @root to access the root but a class should not interact with its instance variables directly. That's why we need a getter and setter method provided by attr_accessor. But it's not working. What I am missing ?

Below are the screenshots of the book POODR. It says never to use instance variables directly even in class.

Keep in mind the only things in Ruby that are logically false are nil and false so unless there's some way that things like right_child can ever be literally false it's not necessary nor recommended to do comparisons like == nil. This not only clutters your code up and raises questions about what values that property can contain, but runs the risk of an accidental assignment if you type = nil by mistake. — tadman
@tadman Hey! I wanted to give a review if the OP took it over to CR.se! ;) — thesecretmaster
@thesecretmaster That's just the beginning. There's a lot you could convey here to make it more Ruby-like. — tadman

thesecretmaster thesecretmaster · Accepted Answer · 2017-12-26T02:59:12

It's actually totally OK to use instance variables within instance methods. In fact, that's what they're for! Setters and getters allow things outside the instance to access variables inside the instance. They (basically) define instance methods for the class like this:

class Foo
  # getter -- Same as attr_reader :root
  def root
    @root
  end

  # setter -- Same as attr_writer :root
  def root=(root)
    @root = root
  end

  # attr_accessor defines a setter *and* a getter.
end

So, you could simplify your code by defining #insert so that it only takes one argument (value) and replace every place where you reference node with a reference to @root.

The way that I think you're looking for (but is not the "right" way, and I wouldn't recommend) is to call the root and root= methods defined by the accessor.

If you took this route, you'd also have to define #insert to only take value as an argument and replace every place that you reference node with root. This will work, but it's not the right way to solve the problem. If you solve it this way, please ask a question on CodeReview.se so I can clarify how you can make the code better.

Why it's not working

In the #insert method, you're manipulating the node parameter that was passed to the method, not root. Ruby is pass by value not pass by reference (sorta), so when you pass mybst.root to #insert, you're effectively passing nil because mybst.root == nil. Then the mybst.insert call returns a new Node, but you don't do anything with that return value. If you wanted to set root to that return value, you could do:

mybst = BST.new
p mybst.root
mybst.root = mybst.insert(1, mybst.root)
mybst.root = mybst.insert(2, mybst.root)
mybst.root = mybst.insert(10, mybst.root)
mybst.root = mybst.insert(12, mybst.root)

p mybst

Explanation of what that textbook is trying to say

I think that the confusing part here is where the textbook says:

Hide the variables, even from the class that defines them

This is correct, but I think you're misinterpreting it. This section is saying that you should hide instance variables from anything outside of that instance. Within that instance, it's totally OK to use them, and that's actually the reason why instance variables exist -- to store state within the instance. It's just considered better to define methods for behaviors rather than directly exposing the instance variables. Of course, this is just one rule to keep in mind -- I'm sure you'll come across a situation where this advice does not apply, but generally you want to keep instance variables internal.

Why attr_accessor won't work in setting methods

1 Answers

Why it's not working

Explanation of what that textbook is trying to say