[Israel.pm] Comments on https://github.com/moshe742/mail-stats/blob/master/mail-stats.pl

Shlomi Fish shlomif at shlomifish.org
Sun Sep 23 11:42:09 PDT 2012


Hi Moshe!

Here are my comments for:

https://github.com/moshe742/mail-stats/blob/master/mail-stats.pl

First of all, you should read:

http://perl-begin.org/tutorials/bad-elements/

Now for the stuff:

#. There should be a line spacing between the sha-bang "#!" and the "use warnings".

#. <STDIN> should preferably just be <>: 

http://perl-begin.org/tutorials/bad-elements/#STDIN_instead_of_ARGV

#. As a general rule I prefer inputting input using @ARGV instead of <>.

#. If you do insist on using command line input, consider using Term::ReadLine :

http://perldoc.perl.org/Term/ReadLine.html

#. You have "html { " , "down {" ,etc. Are these supposed to be subroutines?
Where is the "sub" keyword?

#. "$dir/mail.txt" should preferably be done using File::Spec->catfile.

#. You should parse HTML using a module:

http://perl-begin.org/uses/text-parsing/

#. << push @files, $gzip if $gzip =~ /$year/; >> $year will be interpolated as
a small regex. You may want to use \Q and \E or better yet - https://duckduckgo.com/?q=perl%20index

#. print "downloading and extracting the files, please be
patient...\n\n";   - the message should start with a Capital letter.

#.

<<<
for ( @files ) {
system("wget -P $dir $_") if $_ =~ /$year/;
my $ur = $_;
>>>

Don't misues $_ for loops in production code. Also, use a more meaningful name than "ur". 
Finally, you can use LWP instead of shelling out to wget.

#. system("gzip -d $dir/*.gz"); 

This is potentially dangerous. Try using Compress::ZLib instead.

#. open my $FILE05, ">", "$dir$file_name" or die "can't open

Why is it called $FILE05? What is the meaning of the 05 digits?

#. next if $file_name eq "." or $file_name eq "..";

Use File::Spec->no_upwards instead:

http://perl-begin.org/tutorials/bad-elements/#no_upwards_for_dirs

#. 	for ( @file ) {

Again , don't misuse $_. It gets clobbered easily.

#.	while ( <$FILE> ) {
say $FILE05 $_;
}

Wrap $FILE05 with  { ... } and are you aware that you are printing
it with double newline?

#.  	my $num_of_mails = 0;

for ( 1 .. $number ) {
$num_of_mails = $sort[-$_] + $num_of_mails;
}


Just use List::Util::sum with an array slice.

my $num-of_mails = sum ( @sort[ -$number .. -1 ]  );

===============

Regards,

	Shlomi Fish

-- 
-----------------------------------------------------------------
Shlomi Fish       http://www.shlomifish.org/
Why I Love Perl - http://shlom.in/joy-of-perl

Tcl is Lisp on drugs. Using strings instead of S‐expressions for closures is
Evil with one of those gigantic E’s you can find at the beginning of chapters.

Please reply to list if it's a mailing list post - http://shlom.in/reply .


More information about the Perl mailing list