Krdytkyu

I am implementing a parent-child relationship, represented by hashes that look like this:

parent = { 'parent' => nil, 'id' => 3 }

child = { 'parent' => parent, 'id' => 7 }

These hashes are data given to me, for example, through a JSON API.

(As per a question in the comments, the JSON for the child would look like:)

{

  "parent": { "parent": null, "id": 3, ...etc.},

  "id": 7,

  ...etc.

}

Parent and child have more keys like name, etc. (they are a lot so I am only showing the id as an example). They both have the same keys. There are only parents and children, i.e., no grandchildren or grandparents, etc. There is only one "level". Each child has only one parent, but each parent may have several children.

I wrapped the data in a class to represent it, so that my application can use an object. Both parent and child are represented by the same class. So I did this:

class Item

  def initialize(data)

    @data = data

  end



  def parent

    @parent ||= Item.new(data['parent']) if data['parent']

  end



  def id

    data['id']

  end



  private



  attr_reader :data

end

Every key in the hash has its method to retrieve it, and Item is the class that represents parents and children. I use it like this:

describe 'children' do

  let(:subject1) { Item.new(child) }



  it 'is an item' do

    subject1.class.must_equal(Item)

  end



  it 'has an id' do

    subject1.id.must_equal(7)

  end



  it 'has a parent' do

    subject1.parent.id.must_equal(3)

  end

end



describe 'parent' do

  let(:subject2) { subject1.parent }



  it 'is also an item' do

    subject2.class.must_equal(Item)

  end



  it 'does not have a parent' do

    assert_nil(subject2.parent)

  end

end

With this, children know who is their parent, and for a parent to know its children we can iterate through all the children and compare their parent's id with the id we are searching.

However, I have a bad feeling about calling Item.new inside of Item. Is like a lot of things could go wrong. I thought of using a Item factory through dependency injection, but really all the knowledge about how to build a Item is in Item, so why introduce that dependency. Is this like a weird kind of recursion?

It doesn't help that I haven't seen this implementation anywhere (I'm not a computer scientist though). Is this bad code? I have a weird feeling, but I can't quite word why.

I don't think you should worry about calling new like that, but you can avoid it by embracing objects and Ruby, it's not perl, you're not stuck with hashes! Make your data into objects and if you need a hash representation of it then write a method that converts the object into a hash.

If you want to be able to keep track of objects that's when I'd use a hash, by putting it into a class variable, or even better, a class instance variable.

Additional:

Since you've mentioned JSON in the comments, or perhaps you'd be passed the hash, then I'd still use objects, just add a way to coerce a hash into an object. A library I've forked called Grackle communicates with Twitter and can receive either JSON or XML. It does this by loading handlers, which means the same data object class can receive many differing types of input and still produce an object with a standard interface.

module Handlers

  module JSONHandler

    require 'json'



    def from_json json

      hash = JSON.parse(json)

      recursive hash

    end



    # You might want to add an extra check to stop an infinite loop

    # Beware the data!

    def recursive hash

      return nil if hash.nil?

      return hash unless hash.respond_to? :fetch

      Item.new id: hash["id"], parent: recursive(hash["parent"])

    end

  end

end





class Item

  extend Handlers::JSONHandler



  def self.items  

    @items ||= {}    

  end    





  def initialize(id:, parent:nil)  

    @parent = parent    

    @id = id    

    self.class.items[@id] = self  

  end    



  attr_reader :parent, :id  



  def to_h  

    h = {id: @id, parent: (@parent && parent.id) }    

    h.reject{|k,v| v.nil? }    

  end    



  alias_method :to_hash,:to_h  

end

# => Item

parent = Item.new id: 3

# => #<Item:0x007fa94a4010c8 @parent=nil, @id=3>

child = Item.new id: 7, parent: parent

# => #<Item:0x007fa94a9abe38 @parent=#<Item:0x007fa94a4010c8 @parent=nil, @id=3>, @id=7>

Item.items

# => {3=>#<Item:0x007fa94a4010c8 @parent=nil, @id=3>, 7=>#<Item:0x007fa94a9abe38 @parent=#<Item:0x007fa94a4010c8 @parent=nil, @id=3>, @id=7>}

Item.items[3]

# => #<Item:0x007fa94a4010c8 @parent=nil, @id=3>

Item.items[7]

# => #<Item:0x007fa94a9abe38 @parent=#<Item:0x007fa94a4010c8 @parent=nil, @id=3>, @id=7>

Item.items[7].parent

# => #<Item:0x007fa94a4010c8 @parent=nil, @id=3>

parent.to_h

# => {:id=>3}

child.to_hash

# => {:id=>7, :parent=>3}

json_parent = parent.to_h.to_json

# => "{"id":3}"

json_child = child.to_h.to_json

# => "{"id":7,"parent":3}"

parent2 = Item.from_json json_parent

# => #<Item:0x007fb543e26940 @parent=nil, @id=3>

child2 = Item.from_json json_child

# => #<Item:0x007fb5431d7270 @parent=3, @id=7>

and to make calling it easy:

def self.Item(*args, **keywords)

  case args.first

  when String # it's JSON

    Item.from_json args.first

  when Hash

    # you might want to nick the recursive bit from the handler

    # or write an Hash handler etc

    Item.new id: args.first["id"], parent: args.first["parent"]

  when Item

    args.first

  else

    Item.new id: keywords[:id], parent: keywords[:parent]

  end

end



Item(parent)

# => #<Item:0x007fb9b2765160 @parent=nil, @id=3>

Item(child)

# => #<Item:0x007fb9b272f330 @parent=3, @id=7>

Item(json_parent)

# => #<Item:0x007fb9b26fdd30 @parent=nil, @id=3>

Item(json_child)

# => #<Item:0x007fb9b26c7dc0 @parent=3, @id=7>

Item(parent.to_h)

# => #<Item:0x007fb9b26965b8 @parent=nil, @id=3>

Item(child.to_h)

# => #<Item:0x007fb9b2664928 @parent=3, @id=7>

Item(id: 3)

# => #<Item:0x007fb9b262ed50 @parent=nil, @id=3>

Item(id: 7, parent: parent)

# => #<Item:0x007fb9b25fc990 @parent=#<Item:0x007fb9b23bd7b0 @parent=nil, @id=3>, @id=7>