Help with understanding/using dispatch tables

[Mothra]

Hello everyone,

I have a perl script (see below) that uses dispatch tables to set values,
however
subs get_lat_N and get_lat_S are almost identical :-( this tells me thatt I
am
doing something wrong. How can I modify my dispatch table structure to avoid
duplicate code?

Thanks

Mothra



-------script below--------

use HTTP::Request::Common;
use POSIX qw(floor);

#use Data:umper;
use HTML::Form;
use LWP::UserAgent;

#use LWP:ebug qw(+);
use strict;
use warnings;

my %dispatch_table_lat = (
'N' => \&get_lat_N,
'S' => \&get_lat_S
);
my %dispatch_table_lon = (
'E' => \&get_long_E,
'W' => \&get_long_W
);

our %location = ();

#open (OUT, ">> d:/tmp/fetched_cities.txt") or die "Can't open file:$!\n";
while (<DATA>) {

/(\w+),\s+(\w+)\s+(\d+)\s+(\d+)\s+(\w)\s+(\d+)\s+(\d+)\s+(\w)/;

$dispatch_table_lat{$5}->( $3, $4 );

$dispatch_table_lon{$8}->( $6, $7 );

$location{'zz1'} = floor( $location{'xx1'} / 15 );
my ( $sunrise, $sunset ) = get_sunrise();

print "$1 $2 sunrise: $sunrise sunset: $sunset\n";

#print OUT "$1, $2 $3 $4 $5 $6 $7 $8 sunrise: $sunrise sunset:
$sunset\n";

}

close OUT;

sub get_lat_N {
my ( $degree, $minute ) = @_;
$location{'yy0'} = '1';
$location{'yy1'} = $degree;
$location{'yy2'} = $minute;
return;
}

sub get_lat_S {
my ( $degree, $minute ) = @_;
$location{'yy0'} = '-1';
$location{'yy1'} = $degree;
$location{'yy2'} = $minute;
return;

}

sub get_long_E {
my ( $degree, $minute ) = @_;
$location{'zz0'} = '1';
$location{'xx0'} = '1';
$location{'xx1'} = $degree;
$location{'xx2'} = $minute;
return;
}

sub get_long_W {
my ( $degree, $minute ) = @_;
$location{'zz0'} = '-1';
$location{'xx0'} = '-1';
$location{'xx1'} = $degree;
$location{'xx2'} = $minute;
return;

}

sub get_sunrise {

my $ua = LWP::UserAgent->new;
$ua->agent('Mozilla/4.73');
my $req =
HTTP::Request->new(
GET => 'http://aa.usno.navy.mil/data/docs/RS_OneDay.html' );
my $res = $ua->request($req);

my @form = HTML::Form->parse( $res->content, $res->base() );

#xxy is year
#xxm is month
#xxd is day
$form[1]->value( 'xxy', "2007" );
$form[1]->value( 'xxm', "2" );
$form[1]->value( 'xxd', "20" );

#xx0 is -1 or 1 long 1 for east -1 for west
#xx1 is long in degrees
#xx2 is long in minites

$form[1]->value( 'xx0', $location{'xx0'} );
$form[1]->value( 'xx1', $location{xx1} );
$form[1]->value( 'xx2', $location{'xx2'} );

#yy0 is -1 for west 1 for east latitude
#yy1 is degrees
#yy2 is minites

$form[1]->value( 'yy0', $location{'yy0'} );
$form[1]->value( 'yy1', $location{'yy1'} );
$form[1]->value( 'yy2', $location{'yy2'} );

#zz1 is offset from GMT
#zz0 is -1 west of GMT 1 east

$form[1]->value( 'zz1', $location{'zz1'} );
$form[1]->value( 'zz0', $location{'zz0'} );

$req = $form[1]->make_request;
$req = $ua->request($req);
$req->content =~ /\s+Sunrise\s+(\d+:\d+)\s+/;

#print $req->content;
my $sunrise = $1;
$req->content =~ /\s+Sunset\s+(\d+:\d+)\s+/;
my $sunset = $1;
return ( $sunrise, $sunset );
}

__DATA__
Aberdeen, Scotland 57 9 N 2 9 W
Adelaide, Australia 34 55 S 138 36 E
Algiers, Algeria 36 50 N 3 0 E
Amsterdam, Netherlands 52 22 N 4 53 E
Ankara, Turkey 39 55 N 32 55 E
Asuncion, Paraguay 25 15 S 57 40 W

 

[Uri Guttman]

M> I have a perl script (see below) that uses dispatch tables to set
M> values, however subs get_lat_N and get_lat_S are almost identical
M> :-( this tells me thatt I am doing something wrong. How can I
M> modify my dispatch table structure to avoid duplicate code?

well, the simple answer is get rid of the dispatch tables as you don't
need them. they are not the best way to do this as you can simplify the
whole script by folding get_lat_N and get_lat_S into get_lat (same for
the EW subs). the dispatch table you have effectively just selects 1 or
-1 based on the compass value. you can use a hash for that or do a
simple test inside the subs.


M> my %dispatch_table_lat = (
M> 'N' => \&get_lat_N,
M> 'S' => \&get_lat_S
M> );
M> my %dispatch_table_lon = (
M> 'E' => \&get_long_E,
M> 'W' => \&get_long_W
M> );

M> $dispatch_table_lat{$5}->( $3, $4 );
M> $dispatch_table_lon{$8}->( $6, $7 );


$5 and $8 have NW and EW so why not just pass them as additional args
like this:


get_lat_NS( $3, $4, $5 );
get_lon_EW( $6, $7, $8 ) ;


M> sub get_lat_N {
M> my ( $degree, $minute ) = @_;
M> $location{'yy0'} = '1';
M> $location{'yy1'} = $degree;
M> $location{'yy2'} = $minute;
M> return;
M> }

M> sub get_lat_S {
M> my ( $degree, $minute ) = @_;
M> $location{'yy0'} = '-1';
M> $location{'yy1'} = $degree;
M> $location{'yy2'} = $minute;
M> return;

M> }

merge those two into this:

sub get_lat_NS {
my ( $degree, $minute, $compass ) = @_;

$location{'yy0'} = ( $compass eq 'N' ? '1' : '-1' ;
$location{'yy1'} = $degree;
$location{'yy2'} = $minute;
return;
}

do the same for the longitude subs.

so you lose 2 subs, 2 useless dispatch tables and have cleaner code.

a dispatch table is more useful when you have more than 2 subs to call
based on a key. and they should be doing different things. if they are
very similar as yours are, it is better to put the conditional logic in
the subs themselves as then you only have to change 1 sub when your
requirements change.

and why is the 1/-1 value in quotes? do you need it as a string? it
seems to be a form value so maybe that is ok.

 

[Mothra]

Hi Uri,

(snipped)

well, the simple answer is get rid of the dispatch tables as you don't
need them. they are not the best way to do this as you can simplify the
whole script by folding get_lat_N and get_lat_S into get_lat (same for
the EW subs). the dispatch table you have effectively just selects 1 or
-1 based on the compass value. you can use a hash for that or do a
simple test inside the subs.

(More snippage)
Ok I understand

$5 and $8 have NW and EW so why not just pass them as additional args
like this:


get_lat_NS( $3, $4, $5 );
get_lon_EW( $6, $7, $8 ) ;

Ok so far

(more snippage)

merge those two into this:

sub get_lat_NS {
my ( $degree, $minute, $compass ) = @_;

$location{'yy0'} = ( $compass eq 'N' ? '1' : '-1' ;
$location{'yy1'} = $degree;
$location{'yy2'} = $minute;
return;
}

works perfectly

do the same for the longitude subs.

I see, like this

sub get_long_EW {
my ( $degree, $minute, $compass ) = @_;
$location{'zz0'} = ($compass eq 'E' ? '1' : '-1');
$location{'xx0'} = $location{'zz0'};
$location{'xx1'} = $degree;
$location{'xx2'} = $minute;
return;
}

so you lose 2 subs, 2 useless dispatch tables and have cleaner code.

a dispatch table is more useful when you have more than 2 subs to call
based on a key. and they should be doing different things. if they are
very similar as yours are, it is better to put the conditional logic in
the subs themselves as then you only have to change 1 sub when your
requirements change.

Thanks for the clarification makes perfect sence

and why is the 1/-1 value in quotes? do you need it as a string? it
seems to be a form value so maybe that is ok.

Yes, I do need it as a string. it is a form value.

As always your advise is right on!

Thanks

Mothra