<div dir="ltr"><div class="gmail_quote">On Sun, Aug 5, 2012 at 10:18 PM, moshe nahmias <span dir="ltr">&lt;<a href="mailto:moshegrey@ubuntu.com" target="_blank">moshegrey@ubuntu.com</a>&gt;</span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div dir="ltr">First of all, thanks for your comments.<br><br><div class="gmail_quote"><div class="im">On Sun, Aug 5, 2012 at 10:32 AM, Gaal Yahas <span dir="ltr">&lt;<a href="mailto:gaal@forum2.org" target="_blank">gaal@forum2.org</a>&gt;</span> wrote:<br>

<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr">I wanted to mention one other thing -- how do you know this thing works?<div><br></div><div>When you&#39;re developing it, you probably ran it several times. This is fine to do occasionally, but it has a few problems:</div>


<div>- it puts load on Gabor&#39;s servers</div><div>- it&#39;s slow! you have to fetch a bunch or resources from the network every time and uncompress them.</div><div>- when you have a bug, you have to figure out what&#39;s wrong from looking at the output of the entire program. This can be hard.</div>


<div>- it&#39;s possible you have a bug in the statistics code that doesn&#39;t crash the program, but which causes the wrong output! You&#39;d never find it by looking at the report, unless you had some prior knowledge about the actual stats.</div>


<div><br></div><div>Testing is a big subject, and I don&#39;t know how well you&#39;re already familiar with it, so I&#39;ll just again give a few general remarks:</div><div><br></div><div><div>- The main point I&#39;m making here is that your script is a single flow of code, and if you break it down to functions (sub get_page, sub analyze_page etc.) you can cause only specific parts of your code to run, with input that&#39;s under your control, and verify that the output is what you expect.</div>


</div><div><br></div><div>- You might be tempted to comment out pieces of code / add flags like $TEST or whatever -- it&#39;s okay here and there during development, but it doesn&#39;t scale well down the line. You end up with messy code or things you have to clean up before release, and have to repeat some work every time you want to test something.</div>


<div><br></div><div>- Whatever works. Perl has spectacular support for testing, but it means it&#39;s easy to get overwhelmed by the options. Start with Test::More or something else that&#39;s well-known.</div><div><div>

<div><br></div></div></div></div></blockquote></div><div><br>Right, I really need to start making tests for this and do it in parts, I think this will be for my next version of the script (tests and building it in smaller chunks).<br>
</div></div></div></blockquote><div><br></div><div>Sure. One nice thing about testing is that there are gains to be had even if you only have very simple tests -- any addition can be an improvement, but you don&#39;t have to do it all at once.</div>
<div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div class="gmail_quote"><div>
 <br></div><div class="im"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div><div><div></div>
<div><div class="gmail_quote">On Sun, Aug 5, 2012 at 9:51 AM, Gaal Yahas <span dir="ltr">&lt;<a href="mailto:gaal@forum2.org" target="_blank">gaal@forum2.org</a>&gt;</span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">


<div dir="ltr"><div>Since you asked for comments, here are a few general ones.</div><div><br></div><div>- You use autodie, but aren&#39;t telling it to capture errors in system. The most likely part of your program to fail is the network part (what you currently delegate to wget), so it&#39;s probably the one you want the most control over. Currently you&#39;d fail silently. If you say &quot;use autodie qw(:all)&quot; you&#39;d die on the first wget failure. What would you like to do? I&#39;m guessing that ideally, you should retry (or have wget retry). But at the very least, you want good error messages.    To summarize this point: don&#39;t give up contorl of error reporting in the heart of what you&#39;re trying to accomplish.</div>



<div> <br></div></div></blockquote></div></div></div></div></div></blockquote></div><div>OK, so I removed the autodie, but can i use it somehow and still have my own warnings? (want to learn, so a link will be good enough).<br>
</div></div></div></blockquote><div><br></div><div>Sure. The autodie manual says,</div><div><br></div><div><div>       The &quot;autodie&quot; pragma has lexical scope, meaning that functions and</div><div>       subroutines altered with &quot;autodie&quot; will only change their behaviour</div>
<div>       until the end of the enclosing block, file, or &quot;eval&quot;.</div></div><div><br></div><div>This means you can do:</div><div><br></div><div>use autodie qw(:all);  # at the top of your program</div><div><br>
</div><div>open my $fh, &quot;&lt;&quot;, $filename;  # benefits from autodie</div><div><br></div><div>{</div><div>    no autodie;</div><div>    open my $other_fh, &quot;&lt;&quot;, $otherfile or die &quot;my own error message: $!&quot;;</div>
<div>}</div><div><br></div><div>close $fh;  # autodie again, because we&#39;ve left the scope.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr">
<div class="gmail_quote"><div>
About the wget, I meant the program on linux distro&#39;s, not something on perl itself.<br>How do I get the errors of a system error? I use system() for the wget and gzip so if there is an error i can see it in the terminal, but if I will be able to get the errors it can help if and when there will be problems and save them for a time I can look at them. <br>
</div></div></div></blockquote><div><br></div><div>system&#39;s error values are pretty unusual. &quot;perldoc -f system&quot; will give you the details, but at the very least expect 0 for success, something else is failure.</div>
<div><br></div><div>A modern alternative is IPC::Cmd - <a href="http://perldoc.perl.org/IPC/Cmd.html">http://perldoc.perl.org/IPC/Cmd.html</a> - the example there even uses wget!</div><div><br></div><div>Corelist says this is a standard module since Perl 5.9.5, so it&#39;s likely that you can use it.</div>
<div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div class="gmail_quote"><div>
</div><div class="im"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div><div><div><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">

<div dir="ltr"><div></div><div>- You&#39;re using use strict, which is very good, but notice that the way your file handles are named, they are unchecked globals. Avoid all-uppercase, sigilless names (FILE and so one) if you want protection; use lexicals instead:</div>



<div><br></div><div>  open my $fh, &quot;&lt;&quot;, &quot;myfile&quot; or die ...</div><div><br></div><div>(BTW, you have or die here and there, despite autodie. I&#39;d recommend for now just getting rid of autodie entirely and doing all the errors yourself.)</div>



<div><br></div><div>When you use lexical filehandles, the files are closed automatically when the variable goes out of scoped. This is great for reads. For writes, you should always close the file yourself to check for errors. There are fancy ways of doing that automatically but never mind them for now.</div>



<div><br></div></div></blockquote></div></div></div></div></div></blockquote></div><div>I understand from your comment that I can check for errors when closing a file, how? (again, if you prefer giving a link that&#39;s OK too).<br>
</div></div></div></blockquote><div><br></div><div>If you are using autodie, just &quot;close $fh&quot; will work. Otherwise, &quot;close $fh or die &quot;close: $!&quot;.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div dir="ltr"><div class="gmail_quote"><div>
 <br></div><div class="im"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div><div><div><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">

<div dir="ltr"><div></div><div>- It&#39;s not unreasonable to use external tools in your script. Whatever gets the job done. Of course there are tradeoffs: you comment that you don&#39;t know if these tools are available on other platforms. Well, modules for gzip and http may not be available on all platforms either -- take a look at corelist if you want to make educated guesses about what to expect.</div>



<div><br></div></div></blockquote></div></div></div></div></div></blockquote></div><div>Didn&#39;t understand what you mean, I used the linux programs wget and gzip in this script, thus i don&#39;t know if those programs are available for windows for example, thus I don&#39;t know if it will work on windows.<br>
</div></div></div></blockquote><div><br></div><div>This stuff is up to you -- you can&#39;t support every system in the world, and if you don&#39;t know yet where your script will be running your safest bet is to stick to modules that come bundled with Perl itself (the corelist). Today, that&#39;s quite a lot of stuff. For example, HTTP::Tiny can fetch things from the network. And you can read gzipped files from Perl with IO::Uncompress::Gunzip.</div>
<div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div class="gmail_quote"><div>
 <br></div><div class="im"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div><div><div><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">

<div dir="ltr"><div></div><div>- Syntactically, this:</div><div><br></div><div>  if (CONDITION 1) {</div><div>    CODE 1</div><div>  } if (CONDITION 2) {</div><div>    CODE 2</div><div>  } if (CONDITION 3) {</div><div>    CODE 3</div>



<div>  } ...</div><div><br></div><div>  checks all the conditions and may run all the branches. I think you meant &quot;elsif&quot; there.</div><div><br></div></div></blockquote></div></div></div></div></div></blockquote>

</div><div>I use it this way because it doesn&#39;t get the right numbers and make mistakes if I use elsif (at least on some of the years, don&#39;t remember why).<br></div></div></div></blockquote><div><br></div><div>The only way this can be possible is if for some inputs, more than one condition is true. What&#39;s happening then is that the last condition wins.</div>
<div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div class="gmail_quote"><div> <br></div><div class="im"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">

<div dir="ltr"><div><div><div><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div></div><div>- Semantically, what you&#39;re doing is extracting a bunch of values from a hash into a number of scalars. Why? This has a few costs:</div>



<div><br></div><div>1. You have to pick names for these. If you ever scratch your head about the name of a variable, and end up with something mundane like &quot;first&quot;, something&#39;s wrong. (I mean, first is perfectly all right if its scope is very small, and within that block of code it&#39;s clear what it&#39;s intended to refer to. But here the scope of these variables is pretty much the whole program, and the reader is confounded with arbitrary distinctions like &quot;first&quot; and &quot;firs&quot;.</div>



<div><br></div></div></blockquote></div></div></div></div></div></blockquote></div><div>I have refactored the names, it&#39;s better, not great.<br>The reason for the names is that $first is the one with the most mails on this year, so may be the name isn&#39;t great but I don&#39;t know if there is a much better choice. I am open to suggestions.<br>
</div></div></div></blockquote><div><br></div><div>You have a collection of people, who you are putting in some order. To me this suggests an array, where the first person is the most prolific author, the second is the second most prolific, and so on. The decision to cut off after 10 authors is artificial, so ideally it should be a parameter and there should be exactly one place in the code that you have to change in order to instead see the top 3 or 20 or whatever.</div>
<div><br></div><div>So how about @authors, or @authors_by_volume? Then $authors[0] is your $first, $authors[1] is your $second, $authors[-1] is your $tenth and so on.</div><div> </div><div>Oh wait, this is a collection of their scores, not their names. Your @sort is *all* the authors by volume. (Bad name, probably.) So you need a parallel array with their scores. Here are two ways to do it:</div>
<div><br></div><div># %names is also a bad name. It stores name =&gt; score elements, so %score_by_name, or just %scores if you can get rid of @scores. More on this below.</div><div><br></div><div>my $cutoff = 10;</div><div>
my @scores;</div><div>die &quot;gaal was too lazy to handle the case where there are few authors&quot; if @sorted &lt; $cutoff;</div><div>for my $i (0 .. $cutoff - 1) {</div><div>  push @scores, $names{ $sorted[$i] };</div>
<div>}</div><div><br></div><div>Another way is to use slices. If I have a collection and want to get a subset of it, I can use multiple keys at once to get multiple values.</div><div><br></div><div>@scores_for_moshe_and_gaal = @names{&quot;moshe&quot;, &quot;gaal&quot;};  # or with fancier syntax,</div>
<div>@scores_for_moshe_and_gaal = @names{qw/moshe gaal/};</div><div><br></div><div>So if we had @top_10_names = qw/gabor moshe .../;, we could do</div><div><br></div><div>@top_10_scores = @names{ @top_10_names };</div><div>
<br></div><div>But I said I don&#39;t like hardcoding the value &quot;10&quot; anywhere, it&#39;s arbitrary. So you can slice both the name-to-score hash, and the top name array!</div><div><br></div><div>@top_scores = @names{ @sorted[0 .. $cutoff - 1] };</div>
<div><br></div><div>You&#39;re still doing ten ($cutoff) hash lookups behind the scenes, and in fact are creating a temporary array of size 10 for the smaller sorted authors collection. But 10 is better than 100, and 1 line of code is nicer than 20.</div>
<div><br></div><div>Yet another way is to use map:</div><div><br></div><div>@top_scores = map $names{$_}, @sorted[0 .. $cutoff - 1];</div><div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div dir="ltr"><div class="gmail_quote"><div>
<br></div><div class="im"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div><div><div><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">

<div dir="ltr"><div></div><div>2. It&#39;s inefficient in code. You have a 20 lines of code that aren&#39;t furthering your goal as a programmer here. Couldn&#39;t the user of this data may as well just say $names{$some_name}?</div>



<div><br></div></div></blockquote></div></div></div></div></div></blockquote></div><div>The point of these 20 lines is to get the numbers of the most active participants, I guess there are other ways (and may be better ways) but I don&#39;t know them yet.<br>

 <br></div><div class="im"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div><div><div><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">

<div dir="ltr"><div></div><div>3. It&#39;s inefficient in performance. Your for loop runs as many times as there are elements in %names, and performs ten conditions. So you have 100 branches in your code where you didn&#39;t need any. In this case, it doesn&#39;t matter; but you have to keep these things in mind for when you have larger data structures.</div>



<div><br></div></div></blockquote></div></div></div></div></div></blockquote></div><div>So, what you say is that i should find a way to make it run only on the &quot;right&quot; parts of my variable? the big question is how can I do that?<br>
</div></div></div></blockquote><div><br></div><div>You aren&#39;t ever printing the top scores, just counting them for statistics. So really even a collection of the top scores is more than you need. So you can do:</div><div>
<br></div><div><div>my $top_posters_contribution;</div><div>for my $i (0 .. $cutoff - 1) {</div><div>  $top_posters_contribution += $names{ $sorted[$i] };</div><div>}</div><div><br></div><div>Same thing with fancier syntax:</div>
<div><br></div><div>use List::Util qw(sum);</div><div>my $top_posters_contribution = sum map $names{$_}, @sorted[0 .. $cutoff - 1];</div></div><div><br></div><div>You should certainly only use the styles that you like. There&#39;s more than one way to do it! The shortest one isn&#39;t necessarily the best. But try to avoid wasteful ways.</div>
<div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div class="gmail_quote"><div>
 <br></div><div class="im"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div><div><div><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">

<div dir="ltr"><div></div><div>- Again on naming, &quot;percent&quot; vs. &quot;per&quot; -- the reader can&#39;t tell what these mangled names are for.</div><div><br></div><div>- I see you&#39;re using &quot;say&quot;, which is a new language feature. If you&#39;re doing that, you may as well know about and use given/when instead of if/else, though like I said, it&#39;s not actually required here.</div>



<div><br></div><div><br></div><div>Keep having fun,</div><div>And I hope this is useful,</div><div>Gaal</div><br></div></blockquote></div></div></div></div></div></blockquote></div><div>Thanks again for your comments, it&#39;s very helpful and I think I learnt some things already :)<br>

 <br></div><div class="im"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div><div><div><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">

<div dir="ltr"><div class="gmail_quote"><div><div>On Sun, Aug 5, 2012 at 3:39 AM, moshe nahmias <span dir="ltr">&lt;<a href="mailto:moshegrey@ubuntu.com" target="_blank">moshegrey@ubuntu.com</a>&gt;</span> wrote:<br>

</div></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div><div><div dir="ltr">Hi all,<br>I just uploaded my first little program to github. I want your feedback on the script.<br>


the script is intended to make the statistics for mailing groups from their archives, for now it only works on linux (or at least if wget and gzip are installed).<br>

<br>So, what do you think?<br>I know its not perfect so feel free to teach me, after all, that&#39;s one of the reasons to publish this :)<br>the link: <a href="https://github.com/moshe742/mail-stats.git" target="_blank">https://github.com/moshe742/mail-stats.git</a><span><font color="#888888"><br>




<br>Moshe<br></font></span></div>
<br></div></div>_______________________________________________<br>
Perl mailing list<br>
<a href="mailto:Perl@perl.org.il" target="_blank">Perl@perl.org.il</a><br>
<a href="http://mail.perl.org.il/mailman/listinfo/perl" target="_blank">http://mail.perl.org.il/mailman/listinfo/perl</a><span><font color="#888888"><br></font></span></blockquote></div><span><font color="#888888"><br>
<br clear="all"><div><br></div>-- <br>Gaal Yahas &lt;<a href="mailto:gaal@forum2.org" target="_blank">gaal@forum2.org</a>&gt;<br>
<a href="http://gaal.livejournal.com/" target="_blank">http://gaal.livejournal.com/</a><br>
</font></span></div>
</blockquote></div><br><br clear="all"><div><br></div>-- <br>Gaal Yahas &lt;<a href="mailto:gaal@forum2.org" target="_blank">gaal@forum2.org</a>&gt;<br><a href="http://gaal.livejournal.com/" target="_blank">http://gaal.livejournal.com/</a><br>


</div></div></div></div>
<br>_______________________________________________<br>
Perl mailing list<br>
<a href="mailto:Perl@perl.org.il" target="_blank">Perl@perl.org.il</a><br>
<a href="http://mail.perl.org.il/mailman/listinfo/perl" target="_blank">http://mail.perl.org.il/mailman/listinfo/perl</a><br></blockquote></div></div><br></div>
<br>_______________________________________________<br>
Perl mailing list<br>
<a href="mailto:Perl@perl.org.il">Perl@perl.org.il</a><br>
<a href="http://mail.perl.org.il/mailman/listinfo/perl" target="_blank">http://mail.perl.org.il/mailman/listinfo/perl</a><br></blockquote></div><br><br clear="all"><div><br></div>-- <br>Gaal Yahas &lt;<a href="mailto:gaal@forum2.org">gaal@forum2.org</a>&gt;<br>
<a href="http://gaal.livejournal.com/">http://gaal.livejournal.com/</a><br>
</div>