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

[Shlomi Fish]

Hi Moshe,

see below for my comments.

On Sun, 30 Sep 2012 13:00:34 +0200
moshe nahmias <moshegrey at ubuntu.com> wrote:

> 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:
> 
>     https://github.com/moshe742/mail-stats/blob/master/mail-stats.pl
> 
> First of all, thanks for the feedback, till now only you and gaal gave
> feedback.
> At least sawyer and Gabor said they will make it OO and refactoring
> for the code.

OK.

> 
>     #. <STDIN> should preferably just be <>:
> 
>     http://perl-begin.org/tutorials/bad-elements/#STDIN_instead_of_ARGV
> 
> 
> 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...

Well, <> allows one to read from files in the command-line arguments.

> 
> 
>     #. 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, @ARGV or STDIN

OK.

> 
> 
>     #. If you do insist on using command line input, consider using
> Term::ReadLine :
> 
>     http://perldoc.perl.org/Term/ReadLine.html
> 
> 
> 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 :)


Term::ReadLine allows you to input lines using command-line editing (similar
to what Bash and other interactive http://en.wikipedia.org/wiki/Read%E2%80%93eval%E2%80%93print_loop
have). See:

* http://en.wikipedia.org/wiki/GNU_readline

(Also see the replacements there in the external links.)

> 
> 
>     #. You have "html { " , "down {" ,etc. Are these supposed to be
> subroutines?
>     Where is the "sub" keyword?
> 
> 
> Added the sub keyword and started to work on OO version.

Nice.

> 
>     #. << 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
> 
> Didn't understand the link you gave. Can you explain?
> 

See:

http://perldoc.perl.org/functions/index.html

You can do if (index($gzip, $year) >= 0).

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

OK.

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

Yes, exactly.

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

OK.

Regards,

	Shlomi Fish

-- 
-----------------------------------------------------------------
Shlomi Fish       http://www.shlomifish.org/
http://www.shlomifish.org/humour/ways_to_do_it.html

And the top story for today: wives live longer than husbands because they are
not married to women.
    — Colin Mochrie in Whose Line is it, Anyway?

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