in reply to my, my, my

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. It also gives you the opportunity to pick better names (e.g., don't suggest that a routine returns a list when it really returns a hash).

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]
Note that the 'my' count has been reduced.

Replies are listed 'Best First'.
Re: Re: my, my, my
by princepawn (Parson) on Nov 14, 2002 at 19:36 UTC
    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

[reply]
[d/l]
[select]
      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.

[reply]
[d/l]
[select]
      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.

[reply]

In Section Meditations