Aran Deltac wrote: > Hi all, I've been hacking away at a set of components for representing, > modifying, and traversing trees of DBIx::Class objects. The API was > hammered out with some help from Matt and others on #dbix-class. > > Before a first release is ever made I wanted to give everyone some time > to take a look at the first of the Tree modules, > DBIC:Tree::AdjacencyList and DBIC:Tree::AdjacencyList::Ordered. I have > plans to add Nested Set type trees (and other lesser known types) and > provide a way to traverse the trees in a similar fashion as > Tree::Simple::Visitor does it. I'm hoping to have a final API that can > be consistent across all Tree types, so you can plug-n-play Tree > components as your needs change.
I don't use DBIC yet - I'll likely change from CDBI next time I revisit my model code - but I do use trees, so I was interested to see this. AdjacencyList parent - what does the code actually do if you try to make an object the root? The Description says the root is the 'row with a Parent ID of 0' but I can't see how that would ever get set. method return values - the docs don't state the return values. Since accessor/mutators normally always return the current attribute value, the fact that these don't really needs to be made VERY clear. BTW, what's the rationale for the values that are returned? attach_child - DBIC follows the CDBI convention of add_to_*. So why not call this method add_to_children? The description could also be clearer. The method could also take a list of new children. attach_sibling - same issue with name and possible list of arguments. Ordered position_column & parent_column - since they both have to be called exactly once in a specific order, it might be worth considering combining them into a single method. Is the ordering really restricted to a single column rather than some predicate? Could that be relaxed? position_column - this method is not listed. attach_before - $this->attach_before($that) reads as "this attach before that" but attach_before actually attaches that rather than this so IMHO a different name would be better. I can't think of one I like - add_senior_sibling? attach_after - same issue as attach_before _grouping_clause - this is deeply worrying. It overrides a private method of a base class that is marked in that class as "These methods are used internally. You should never have the need to use them." So there's something wrong somewhere! Minor typo stuff: "add .. to the top of the .. list" => "add .. to the front of the .. list" or is that a US => UK thing? General Comments There're no traversal methods (depth-first or breadth-first, all nodes at level 5 etc) or classification predicates (is_leaf etc). I guess you plan to add those? When you generalize the API for other representations, it would be nice if it could also handle DAGs. I must confess to a prejudice. I have a wariness about trees/graphs in databases. I tend to build the tree in memory and interrogate/manipulate it there, using the database only as a backing store. The tree structure itself is usually quite small compared to current memory sizes, even if it contains 100,000 nodes or so. So something that automatically cached the tree and knew which nodes needed writing back at a checkpoint would probably be more use to me. Cheers, Dave _______________________________________________ List: http://lists.rawmode.org/cgi-bin/mailman/listinfo/dbix-class Wiki: http://dbix-class.shadowcatsystems.co.uk/ IRC: irc.perl.org#dbix-class SVN: http://dev.catalyst.perl.org/repos/bast/trunk/DBIx-Class/
