<div dir="ltr">Hi, comments are in post<br><br>On Sun, Sep 23, 2012 at 8:42 PM, Shlomi Fish <<a href="mailto:shlomif@shlomifish.org">shlomif@shlomifish.org</a>> wrote:<br><br> Hi Moshe!<br><br> Here are my comments for:<br>
<br> <a href="https://github.com/moshe742/mail-stats/blob/master/mail-stats.pl">https://github.com/moshe742/mail-stats/blob/master/mail-stats.pl</a><br><br>First of all, thanks for the feedback, till now only you and gaal gave feedback.<br>
At least sawyer and Gabor said they will make it OO and refactoring for the code.<br><br> First of all, you should read:<br><br> <a href="http://perl-begin.org/tutorials/bad-elements/">http://perl-begin.org/tutorials/bad-elements/</a><br>
<br>I will read it soon.<br> <br><br> Now for the stuff:<br><br> #. There should be a line spacing between the sha-bang "#!" and the "use warnings".<br><br><br>Done.<br><br><br> #. <STDIN> should preferably just be <>:<br>
<br> <a href="http://perl-begin.org/tutorials/bad-elements/#STDIN_instead_of_ARGV">http://perl-begin.org/tutorials/bad-elements/#STDIN_instead_of_ARGV</a><br><br><br>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...<br>
<br><br> #. As a general rule I prefer inputting input using @ARGV instead of <>.<br><br><br>Done both of the options, now you can put the information through file, @ARGV or STDIN<br><br><br> #. If you do insist on using command line input, consider using Term::ReadLine :<br>
<br> <a href="http://perldoc.perl.org/Term/ReadLine.html">http://perldoc.perl.org/Term/ReadLine.html</a><br><br><br>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.<br>
Any way, what are the pro's and con's for doing this in this way? at least I will learn something :)<br><br><br> #. You have "html { " , "down {" ,etc. Are these supposed to be subroutines?<br>
Where is the "sub" keyword?<br><br><br>Added the sub keyword and started to work on OO version.<br><br><br> #. "$dir/mail.txt" should preferably be done using File::Spec->catfile.<br><br><br>
Done.<br><br><br> #. You should parse HTML using a module:<br><br> <a href="http://perl-begin.org/uses/text-parsing/">http://perl-begin.org/uses/text-parsing/</a><br><br><br>Done.<br><br><br> #. << push @files, $gzip if $gzip =~ /$year/; >> $year will be interpolated as<br>
a small regex. You may want to use \Q and \E or better yet - <a href="https://duckduckgo.com/?q=perl%20index">https://duckduckgo.com/?q=perl%20index</a><br><br>Didn't understand the link you gave. Can you explain?<br>
<br> #. print "downloading and extracting the files, please be<br> patient...\n\n"; - the message should start with a Capital letter.<br> <br>Thanks, done, and if there are more problems with my grammar writing please tell me.<br>
<br> #.<br><br> <<<<br> for ( @files ) {<br> system("wget -P $dir $_") if $_ =~ /$year/;<br> my $ur = $_;<br> >>><br><br> Don't misues $_ for loops in production code. Also, use a more meaningful name than "ur".<br>
<br>Done, including the name change.<br><br> Finally, you can use LWP instead of shelling out to wget.<br><br> #. system("gzip -d $dir/*.gz");<br><br> This is potentially dangerous. Try using Compress::ZLib instead.<br>
<br>Now using LWP and IO::Uncompress::Gunzip for downloading and extracting respectively.<br><br> #. open my $FILE05, ">", "$dir$file_name" or die "can't open<br><br> Why is it called $FILE05? What is the meaning of the 05 digits?<br>
<br>Done.<br><br> #. next if $file_name eq "." or $file_name eq "..";<br><br> Use File::Spec->no_upwards instead:<br><br> <a href="http://perl-begin.org/tutorials/bad-elements/#no_upwards_for_dirs">http://perl-begin.org/tutorials/bad-elements/#no_upwards_for_dirs</a><br>
<br>Done another way, but I will read it anyway later.<br><br> #. for ( @file ) {<br><br> Again , don't misuse $_. It gets clobbered easily.<br><br> #. while ( <$FILE> ) {<br> say $FILE05 $_;<br>
}<br><br> Wrap $FILE05 with { ... } and are you aware that you are printing<br> it with double newline?<br> <br>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).<br>
<br> #. my $num_of_mails = 0;<br><br> for ( 1 .. $number ) {<br> $num_of_mails = $sort[-$_] + $num_of_mails;<br> }<br><br><br> Just use List::Util::sum with an array slice.<br><br> my $num-of_mails = sum ( @sort[ -$number .. -1 ] );<br>
<br>Done.<br><br> ===============<br><br> Regards,<br><br> Shlomi Fish<br><br> --<br> -----------------------------------------------------------------<br> Shlomi Fish <a href="http://www.shlomifish.org/">http://www.shlomifish.org/</a><br>
Why I Love Perl - <a href="http://shlom.in/joy-of-perl">http://shlom.in/joy-of-perl</a><br><br> Tcl is Lisp on drugs. Using strings instead of S‐expressions for closures is<br> Evil with one of those gigantic E’s you can find at the beginning of chapters.<br>
<br> Please reply to list if it's a mailing list post - <a href="http://shlom.in/reply">http://shlom.in/reply</a> . <br></div>