[LA.pm] See if this sparks some perl talk.

[Kevin Scaldeferri]

On Oct 18, 2004, at 11:22 AM, Nicholas Bernstein wrote:

>
> Anyway, since I threw this together rather quickly, didn't put enough
> effort in to be embarrassed by it, I figured maybe it might be a good
> way to spark up the list a bit. Critique, improve, extend, replace, 
> turn
> into something else, etc.

Okay, I'll add a couple comments.  Some are things I think are pretty 
universal best practices, while some are more my personal preference.


>
>
> ###------------------------ start script
>
>         #!/usr/bin/perl -w
>         use warnings;
>         #use Mail::Sendmail;
>         #use diagnostics;
>
>         $^W=0;

-w, warnings, and $^W all do nearly the same thing.  You seem to be 
undecided about what you actually want here.


>         $debug = 0;
>

I'd make this modifiable at the command line.

>
>         $url 		= "\'" . $ARGV[0] .  "\'" ;

This is overly verbose.  You could just say

$url = "'$ARGV[0]'";

however...

>         $mail_app	= '/usr/bin/mail' ;
>         $have_score 	= 0 ;
>         @page 		= `lynx -dump $url`;

The single quotes added above are presumably to protect this command, 
but it isn't really sufficient.  (What if $url contains a single 
quote?)  It's better to do

@page = `lynx -dump \Q$url\E`

to escape all non-alphanumerics


>         $email 		=  '8675309 at vtext.com, me at nicholasbernstein.com';
>         $regex		=  '(NY Yankees \d+, [Bb]oston \d+)';

use qr// here

>
>
>         if ( defined($ARGV[2]) ) {
>         	$log = $ARGV[2];
>         } else {
>         	$log = "/tmp/scores.pl.log";
>         }

$log = $ARGV[2] || "/tmp/scores.pl.log";

>
>         print "$log\n" if ( $debug == 1 ) ;
>
>         foreach ( @page ) {
>         	if (	( $_ =~ m/$regex/)
>         		and ( $have_score == 0 )) {

Never write something like "$_ =~ m/.../".  Either name the variable or 
use $_ implicitly.  Using $_ explicitly is just ugly.

You don't need the parens if you use 'and'.

I would get rid of the $have_score flag and just call 'last' down where 
you set it to 1.


>         			$score = $1;
>         			chomp($score);

Pointless, your regex don't match a trailing newline.

>         			if ( -f $log ) {
>         				print "In log exists\n" 	if ($debug==1);
>         				open("LOG", "<$log") 		or die("problem opening file\n");

Don't use the 2 argument open.

>         					@old_score = <LOG> 	or die("could not read log\n");

If your log file gets big, this could use a bunch of memory.  
Pointless, especially if you're only going to use the first line.

>         				close("LOG");
>         				print "\nLog: $old_score[0]\n" 	if ( $debug == 1 );
>         				$old_score=$old_score[0];
>         			} else {
>                                         print "In log does not 
> exist\n"	if ($debug==1);
>         				open("LOG", ">", $log);
>         					print LOG "none";
>         				close(LOG);
>         				$old_score = "none";
>         			}

I'm a big fan of lexical filehandles.  For little scripts, maybe it 
doesn't matter much, but as things get bigger, global filehandles are 
bad news, particuarly if you give them names like LOG or IN or OUT.


>
>         			chomp($old_score);
>         	
>         			if  ( $score ne $old_score ) {
>         				open(MAILAPP, "|$mail_app -s \"RedSox Score\" -r 
> \"scores\@nicholasbernstein.com\" $email");

How come the recipients are defined in a variable, buy not the return 
address?


>         					print  MAILAPP "$score" ;
>         				close("MAILAPP");
>         				print "$score\n";
>         				open ("LOG", ">", $log);
>         					print LOG "$score\n";
>         				close("LOG");

Ahh... I see here that you overwrite the log, so it's only ever 1 line. 
  So, I guess maybe my comment above is moot.

You print the same thing to three different filehandles.  Seems like a 
good place for a loop.


>         				print "Scores are different"	if ( $debug == 1 );

You use this construct a lot.  Seems like you need a "debug" subroutine.


>         			}
>         			$have_score = 1 ;

See above, replace with 'last'

>         	}
>         }
>
> ###------------------------ end script
>


-kevin