Re: Please help critcize and shorten my sub code

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