I'm also curious why you would return the instance and the node list. If you're trying to hide details, handing back that instances raises a flag that there's something amiss in whichever callers are using the nodes_only form. Also, it seems odd to return undef rather than an empty hash if there no divisions. Is that special case really an abnormal error?

A refactoring might look like:

sub getDivisionListNodes {
    my($agency) = @_;

    return undef unless defined($agency);

    return new($agency)->{ts}->get_node_list('division');
}

sub getDivisionNameToIdMap {
    my($agency) = @_;

    return undef unless defined($agency);

    my %nameToId;
    foreach my $node ( getDivisionListNodes($agency) ) {
        $nameToId{$node->value{'divname')} = $node->value('siebel_id')
+;
    }
    return %nameToId;
}
[download]

First of all, thank you for your input. Now for my comments:

Something smells about that subroutine. On first glance it looks like a refactoring to split it into two methods (to get rid of the $nodes_only) parameter might make the code clearer.

Hmmm, now that is a good idea. I just sorta took advantage of the fact that everyone else was going to call it in a scalar context and shoved the things I needed on the front of a list so only I would get them. But you are right, the refactoring is a good idea.

It also gives you the opportunity to pick better names (e.g., don't suggest that a routine returns a list when it reallyl returns a hash).

I agree, but try and tell my boss that. Everyone around here calls a hash a "hash array" for some reason.

I'm also curious why you would return the instance and the node list. If you're trying to hide details, handing back that instances raises a flag that there's something amiss in whichever callers are using the nodes_only form.

Agreed, I was told to write a bunch of self-contained routines. In retrospect, I should have written a bunch of methods callable after a new. Instead, each function is calling new itself. This gets especially onerous when you have 80 divisions and need to call getDivisionList80 times that spells 80 object creations and destructions instead of just one object creation and 80 method calls with the initialized object.

Also, it seems odd to return undef rather than an empty hash if there no divisions. Is that special case really an abnormal error?

It is my understanding that return returns whatever is most appropriate based on the caller. Ie, it returns undef in scalar context and an empty list in the list context (which would cover both arrays and hashes).

Carter's compass: I know I'm on the right track when by deleting something, I'm adding functionality

It is my understanding that return returns whatever is most appropriate based on the caller. Ie, it returns undef in scalar context and an empty list in the list context (which would cover both arrays and hashes).

return; does. return undef does not. I'd remove the undef from the sanity-check.

This gets especially onerous when you have 80 divisions and need to call getDivisionList80 times that spells 80 object creations and destructions instead of just one object creation and 80 method calls with the initialized object.

Without knowing what kind of additional overhead is involved with creating an instance of an object, it's hard to know whether it's worth worrying about a relatively small number of creates and DESTROYs. If there's a lot of overhead involved, you might consider creating a special DivisionLister class that can hang on to a single instance of the object.

Abigail

Either that or given that my is so common it might be the default or pragma-chooseable.

Carter's compass: I know I'm on the right track when by deleting something, I'm adding functionality

No, that wouldn't help. There has to be a way to discriminate declaration from use if you want to be able to tell a use of a mistyped variable apart from a declaration of a new one. I don't think three characters is a high price to pay for that service - especially considering you can, if the occasion lends itself to it, declare multiple variables with a single my.

Your code has room for clarification and shortening in other areas; I'd write it like this:

sub getDivisionList {
    (my ($agency_name, $nodes_only) = @_) or return;

    my $obj = __PACKAGE__->new($agency_name);
    my @node = $obj->{ts}->get_node_list('division');

    return
        (!@node)    ? () :
        $nodes_only ? ($o, @n) :
            map {($n->value('divname'), $n->value('siebel_id'))} @n;
}
[download]

(The jury's still out on how to indent the dangling last "default" expression, btw. I'm not entirely satisfied with any of the options so far.)

Makeshifts last the longest.

— John

Perl6 will let you, with operator-subs, and unicode-named subs. Of course, being able to type ∃ and ∀ is up to you. (You can do this in p5 with a source filter, but you might have trouble if you use ∃ or ∀ for other things in your code, and the filter isn't written correctly.

Of course, you were joking, weren't you. Perl6 will allow implementation of all sorts of new jokes.

---

Warning: Unless otherwise stated, code is untested. Do not use without understanding. Code is posted in the hopes it is useful, but without warranty. All copyrights are relinquished into the public domain unless otherwise stated. I am not an angel. I am capable of error, and err on a fairly regular basis. If I made a mistake, please let me know (such as by replying to this node).

Doesn't Perl 5 allow Unicode in sub names, just as in any variable name?

Those are math symbols, not "letters", so it won't match \W and would not be legal in identifiers, as things stand now.

But I can certainly see the use for non-letter names of one character (similar to today's punctuation-mark names), as that will open up the math operators and other symbols.

I take it you want undeclared variables to be used in a lexical scope instead of global. I don't see what functionality you're adding except for making 'my' optional. Nice idea though. I'd probably want a something-scoped prama to select which behaviour undeclared variables go to though. I'd guess you could implement this for perl6 if you wanted it bad enough.

__SIG__
use B;
printf "You are here %08x\n", unpack "L!", unpack "P4", pack
  "L!", B::svref_2object(sub{})->OUTSIDE;
[download]