<!DOCTYPE HTML PUBLIC "-//W3C//DTD HTML 4.0//EN" "http://www.w3.org/TR/REC-html40/strict.dtd"><html><head><meta name="qrichtext" content="1" /><style type="text/css">p, li { white-space: pre-wrap; }</style></head><body style=" font-family:'DejaVu Sans'; font-size:9pt; font-weight:400; font-style:normal;">On May 1, 2009 03:15:19 pm Uri Guttman wrote:<br>
> >>>>> "HB" == Henry Baragar <Henry.Baragar@instantiated.ca> writes:<br>
><br>
> HB> #! /usr/bin/perl -w<br>
><br>
> use warnings is better these days.<br>
> HB> use strict;<br>
><br>
> HB> sub oid_tree_add {<br>
> HB> my ($tree, $oid, $value) = @_;<br>
> HB> my @oid = split /\b/, $oid;<br>
><br>
> why are you splitting on \b? you could just split on /\./ and get the<br>
> oid numbers. then you can simplify (and speed up) the code below.<br>
><br>
You are correct (the code evolved and I did not finish refactoring).<br>
<p style="-qt-paragraph-type:empty; margin-top:0px; margin-bottom:0px; margin-left:0px; margin-right:0px; -qt-block-indent:0; text-indent:0px; -qt-user-state:0;"><br></p>See attached with this refactoring.<br>
<p style="-qt-paragraph-type:empty; margin-top:0px; margin-bottom:0px; margin-left:0px; margin-right:0px; -qt-block-indent:0; text-indent:0px; -qt-user-state:0;"><br></p>> HB> _oid_node_add($tree,$value,@oid);<br>
> HB> }<br>
><br>
> HB> sub _oid_node_add {<br>
> HB> my ($tree, $value, $key, @suboid) = @_;<br>
> HB> die "Expecting integer, got '$key'" unless $key =~/^\d+/;<br>
> HB> my $node = $tree->{$key};<br>
> HB> if (my $dot = shift @suboid) {<br>
> HB> die "Expecting '.', got '$dot'" unless $dot eq ".";<br>
> HB> $node->{subtree} = _oid_node_add($node->{subtree}, $value, @suboid);<br>
><br>
> i won't get into detail here but if you split on . as i said above, you<br>
> can just delete the check for dot. i can't see any reason to keep that<br>
> logic.<br>
><br>
> HB> }<br>
> HB> else {<br>
> HB> $node->{value} = $value;<br>
> HB> }<br>
> HB> $tree->{$key} = $node;<br>
><br>
> i don't see $node being modified anywhere. you modify its keys/values<br>
> but $node is still the same hashref. so that line probably could go.<br>
It simply reads better (IMHO).<br>
<p style="-qt-paragraph-type:empty; margin-top:0px; margin-bottom:0px; margin-left:0px; margin-right:0px; -qt-block-indent:0; text-indent:0px; -qt-user-state:0;"><br></p>> HB> $tree;<br>
><br>
> use explicit return statements vs implicit last value returns. this is a<br>
> bug waiting to happen if you ever add/modify code in that section. you<br>
> may forget what was being returned and return some other expression.<br>
><br>
> HB> }<br>
><br>
> HB> sub oid_tree_sorted {<br>
> HB> my ($tree) = @_;<br>
> HB> my @sorted;<br>
> HB> for my $key (sort {$a <=> $b} keys %$tree) {<br>
> HB> my $node = $tree->{$key};<br>
> HB> push @sorted, $node->{value} if $node->{value};<br>
> HB> push @sorted, oid_tree_sorted($node->{subtree})<br>
> HB> if $node->{subtree};<br>
><br>
> if a node has a value it won't have a subtree. <br>
No, a node is allowed to have both (this is not a pure tree).<br>
<p style="-qt-paragraph-type:empty; margin-top:0px; margin-bottom:0px; margin-left:0px; margin-right:0px; -qt-block-indent:0; text-indent:0px; -qt-user-state:0;"><br></p>> so you don't need the<br>
> second test. in general you could have eliminated the need for the<br>
> value/subtree keys and just used the value or hash ref. then you can<br>
> tell if it is a value or tree node with ref(). it is a simpler and<br>
> cleaner data tree design. again, i could be wrong in your details but<br>
> this is a common design idea in perl.<br>
><br>
True, if you have a pure tree (which we don't)<br>
<p style="-qt-paragraph-type:empty; margin-top:0px; margin-bottom:0px; margin-left:0px; margin-right:0px; -qt-block-indent:0; text-indent:0px; -qt-user-state:0;"><br></p>> i leave the actual rewrite as an exercise as i am not up for it at the<br>
> moment. :)<br>
><br>
> thanx,<br>
><br>
> uri<br>
<p style="-qt-paragraph-type:empty; margin-top:0px; margin-bottom:0px; margin-left:0px; margin-right:0px; -qt-block-indent:0; text-indent:0px; -qt-user-state:0;"><br></p>Regards,<br>
Henry<br>
<br />-- <br /><br>
Henry Baragar<br>
Instantiated Software<br>
416-907-8454 x42</p></body></html>