Paul Johnson wrote:
>
> > sub get_root_classes {
> > my $self = shift;
> >
> > my @root_classes;
> > my @unresolved_classes;
> >
> > foreach (keys $self->{'Classes'}) {
> > if ($_->{'parent'}) {
> > push $_, @unresolved_classes;
> > } else {
> > push $_, @root_classes;
> > }
> > }
> >
> > return [EMAIL PROTECTED], [EMAIL PROTECTED];
> > }
> >
> > Feedback? Is the above terribly mysterious about what it does? Does it
> > really requie comments to elucidate?
>
> I think it needs something, possibly fewer syntax errors ;-)
>
> I presume this is code that you have just made up on the spot rather than
> something taken from a working project,
Well, no, [blusing deep red] but I did have to reverse the push arguments to get
it to work.
> but I couldn't say for sure what
> the Classes hash was doing.
Holding classes. Sorry, that is a bit confusing in terms of nomenclature.
$self is a ConceptHierarchy reference, so I am pretty constantly aware of the
possible ambiguity between internal progream structure and the subject matter.
> Here's how I would code something similar for
> myself or on a team of experienced Perl programmers. I don't expect
> everyone to agree with everything. In particular, I expect most people
> would be happier with an explicit return statement.
>
> sub get_class_types
> {
> my $self = shift;
>
> my $roots = [];
> my $unresolved = [];
>
> while (my ($class, $props) = each %{$self->{classes}})
> {
> push @{$props->{parent} ? $unresolved : $roots}, $class;
> }
>
> ($roots, $unresolved)
> }
OK, you tipped the balance. I was debating whether to make resolved and
unresolved anonymous, and whether to use the ? operator. What I have now pretty
much looks like what you show, except that I explicitly provided the default
variable as the *second*argument to push, and did indeed make the return
statement explicit.
Joseph
--
To unsubscribe, e-mail: [EMAIL PROTECTED]
For additional commands, e-mail: [EMAIL PROTECTED]
