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

moshe nahmias moshegrey at ubuntu.com
Sun Sep 30 04:00:34 PDT 2012

Hi, comments are in post

On Sun, Sep 23, 2012 at 8:42 PM, Shlomi Fish <shlomif at shlomifish.org> wrote:

    Hi Moshe!

    Here are my comments for:


First of all, thanks for the feedback, till now only you and gaal gave
At least sawyer and Gabor said they will make it OO and refactoring for the

    First of all, you should read:


I will read it soon.

    Now for the stuff:

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


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


Why <> instead of <STDIN>? If I use <> it doesn't stop there but goes on to
the next line of code while I want it to stop since the user supposed to
input something there...

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

Done both of the options, now you can put the information through file,

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


I should think about it, I don't see a benefit from it but now I thought of
something that can make it benefitial (even for this code), so I might do
it after all.
Any way, what are the pro's and con's for doing this in this way? at least
I will learn something :)

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

Added the sub keyword and started to work on OO version.

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


    #. You should parse HTML using a module:



    #. << 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 -

Didn't understand the link you gave. Can you explain?

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

Thanks, done, and if there are more problems with my grammar writing please
tell me.


    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".

Done, including the name change.

    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.

Now using LWP and IO::Uncompress::Gunzip for downloading and extracting

    #. 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:


Done another way, but I will read it anyway later.

    #.      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?

Only after you mentioned it I paid attention to this, guess its because
there is a new line in this occation from $_ (or the variable I put there)
AND from say, right? solved it by replacing say with print (at least I
think I solved it).

    #.      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 ]  );




            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

    Please reply to list if it's a mailing list post - http://shlom.in/reply.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://mail.perl.org.il/pipermail/perl/attachments/20120930/2dbc4413/attachment.htm 

More information about the Perl mailing list