Please help critcize and shorten my sub code

[Richard Lee]

I dont know how to go through the array over and over again pending on
my previous search so I ended up writing it like below which works.. but
looks really really
inefficient..

sub dd_fact {
my $routename = shift;
my $routegroupid;
my $trunkgroupid;
my $carriername;
my $carrier_active;
my $carrierid;
AHI: for (@dat) {
if (exists $_->{outsideroute_group_m}{route_name}
and $_->{outsideroute_group_m}{route_name} eq "$routename") {
$routegroupid = $_->{outsideroute_group_m}{route_group_id};
last AHI;
}
}

EWF: for (@dat) {
if (exists $_->{outsideroute_trunk_m}{route_group_id}
and $_->{outsideroute_trunk_m}{route_group_id} eq
"$routegroupid") {
$trunkgroupid = $_->{outsideroute_trunk_m}{trunkgroup_id};
last EWF;
}
}

WWW: for (@dat) {
if (exists $_->{outsideotrunkgroup_m}{trunkgroup_id}
and $_->{outsideotrunkgroup_m}{trunkgroup_id} eq
"$trunkgroupid") {
$carrierid = $_->{outsideotrunkgroup_m}{carrier_id};
last WWW;
}
}

for (@dat) {
if (exists $_->{outsidecarrier_m}{carrier_id}
and $_->{outsidecarrier_m}{carrier_id} eq "$carrierid") {
$carriername = $_->{outsidecarrier_m}{carrier_name};
$carrier_active = live($_->{outsidecarrier_m}{active});
return($trunkgroupid,$carriername,$carrier_active);
}
}
}

[Jenda Krynicky]

From: Richard Lee <rich...@gmail.com>

> I dont know how to go through the array over and over again pending on
> my previous search so I ended up writing it like below which works.. but
> looks really really
> inefficient..
>
>
> sub dd_fact {
> my $routename = shift;
> my $routegroupid;
> my $trunkgroupid;
> my $carriername;
> my $carrier_active;
> my $carrierid;

You can declare several variables at once:

my ($routegroupid, $trunkgroupid, $carriername, $carrier_active,
$carrierid);

> AHI: for (@dat) {
> if (exists $_->{outsideroute_group_m}{route_name}
> and $_->{outsideroute_group_m}{route_name} eq "$routename") {
> $routegroupid = $_->{outsideroute_group_m}{route_group_id};
> last AHI;
> }
> }

You do not need the label unless you need to jump out of some other
loop than the innermost. In this case there are no nested loops so it
would probably be better to skip the label.

Also you do not need to, and should not, enclose variables in quotes.
This is Perl, not shell scripting.

>
> EWF: for (@dat) {
> ...

You may do something like this:

for my $wanted (
[outsideroute_group_m => route_name => \$routename,
route_group_id => \$routegroupid],
[outsideroute_trunk_m => route_group_id => \$routegroupid,
trunkgroup_id => \$trunkgroupid],
[outsideotrunkgroup_m => trunkgroup_id => \$trunkgroupid,
carrier_id => \$carrierid],
[outsidecarrier_m => carrier_id => \$carrierid, carrier_name =>
\$carriername],
[outsidecarrier_m => carrier_id => \$carrierid, active =>
\$carrier_active],
) {
if (exists $_->{$wanted->[0]}{$wanted->[1]}
and $_->{$wanted->[0]}{$wanted->[1]} eq ${$wanted->[2]}) {
${$wanted->[4]} = $_->{$wanted->[0]}{$wanted->[3]};
last;
}
}

return($trunkgroupid,$carriername,$carrier_active);
}

But I think you should rather rethink and rework your data structure.
Or possibly keep the data in a database (DBD::SQLite would probably
be best, no need for any external application, the whole database
engine in built into the module) and you can query the data to your
hearts content.

Jenda
===== Je...@Krynicky.cz === http://Jenda.Krynicky.cz =====
When it comes to wine, women and song, wizards are allowed
to get drunk and croon as much as they like.
-- Terry Pratchett in Sourcery

[Richard Lee]

thanks.. I am looking at your solution(looks very complicated) and also
will alos look into DBD::SQLite as well

thanks a bunch for quick response.

[Jenda Krynicky]

From: Richard Lee <rich...@gmail.com>

> Jenda Krynicky wrote:
> > You may do something like this:
> >
> > for my $wanted (
> > [outsideroute_group_m => route_name => \$routename,
> > route_group_id => \$routegroupid],
> > [outsideroute_trunk_m => route_group_id => \$routegroupid,
> > trunkgroup_id => \$trunkgroupid],
> > [outsideotrunkgroup_m => trunkgroup_id => \$trunkgroupid,
> > carrier_id => \$carrierid],
> > [outsidecarrier_m => carrier_id => \$carrierid, carrier_name =>
> > \$carriername],
> > [outsidecarrier_m => carrier_id => \$carrierid, active =>
> > \$carrier_active],
> > ) {
> > if (exists $_->{$wanted->[0]}{$wanted->[1]}
> > and $_->{$wanted->[0]}{$wanted->[1]} eq ${$wanted->[2]}) {
> > ${$wanted->[4]} = $_->{$wanted->[0]}{$wanted->[3]};
> > last;
> > }
> > }
> >
> > return($trunkgroupid,$carriername,$carrier_active);
> > }
> >

> thanks.. I am looking at your solution(looks very complicated) and also
> will alos look into DBD::SQLite as well

It's not terribly complicated once you get used to references. I just
extracted the differing parts from your loops so that I could replace
them by a single one.

And I made a mistake doing so, the code should have been like this:

for my $wanted (
[outsideroute_group_m => route_name => \$routename,
route_group_id => \$routegroupid],
[outsideroute_trunk_m => route_group_id => \$routegroupid,
trunkgroup_id => \$trunkgroupid],
[outsideotrunkgroup_m => trunkgroup_id => \$trunkgroupid,
carrier_id => \$carrierid],
[outsidecarrier_m => carrier_id => \$carrierid, carrier_name =>
\$carriername],
[outsidecarrier_m => carrier_id => \$carrierid, active =>
\$carrier_active],
) {

for (@dat) {

if (exists $_->{$wanted->[0]}{$wanted->[1]}
and $_->{$wanted->[0]}{$wanted->[1]} eq ${$wanted->[2]}) {
${$wanted->[4]} = $_->{$wanted->[0]}{$wanted->[3]};
last;
}
}
}

return($trunkgroupid,$carriername,$carrier_active);
}

I've forgotten the inner loop.

Maybe you'd like it better if I would have hidden the inner loop in a
subroutine:

my $routegroupid = findValue( 'outsideroute_group_m', 'route_name',
$routename, 'route_group_id');
my $trunkgroupid = findValue( 'outsideroute_trunk_m',
'route_group_id', $routegroupid, 'trunkgroup_id');
my $carrierid = findValue( 'outsideotrunkgroup_m', 'trunkgroup_id',
$trunkgroupid, 'carrier_id');
my $carriername = findValue( 'outsidecarrier_m', 'carrier_id',
$carrierid, 'carrier_name');
my $carrier_active = findValue( 'outsidecarrier_m', 'carrier_id',
$carrierid, 'active');

return($trunkgroupid,$carriername,$carrier_active);
}

sub findValue {
my ($dat, $section, $lookup_key, $lookup_value, $value_key) = @_;
for (@$dat) {
if (exists $_->{$section}{$lookup_key}
and $_->{$section}{$lookup_key} eq $lookup_value) {
return $_->{$section}{$value_key};
}
}
}

Looking at it again, I would like it better as well.

Jenda
P.S.: The => (sometimes called "fat comma") is almost equivalent to
an ordinary comma, the only difference is that if the thing to the
left of the => looks like a word it's automaticaly quoted. So
[one => two => $three]
is equivalent to
['one', 'two', $three]

It's just syntactic sugar.

[Richard Lee]

thank you.

this is excellent and this is what I need to start to think more in
terms of...
re-using the code.. or rather finding ways to re-use the code.

thank you!!! excellent job!

[Peter Scott]

On Sun, 11 May 2008 02:05:06 +0200, Jenda Krynicky wrote:
> You do not need the label unless you need to jump out of some other
> loop than the innermost. In this case there are no nested loops so it
> would probably be better to skip the label.

While I personally program the way you suggest, take note that "Perl Best
Practices" says that loops that are explicitly exited should be labelled
in all cases (http://www.oreilly.com/catalog/9780596001735/toc.html), so
we should not criticize people for following the practice.

--
Peter Scott
http://www.perlmedic.com/
http://www.perldebugged.com/

[Jenda Krynicky]

From: Peter Scott <Pe...@PSDT.com>

> On Sun, 11 May 2008 02:05:06 +0200, Jenda Krynicky wrote:
> > You do not need the label unless you need to jump out of some other
> > loop than the innermost. In this case there are no nested loops so it
> > would probably be better to skip the label.
>
> While I personally program the way you suggest, take note that "Perl Best
> Practices" says that loops that are explicitly exited should be labelled
> in all cases (http://www.oreilly.com/catalog/9780596001735/toc.html), so
> we should not criticize people for following the practice.

Looks like yet another case in which I disagree with PBP. IMHO, only
the loops that need to be labeled should be labeled. To me a label is
a warning sign, something that says "hey, be careful here, this is a
place to which we might jump, a loop out of which we might jump while
inside another loop or something like that." I can't think of a good
similitude, maybe I could compare it to overabundance of trafic
signs. If you put up too many, the drivers will become distracted and
are more likely to ignore or miss the one that was really important.

In either case "would probably be better to" doesn't look like
criticizing to me :-)

Jenda

[Rob Dixon]

Jenda Krynicky wrote:
>
> Only the loops that need to be labeled should be labeled.

I agree. Apart from your spelling of 'labelled' :)

Rob