<div dir="ltr">Hi, comments are in post<br><br>On Sun, Sep 23, 2012 at 8:42 PM, Shlomi Fish &lt;<a href="mailto:shlomif@shlomifish.org">shlomif@shlomifish.org</a>&gt; 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 &quot;#!&quot; and the &quot;use warnings&quot;.<br><br><br>Done.<br><br><br>    #. &lt;STDIN&gt; should preferably just be &lt;&gt;:<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 &lt;&gt; instead of &lt;STDIN&gt;? If I use &lt;&gt; it doesn&#39;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 &lt;&gt;.<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&#39;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&#39;s and con&#39;s for doing this in this way? at least I will learn something :)<br><br><br>    #. You have &quot;html { &quot; , &quot;down {&quot; ,etc. Are these supposed to be subroutines?<br>
    Where is the &quot;sub&quot; keyword?<br><br><br>Added the sub keyword and started to work on OO version.<br><br><br>    #. &quot;$dir/mail.txt&quot; should preferably be done using File::Spec-&gt;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>    #. &lt;&lt; push @files, $gzip if $gzip =~ /$year/; &gt;&gt; $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&#39;t understand the link you gave. Can you explain?<br>
<br>    #. print &quot;downloading and extracting the files, please be<br>    patient...\n\n&quot;;   - 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>    &lt;&lt;&lt;<br>    for ( @files ) {<br>    system(&quot;wget -P $dir $_&quot;) if $_ =~ /$year/;<br>    my $ur = $_;<br>    &gt;&gt;&gt;<br><br>    Don&#39;t misues $_ for loops in production code. Also, use a more meaningful name than &quot;ur&quot;.<br>
<br>Done, including the name change.<br><br>    Finally, you can use LWP instead of shelling out to wget.<br><br>    #. system(&quot;gzip -d $dir/*.gz&quot;);<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, &quot;&gt;&quot;, &quot;$dir$file_name&quot; or die &quot;can&#39;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 &quot;.&quot; or $file_name eq &quot;..&quot;;<br><br>    Use File::Spec-&gt;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&#39;t misuse $_. It gets clobbered easily.<br><br>    #.      while ( &lt;$FILE&gt; ) {<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&#39;s a mailing list post - <a href="http://shlom.in/reply">http://shlom.in/reply</a> . <br></div>