Sort optimization questions

Ruby
1

hello everyone,

i have a model class Page in my rails application which is defined like
this:

class Page < ActiveRecord::Base
attr_accessor :children

relationships here…

end

and a NodeList class to sort this pages based on their parent_id
attributes: http://pastie.org/private/rbq05vzkc7wgyan8pcdrw

here is a simple example of how i’m using them:

node_list = NodeList.new(Page.all())
@pages_list = node_list.get_nodes()

i also have a helper method to print the tree:

def print_children(nodes)
retval = “

    ”
    nodes.each do |node|
    retval += “<li id="page_#{node.id}"><a
    href="/documents/#{node.document_id}/pages/#{node.id}">#{node.name}”
    if node.children && node.children.size > 0
    retval += print_children(node.children)
    end
    retval += “”
    end
    retval += “
”
return retval
end

currently it is giving me what i need:

  • Parent 1
    – Child 1
    — Sub Child 1
    — Sub Child 2
    – Child 2
    – Child 3
  • Parent 2
    – Child 4
    – Child 5
    — Sub Child 3

so, here are my two questions:

  • how can i make this better performing and rubyish?

  • as this is looping through every element of @page_list, after i’m done
    with the element, i would like to remove it with @page_list.delete(obj)
    in
    get_children method after the line “tmp << obj” however this is causing
    me
    lose data so instead of the above structure i get something like this:

  • Parent 1
    – Child 1
    — Sub Child 1
    – Child 2

  • Parent 2
    – Child 4

why is this happening?

thanks in advance.

Muhammet S. AYDIN
http://mengu.net
http://compector.com

2

On Wed, Dec 21, 2011 at 14:56, Muhammet S. AYDIN [email protected]
wrote:

  • how can i make this better performing and rubyish?

First, when you do “if node.children && node.children.size > 0”, I’m
not sure if that’s all needed. Without seeing the definition, I’m not
sure if a node with no children has children being nil, or an empty
array. If a childless node has children as nil, then BTW your first
check is nicely idiomatic Ruby (as opposed to “node.children != nil”
or “! node.children.nil?”). However in that case, checking the array
size may be redundant. On the other claw, if a childless node has an
empty array for children, then checking if it exists is redundant. Or
is it nil if it’s never been initialized, but could be filled with
children which could then get removed, leaving an empty array?

Also, adding onto a string is generally not very performant. I don’t
know offhand if this is the case in Ruby, but in many languages you
wind up creating a whole new object. So instead of tacking onto the
string, push onto an array, and then join it all together at the end.
You can even do it all in one swell foop by having the sub-invocations
return an array, something like:

def print_children(nodes)
get_children(nodes).join
end

def get_children(nodes)
retval = [“

    ”]
    nodes.each do |node|
    retval.push “<li id="page_#{node.id}"><a
    href="/documents/#{node.document_id}/pages/#{node.id}">#{node.name}”
    if node.children && node.children.size > 0
    retval.concat get_children(node.children)
    end
    retval.push “”
    end
    retval.push “
”
end

Give that a whirl and let us know if it improves the speed. It might
not be enough to really matter.

As for making it more Rubyish, note the removal of “return retval” –
you don’t need it, since in Ruby the return value of a function is the
value of the last expression evaluated, and push has the new value of
the array.

-Dave