Hi Moshe, see below for my comments.
On Sun, 30 Sep 2012 13:00:34 +0200 moshe nahmias <[email protected]> wrote: > Hi, comments are in post > > On Sun, Sep 23, 2012 at 8:42 PM, Shlomi Fish <[email protected]> > 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 . _______________________________________________ Perl mailing list [email protected] http://mail.perl.org.il/mailman/listinfo/perl
