Re: Please help critcize and shorten my sub code

From: Richard Lee <[email protected]>
> 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.
===== [email protected] === 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